Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1765533004:
[webnfc] Implement watch method for Android nfc mojo service. (Closed)
|
Created:
4 years, 9 months ago by shalamov Modified:
4 years ago Reviewers:
kenneth.christiansen, Reilly Grant (use Gerrit), Ken Rockot(use gerrit already), kenneth.r.christiansen, dcheng, Ted C, Tom Sepez CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@implement_nfc_watch_in_blink Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[webnfc] Implement watch method for Android nfc mojo service.
Implementation of nfc.watch [1] and nfc.cancelWatch [2] methods
for Android nfc mojo service that is used from W3C WebNFC module.
[1] https://w3c.github.io/web-nfc/#dom-nfc-watch
[2] https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch
BUG=520391
BUG=625589
Committed: https://crrev.com/55ac6a4ca3780a5c315a7014f43642e43c5245f0
Cr-Commit-Position: refs/heads/master@{#435189}
Patch Set 1 : Rebased to master. #Patch Set 2 : Rebased. #
Total comments: 5
Patch Set 3 : Rebased to CL 1486043002 #Patch Set 4 : Handle ignoreRead flag. #Patch Set 5 : Fixes for Kenneth's comments + Rebased to CL crrev.com/1486043002 #Patch Set 6 : Rebase and add size limit check when tag is read. #
Total comments: 20
Patch Set 7 : Fixes for comments from dcheng + unit tests. #
Total comments: 8
Patch Set 8 : Fixes for Reilly's comments #
Total comments: 5
Patch Set 9 : Fixes for review comments from Reilly #Patch Set 10 : Rebased to master #
Total comments: 2
Messages
Total messages: 57 (36 generated)
Patchset #1 (id:1) has been deleted
alexander.shalamov@intel.com changed reviewers: + kenneth.christiansen@gmail.com, kenneth.r.christiansen@intel.com
PTAL.
https://codereview.chromium.org/1765533004/diff/40001/device/nfc/android/java... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1765533004/diff/40001/device/nfc/android/java... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:306: Log.w(TAG, "Cannot read data to NFC tag. IO_ERROR."); 'read to' sounds wrong https://codereview.chromium.org/1765533004/diff/40001/device/nfc/android/java... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:339: boolean matchedMediaType = true; maybe a comment why these are true by default (not obvious) https://codereview.chromium.org/1765533004/diff/40001/device/nfc/android/java... File device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java (right): https://codereview.chromium.org/1765533004/diff/40001/device/nfc/android/java... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:84: Log.w(TAG, "Unknown charset, using UTF-8 by default."); defaulting to UTF-8 ?
Description was changed from ========== [webnfc] Implement watch method for Android nfc mojo service. Implementation of nfc.watch [1] and nfc.cancelWatch [2] methods for Android nfc mojo service that is used from W3C WebNFC module. [1] https://w3c.github.io/web-nfc/#dom-nfc-watch [2] https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch BUG=520391 ========== to ========== [webnfc] Implement watch method for Android nfc mojo service. Implementation of nfc.watch [1] and nfc.cancelWatch [2] methods for Android nfc mojo service that is used from W3C WebNFC module. [1] https://w3c.github.io/web-nfc/#dom-nfc-watch [2] https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch BUG=520391 BUG=625589 ==========
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/1765533004/diff/40001/device/nfc/android/java... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1765533004/diff/40001/device/nfc/android/java... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:306: Log.w(TAG, "Cannot read data to NFC tag. IO_ERROR."); On 2016/04/25 17:55:32, kenneth.christiansen wrote: > 'read to' sounds wrong Done. https://codereview.chromium.org/1765533004/diff/40001/device/nfc/android/java... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:339: boolean matchedMediaType = true; On 2016/04/25 17:55:32, kenneth.christiansen wrote: > maybe a comment why these are true by default (not obvious) Done.
alexander.shalamov@intel.com changed reviewers: + reillyg@chromium.org
alexander.shalamov@intel.com changed reviewers: + dcheng@chromium.org, nick@chromium.org
dcheng, Could ypu please take a look at device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java I've also added size limit check, now NFC messages that are over 32KB cannot be read / written to NFC tags (32KB is max size for tags according to NFC forum specification).
alexander.shalamov@intel.com changed reviewers: + tedchoc@chromium.org - nick@chromium.org
Also, it would have been nice if both ends (https://codereview.chromium.org/1759373003/) were in the same CL; that's what we try to generally recommend to help the mojo security reviewers https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:230: if (mWatchers.indexOfKey(id) >= 0) { Nit: it's more common to structure code as: if (error condition) { /* handle error */ return; } if (another error condition) { /* handle another error */ return; } /* handle success case */ https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:253: callback.call(createError(NfcErrorType.NOT_FOUND)); Is it really an error to call this if the are no active watches? Also, I don't see this in the spec nor does it seem to be referenced atm. Is this for testing only? https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:385: private void enableReaderMode() { Isn't this more like "enableReaderModeIfNeeded"? https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:391: if (mPendingPushOperation == null && mWatchers.size() == 0) return; Nit: isEmpty() https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:418: if (mPendingPushOperation == null && mWatchers.size() == 0) { Why don't we just always do these checks enabling / disabling? It seems like they ought to be matched. https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:465: * Reads NfcMessage from a tag and forwards message to mathcing method. Nit: mathcing => matching https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:528: if (options.mode == NfcWatchMode.WEBNFC_ONLY && message.url.isEmpty()) return false; I would expect validation like this to occur as soon as we try to add a watch? Is this not considered an error in the API exposed to the web? https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java (right): https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:60: throws UnsupportedEncodingException { If we're going to enforce max message size, we should be enforcing it in the browser side (and ultimately triggering the promise rejection from here) https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:73: if (nfcRecord != null) nfcRecords.add(nfcRecord); What does it mean for toNfcRecord() to return null? Shouldn't that be considered an error of sorts? https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:145: return new NdefMessage(new NdefRecord(NdefRecord.TNF_EMPTY, null, null, null)); I'm not sure how common this is in Chromium code, but I feel like having a builder pattern here would be really nice... (here and below as well)
On 2016/09/08 05:02:32, dcheng wrote: > Also, it would have been nice if both ends > (https://codereview.chromium.org/1759373003/) were in the same CL; that's what > we try to generally recommend to help the mojo security reviewers Thanks for comments dcheng@, I will address those shortly and merge blink and browser side into one.
Patchset #7 (id:160001) has been deleted
dcheng@ I added unit tests, similar to mock tests on blink side in https://codereview.chromium.org/1759373003 do you still want me to merge these two CLs? https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:230: if (mWatchers.indexOfKey(id) >= 0) { On 2016/09/08 05:02:32, dcheng wrote: > Nit: it's more common to structure code as: > > if (error condition) { > /* handle error */ > return; > } > if (another error condition) { > /* handle another error */ > return; > } > /* handle success case */ Done. https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:253: callback.call(createError(NfcErrorType.NOT_FOUND)); On 2016/09/08 05:02:31, dcheng wrote: > Is it really an error to call this if the are no active watches? Also, I don't > see this in the spec nor does it seem to be referenced atm. Is this for testing > only? Done. https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:385: private void enableReaderMode() { On 2016/09/08 05:02:31, dcheng wrote: > Isn't this more like "enableReaderModeIfNeeded"? Done. https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:391: if (mPendingPushOperation == null && mWatchers.size() == 0) return; On 2016/09/08 05:02:32, dcheng wrote: > Nit: isEmpty() SparseArray doesn't have isEmpty(), only size(). https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:418: if (mPendingPushOperation == null && mWatchers.size() == 0) { On 2016/09/08 05:02:32, dcheng wrote: > Why don't we just always do these checks enabling / disabling? It seems like > they ought to be matched. When mojo proxy is closed, tab becomes inactive or browser is minimized, e.g. in suspendNfcOperations(), we have to disable reader mode regardless of pending push (write) / watch (read) operations. Method disableReaderModeIfNeeded is used internally to disable reader mode when all watches are removed or when push is completed. https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:465: * Reads NfcMessage from a tag and forwards message to mathcing method. On 2016/09/08 05:02:31, dcheng wrote: > Nit: mathcing => matching Done. https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:528: if (options.mode == NfcWatchMode.WEBNFC_ONLY && message.url.isEmpty()) return false; On 2016/09/08 05:02:32, dcheng wrote: > I would expect validation like this to occur as soon as we try to add a watch? > Is this not considered an error in the API exposed to the web? This is not an error. From the blink side, developer can pass NfcWatchOptions that will have constraints like, NfcWatchMode.WEBNFC_ONLY, etc. Then device would be waiting for the tag in proximity, when tag is in proximity, message on tag would be read and this matching function is called. This check "message.url.isEmpty()" means that nfc tag doesn't have WebNFC (https://w3c.github.io/web-nfc/#web-nfc-id), therefore message is discarded, since doesn't match criteria. https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java (right): https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:60: throws UnsupportedEncodingException { On 2016/09/08 05:02:32, dcheng wrote: > If we're going to enforce max message size, we should be enforcing it in the > browser side (and ultimately triggering the promise rejection from here) Yes, it is done in another CL. https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:73: if (nfcRecord != null) nfcRecords.add(nfcRecord); On 2016/09/08 05:02:32, dcheng wrote: > What does it mean for toNfcRecord() to return null? Shouldn't that be considered > an error of sorts? According to the spec, records that are not supported are skipped. https://w3c.github.io/web-nfc/#steps-receiving 10. Otherwise, skip to the next NDEF record in input. https://codereview.chromium.org/1765533004/diff/140001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:145: return new NdefMessage(new NdefRecord(NdefRecord.TNF_EMPTY, null, null, null)); On 2016/09/08 05:02:32, dcheng wrote: > I'm not sure how common this is in Chromium code, but I feel like having a > builder pattern here would be really nice... (here and below as well) NfcRecord and NfcMessage are final, I can't add static builder methods to them and new NdefRecord(NdefRecord.TNF_EMPTY, null, null, null), it is from Android APIs and there are builder methods, but for some reason NdefRecord.createEmpty(); is missing.
https://codereview.chromium.org/1765533004/diff/180001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1765533004/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:220: callback.call(mWatcherId, null); This logic makes me nervous. I suggest instead: int watcherId = mWatcherId++; mWatchers.put(watcherId, options); callback.call(watcherId, null); https://codereview.chromium.org/1765533004/diff/180001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java (right): https://codereview.chromium.org/1765533004/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:192: int langCodeLength = (text[0] & (byte) 0xFF); Can you add a comment pointing to where the format of this payload is defined? Do we trust that langCodeLength will be less than text.length? Otherwise Arrays.copyOfRange() will throw an exception. https://codereview.chromium.org/1765533004/diff/180001/device/nfc/android/jun... File device/nfc/android/junit/src/org/chromium/device/nfc/NFCTest.java (right): https://codereview.chromium.org/1765533004/diff/180001/device/nfc/android/jun... device/nfc/android/junit/src/org/chromium/device/nfc/NFCTest.java:410: assertEquals(1, mWatchCaptor.getValue().intValue()); The particular watcher ID assigned seems like an implementation detail. Instead perhaps test whether two different watchers get different IDs and that the correct IDs are passed when messages match particular options. It seems like that's what the test below does (except for still depending on particular IDs). https://codereview.chromium.org/1765533004/diff/180001/device/nfc/android/jun... device/nfc/android/junit/src/org/chromium/device/nfc/NFCTest.java:501: assertNull(mErrorCaptor.getValue()); Test that the client is no longer notified after the watch is cancelled?
https://codereview.chromium.org/1765533004/diff/180001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1765533004/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:220: callback.call(mWatcherId, null); On 2016/09/29 04:38:02, Reilly Grant wrote: > This logic makes me nervous. I suggest instead: > > int watcherId = mWatcherId++; > mWatchers.put(watcherId, options); > callback.call(watcherId, null); Done. https://codereview.chromium.org/1765533004/diff/180001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java (right): https://codereview.chromium.org/1765533004/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:192: int langCodeLength = (text[0] & (byte) 0xFF); On 2016/09/29 04:38:02, Reilly Grant wrote: > Can you add a comment pointing to where the format of this payload is defined? > Do we trust that langCodeLength will be less than text.length? Otherwise > Arrays.copyOfRange() will throw an exception. Added reference to the spec and made stricter language code check. Android platform uses & 0xFF bitmask for the language code length for some reason and leaves 7th bit unset. https://codereview.chromium.org/1765533004/diff/180001/device/nfc/android/jun... File device/nfc/android/junit/src/org/chromium/device/nfc/NFCTest.java (right): https://codereview.chromium.org/1765533004/diff/180001/device/nfc/android/jun... device/nfc/android/junit/src/org/chromium/device/nfc/NFCTest.java:410: assertEquals(1, mWatchCaptor.getValue().intValue()); On 2016/09/29 04:38:02, Reilly Grant wrote: > The particular watcher ID assigned seems like an implementation detail. Instead > perhaps test whether two different watchers get different IDs and that the > correct IDs are passed when messages match particular options. It seems like > that's what the test below does (except for still depending on particular IDs). Modified tests to avoid checking exact value. Instead, comparing whether IDs are unique for different watchers. https://codereview.chromium.org/1765533004/diff/180001/device/nfc/android/jun... device/nfc/android/junit/src/org/chromium/device/nfc/NFCTest.java:501: assertNull(mErrorCaptor.getValue()); On 2016/09/29 04:38:02, Reilly Grant wrote: > Test that the client is no longer notified after the watch is cancelled? Done.
Sorry for the delay, this got lost in my email. This lgtm with a couple nits. https://codereview.chromium.org/1765533004/diff/200001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1765533004/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:100: * proximnity, when match algorithm (@see #matchesWatchOptions) returns true, watcher with s/proximnity/proximity/ https://codereview.chromium.org/1765533004/diff/200001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java (right): https://codereview.chromium.org/1765533004/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:197: int langCodeLength = (text[0] & (byte) 0x3F); Check text.length >= 1?
dcheng@ do you have more comments for this CL? https://codereview.chromium.org/1765533004/diff/200001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1765533004/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:100: * proximnity, when match algorithm (@see #matchesWatchOptions) returns true, watcher with On 2016/10/06 04:59:50, Reilly Grant wrote: > s/proximnity/proximity/ Done. https://codereview.chromium.org/1765533004/diff/200001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java (right): https://codereview.chromium.org/1765533004/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:197: int langCodeLength = (text[0] & (byte) 0x3F); On 2016/10/06 04:59:50, Reilly Grant wrote: > Check text.length >= 1? I added early check for text.length, but that should not happen according to the spec.
https://codereview.chromium.org/1765533004/diff/200001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java (right): https://codereview.chromium.org/1765533004/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:197: int langCodeLength = (text[0] & (byte) 0x3F); On 2016/10/06 at 13:47:58, shalamov wrote: > On 2016/10/06 04:59:50, Reilly Grant wrote: > > Check text.length >= 1? > > I added early check for text.length, but that should not happen according to the spec. If the Android API says it can't happen then it's fine to skip the check but if a malicious device can violate the spec and send an empty payload and it gets through to here then it's not.
The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
alexander.shalamov@intel.com changed reviewers: + tsepez@chromium.org
Tom, could you please take a look at: device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java I think it hits *typeconverter* filter, and needs a review from security team.
On 2016/11/28 14:54:43, shalamov wrote: > Tom, could you please take a look at: > > device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java > > I think it hits *typeconverter* filter, and needs a review from security team. I've not reviewed a lot of these .java converters, dcheng should be sufficient.
lgtm but please address the comment in a followup =) https://codereview.chromium.org/1765533004/diff/260001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1765533004/diff/260001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:145: mClient = client; Can we file a bug to get rid of this? Ideally, we should bind this at creation time with a factory getter that creates Nfc and binds NfcClient simultaneously, so we don't need to handle the null |mClient| case elswhere.
https://codereview.chromium.org/1765533004/diff/260001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1765533004/diff/260001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:145: mClient = client; On 2016/11/28 19:46:40, dcheng wrote: > Can we file a bug to get rid of this? Ideally, we should bind this at creation > time with a factory getter that creates Nfc and binds NfcClient simultaneously, > so we don't need to handle the null |mClient| case elswhere. Thank you for looking Daniel, unfortunately, I'm not aware of mechanism for passing data to mojo service at top level interface creation time. The "SetClient" design pattern is quite common for mojo interfaces. https://cs.chromium.org/search/?q=SetClient%5C(+file:(.mojom$)&sq=package:chr... If you have strong opinion against SetClient method and think it must be fixed, I have two possible solutions in mind. 1. Always provide Client interface for Watch method interface Nfc { Watch(NFCWatchOptions options, Client) => (uint32 id, NFCError? error); or Watch(NFCWatchOptions options) => (uint32 id, Client&, NFCError? error); } Will introduce extra complexity / performance issues, since for every call, it would be required to create new instance of Client interface and bind it to message pipe. 2. Introduce top level "factory" interface, e.g. interface Nfc { GetNfcService(Client) => (NfcService?); } interface NfcService { Push Watch ... } This will increase code complexity and allows creation of multiple NfcService instances per frame, thus, I would need to make some sort of per frame singleton that would be wrapped by NfcService. Also mojo already has ServiceRegistry for top level interfaces, it would look strange to have extra wrapper just to provide Client instance. What do you think?
dcheng@chromium.org changed reviewers: + rockot@chromium.org
On 2016/11/29 13:07:23, shalamov wrote: > https://codereview.chromium.org/1765533004/diff/260001/device/nfc/android/jav... > File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): > > https://codereview.chromium.org/1765533004/diff/260001/device/nfc/android/jav... > device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:145: mClient = > client; > On 2016/11/28 19:46:40, dcheng wrote: > > Can we file a bug to get rid of this? Ideally, we should bind this at creation > > time with a factory getter that creates Nfc and binds NfcClient > simultaneously, > > so we don't need to handle the null |mClient| case elswhere. > > Thank you for looking Daniel, unfortunately, I'm not aware of mechanism for > passing data to mojo service at top level interface creation time. The > "SetClient" design pattern is quite common for mojo interfaces. > > https://cs.chromium.org/search/?q=SetClient%5C(+file:(.mojom$)&sq=package:chr... > > If you have strong opinion against SetClient method and think it must be fixed, > I have two possible solutions in mind. Yes, having a SetClient() is generally an anti-pattern. We weren't really strict enough about discouraging this in the past. > > 1. Always provide Client interface for Watch method > > interface Nfc { > Watch(NFCWatchOptions options, Client) => (uint32 id, NFCError? error); > or > Watch(NFCWatchOptions options) => (uint32 id, Client&, NFCError? error); > } > > Will introduce extra complexity / performance issues, since for every call, it > would be required to create new instance of Client interface and bind it to > message pipe. For the reasons you mentioned, we don't want to do this. > > 2. Introduce top level "factory" interface, e.g. > > interface Nfc { > GetNfcService(Client) => (NfcService?); > } > > interface NfcService { > Push > Watch > ... > } > > This will increase code complexity and allows creation of multiple NfcService > instances per frame, thus, I would need to make some sort of per frame singleton > that would be wrapped by NfcService. Also mojo already has ServiceRegistry for > top level interfaces, it would look strange to have extra wrapper just to > provide Client instance. > > What do you think? This is the best approach for now. Not sure if Mojo team has some way to make this easier in the future. I agree it feels a little surprising, but it's not really that much more complex, and you get wins in many other areas (i.e. not having to check that m_client is not null or figure out what happens if SetClient is invoked multiple times) +rockot (In any case, this followup change shouldn't block this CL)
The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexander.shalamov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/1765533004/#ps260001 (title: "Rebased to master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1480496327041760,
"parent_rev": "8858475f6c5304c7ac103a7884fa35403e51253a", "commit_rev":
"7002b98a20cc06328d1933c3f0662c5bb146215c"}
Message was sent while issue was closed.
Description was changed from ========== [webnfc] Implement watch method for Android nfc mojo service. Implementation of nfc.watch [1] and nfc.cancelWatch [2] methods for Android nfc mojo service that is used from W3C WebNFC module. [1] https://w3c.github.io/web-nfc/#dom-nfc-watch [2] https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch BUG=520391 BUG=625589 ========== to ========== [webnfc] Implement watch method for Android nfc mojo service. Implementation of nfc.watch [1] and nfc.cancelWatch [2] methods for Android nfc mojo service that is used from W3C WebNFC module. [1] https://w3c.github.io/web-nfc/#dom-nfc-watch [2] https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch BUG=520391 BUG=625589 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [webnfc] Implement watch method for Android nfc mojo service. Implementation of nfc.watch [1] and nfc.cancelWatch [2] methods for Android nfc mojo service that is used from W3C WebNFC module. [1] https://w3c.github.io/web-nfc/#dom-nfc-watch [2] https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch BUG=520391 BUG=625589 ========== to ========== [webnfc] Implement watch method for Android nfc mojo service. Implementation of nfc.watch [1] and nfc.cancelWatch [2] methods for Android nfc mojo service that is used from W3C WebNFC module. [1] https://w3c.github.io/web-nfc/#dom-nfc-watch [2] https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch BUG=520391 BUG=625589 Committed: https://crrev.com/55ac6a4ca3780a5c315a7014f43642e43c5245f0 Cr-Commit-Position: refs/heads/master@{#435189} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/55ac6a4ca3780a5c315a7014f43642e43c5245f0 Cr-Commit-Position: refs/heads/master@{#435189} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
