-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementing Asynchronous Advertising ID Retrieval for H5vccSystem #4794
Conversation
5682353
to
c381dea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than one (large) rename. Were you able to test this i.e. via devtools?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking close, can be simplified and also needs plugging the JS Mojo Mock.
third_party/blink/renderer/modules/cobalt/h5vcc_system/h_5_vcc_system.cc
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one comment and also agree with Miguel that the web test should be updated. Otherwise, looks good!
third_party/blink/renderer/modules/cobalt/h5vcc_system/h_5_vcc_system.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a TODO(b/...) in void H5vccSystemImpl::GetAdvertisingId() to try and remove the Mojo-to-C++-to-Java (in favour of Mojo-to-Java directly). @andrewsavage1 volunteered to do so, but adding a bug and a TODO should be fast.
(I think I left a comment like this but perhaps it was in #4853).
a73bbab
to
272d2a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one minor comment but otherwise lgtm.
...y/blink/web_tests/wpt_internal/h5vcc/h5vcc-system/h5vcc-system-getAdvertisingId.https.any.js
Outdated
Show resolved
Hide resolved
This reverts commit c784b5b.
Linux builds are failing with a real build error - missing |
You'll likely need to depend on Alternatively you could depend on |
@yell0wd0g IIUC you're suggesting that for cross-platform Mojo interfaces, like this one, we have both:
I was looking into Mojo's Java bindings and saw these recommendations from https://chromium.googlesource.com/chromium/src/mojo/+/HEAD/public/java/bindings/README.md#should-i-or-should-i-not-use-the-java-bindings:
From that doc it sounds like one of the primary reasons for that recommendation is that the Java bindings are missing some features: https://chromium.googlesource.com/chromium/src/mojo/+/HEAD/public/java/bindings/README.md#notable-lacking-features. So do you think we should include both Java and C++ Mojo impls for Mojo interfaces where we don't need those features, and just C++ Mojo impls where we do? Are there examples from upstream where both Java and C++ implementations are provided for cross-platform Mojo interfacaes? |
Tested on devtools, https://screenshot.googleplex.com/43HotkbLgJSQdx3
// Compile the web_test
cobalt/build/gn.py -p linux-x64x11 -C devel &&
autoninja -C out/linux-x64x11_devel blink_wpt_tests &&
autoninja -C out/linux-x64x11_devel dump_syms &&
autoninja -C out/linux-x64x11_devel minidump_stackwalk &&
autoninja -C out/linux-x64x11_devel cobalt/browser/h5vcc_system/public/mojom:mojom_js
// run the web_test
third_party/blink/tools/run_web_tests.py -t linux-x64x11_devel wpt_internal/h5vcc/h5vcc-system
b/377049113