Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1486043002:
[webnfc] Implement push method for Android nfc mojo service. (Closed)
|
Created:
5 years ago by shalamov Modified:
4 years, 4 months ago Reviewers:
kenneth.christiansen, ncarter (slow), Reilly Grant (use Gerrit), riju_, kenneth.r.christiansen, timvolodine, Ted C, Yusuf, alexmos, Tom Sepez, dcheng CC:
chromium-reviews, darin-cc_chromium.org, jam, tsepez (do not use) Base URL:
https://chromium.googlesource.com/chromium/src.git@step_6_add_mojo_service_CL Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[webnfc] Implement push method for Android nfc mojo service.
Initial implementation of Android NFC mojo service that
is used by blink nfc module. Follow-up patches will add
support for nfc.watch / nfc.cancelWatch methods.
BUG=520391
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/3466b6197a8071043533c7adde2b303438ff565d
Cr-Commit-Position: refs/heads/master@{#413108}
Patch Set 1 #Patch Set 2 : Rebased to master. Removed files that are not required. #Patch Set 3 : Make InvalidMessageException static. #
Total comments: 5
Patch Set 4 : Fixes for Kenneth's comments. #Patch Set 5 : Rebased. #Patch Set 6 : Rebased. #
Total comments: 30
Patch Set 7 : Fixes for review comments. #
Total comments: 23
Patch Set 8 : Rebased. #Patch Set 9 : Fixes for review comments. #
Total comments: 2
Patch Set 10 : Avoid caching Activity. #
Total comments: 2
Patch Set 11 : Introduce WindowAndroidProvider / Observer to track window changes. #Patch Set 12 : Rebased to master. #
Total comments: 19
Patch Set 13 : Fixes for Reilly's comments. #Patch Set 14 : Rebased to master. #
Total comments: 7
Patch Set 15 : Rebased to master. #Patch Set 16 : Added more comments to mojom. #
Total comments: 1
Patch Set 17 : Fix mojom field names according to naming conventions. #Patch Set 18 : Move WindowAndroidChangedObserver to other observers in content layer #Messages
Total messages: 84 (45 generated)
Description was changed from ========== [webnfc] Initial NFC mojo service implementation for Android. This patch adds initial implementation for NFC mojo service. Follow-up patches would add rest of the functionality. BUG=520391 ========== to ========== [webnfc] Initial NFC mojo service implementation for Android. This patch adds initial implementation for NFC mojo service. Follow-up patches would add rest of the functionality. BUG=520391 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from
==========
[webnfc] Initial NFC mojo service implementation for Android.
This patch adds initial implementation for NFC mojo service.
Follow-up patches would add rest of the functionality.
BUG=520391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
to
==========
[webnfc] Implement push method for Android nfc mojo service.
Initial implementation of Android NFC mojo service that
is used by blink nfc module. Follow-up patches will add
support for nfc.watch / nfc.cancelWatch methods.
BUG=520391
==========
Please take a look. Blink side implementation: https://crrev.com/1708543002
lgtm but you want someone actively working on Android to review this https://codereview.chromium.org/1486043002/diff/120001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1486043002/diff/120001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:227: if (mReaderCallbackHandler == null) { wouldnt it be nicer with early returns? https://codereview.chromium.org/1486043002/diff/120001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:302: mWasConnected = true; mDidConnect? https://codereview.chromium.org/1486043002/diff/120001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:350: return "UTF-8"; no warning?
alexander.shalamov@intel.com changed reviewers: + timvolodine@chromium.org
https://codereview.chromium.org/1486043002/diff/120001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1486043002/diff/120001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:227: if (mReaderCallbackHandler == null) { On 2016/04/18 16:00:30, kenneth.r.christiansen wrote: > wouldnt it be nicer with early returns? Done. https://codereview.chromium.org/1486043002/diff/120001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:350: return "UTF-8"; On 2016/04/18 16:00:30, kenneth.r.christiansen wrote: > no warning? Done.
tsepez@chromium.org changed reviewers: - tsepez@chromium.org
tsepez@chromium.org changed reviewers: + tsepez@chromium.org
Looks good, deferring to android experts.
Description was changed from
==========
[webnfc] Implement push method for Android nfc mojo service.
Initial implementation of Android NFC mojo service that
is used by blink nfc module. Follow-up patches will add
support for nfc.watch / nfc.cancelWatch methods.
BUG=520391
==========
to
==========
[webnfc] Implement push method for Android nfc mojo service.
Initial implementation of Android NFC mojo service that
is used by blink nfc module. Follow-up patches will add
support for nfc.watch / nfc.cancelWatch methods.
BUG=520391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
alexander.shalamov@intel.com changed reviewers: + alexmos@chromium.org, nick@chromium.org, ppi@chromium.org, tedchoc@chromium.org
Added content and android owners. Could you please take a look?
Description was changed from
==========
[webnfc] Implement push method for Android nfc mojo service.
Initial implementation of Android NFC mojo service that
is used by blink nfc module. Follow-up patches will add
support for nfc.watch / nfc.cancelWatch methods.
BUG=520391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
to
==========
[webnfc] Implement push method for Android nfc mojo service.
Initial implementation of Android NFC mojo service that
is used by blink nfc module. Follow-up patches will add
support for nfc.watch / nfc.cancelWatch methods.
BUG=520391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
ppi@chromium.org changed reviewers: - ppi@chromium.org
https://codereview.chromium.org/1486043002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1486043002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1940: #if !defined(OS_ANDROID) Please add a comment: // On android, NFC is implemented as a Java mojo service. https://codereview.chromium.org/1486043002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1942: base::Bind(&device::NFCImpl::Create)); This line doesn't seem to exist at tip of tree; I see that this is a dependent patch, several layers deep. Are you planning on landing https://codereview.chromium.org/1912363002/diff/120001/content/browser/frame_... first? Should I review that one to? https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:66: private NfcTagWriter mTagWriter; There are very few comments in this file overall. For the @Overrides that's probably okay (since the semantics ought to be documented in the mojom). But, javadoc on these members would be helpful, and javadoc on some of the private methods certainly wouldn't hurt. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:86: // Public methods I don't think this is necessary. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:153: // Implementation Class comment for PendingPushOperation? Can be a one-liner.
some minor comments from my scan of the patch, thanks! https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/BUI... File device/nfc/android/BUILD.gn (right): https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/BUI... device/nfc/android/BUILD.gn:1: # Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2015 -> 2016 https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:59: private NfcManager mNfcManager; final? https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:60: private NfcAdapter mNfcAdapter; final? https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:63: private boolean mHasPermission; final? same for mAcitivy above https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:66: private NfcTagWriter mTagWriter; On 2016/04/25 20:03:14, ncarter wrote: > There are very few comments in this file overall. For the @Overrides that's > probably okay (since the semantics ought to be documented in the mojom). But, > javadoc on these members would be helpful, and javadoc on some of the private > methods certainly wouldn't hurt. +1, bit more documentation would be great https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:128: if (!checkIfReady(callback)) return; should this have a TODO? also below for cancelWatch/cancelAllWatches? https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:142: public void suspendNfcOperations() {} is this intentionally empty or is this a also TODO? (same for resume below) https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:347: if (record.mediaType.endsWith(CHARSET_UTF16)) return "UTF-16LE"; why UTF-16LE? CHARSET_UTF16 doesn't specify LE.
https://codereview.chromium.org/1486043002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1486043002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1940: #if !defined(OS_ANDROID) On 2016/04/25 20:03:14, ncarter wrote: > Please add a comment: > > // On android, NFC is implemented as a Java mojo service. Done. https://codereview.chromium.org/1486043002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1942: base::Bind(&device::NFCImpl::Create)); On 2016/04/25 20:03:14, ncarter wrote: > This line doesn't seem to exist at tip of tree; I see that this is a dependent > patch, several layers deep. > > Are you planning on landing > https://codereview.chromium.org/1912363002/diff/120001/content/browser/frame_... > first? Should I review that one to? Yes, I'm planning to land https://crrev.com/1912363002 first. Reilly Grant, helped to review that CL. If you have time, please take a look. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/BUI... File device/nfc/android/BUILD.gn (right): https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/BUI... device/nfc/android/BUILD.gn:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/26 14:58:26, timvolodine wrote: > 2016 Done. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/26 14:58:27, timvolodine wrote: > 2015 -> 2016 Done. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:59: private NfcManager mNfcManager; On 2016/04/26 14:58:27, timvolodine wrote: > final? Done. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:60: private NfcAdapter mNfcAdapter; On 2016/04/26 14:58:27, timvolodine wrote: > final? Done. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:63: private boolean mHasPermission; On 2016/04/26 14:58:26, timvolodine wrote: > final? same for mAcitivy above Done. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:66: private NfcTagWriter mTagWriter; On 2016/04/25 20:03:14, ncarter wrote: > There are very few comments in this file overall. For the @Overrides that's > probably okay (since the semantics ought to be documented in the mojom). But, > javadoc on these members would be helpful, and javadoc on some of the private > methods certainly wouldn't hurt. Done. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:66: private NfcTagWriter mTagWriter; On 2016/04/26 14:58:26, timvolodine wrote: > On 2016/04/25 20:03:14, ncarter wrote: > > There are very few comments in this file overall. For the @Overrides that's > > probably okay (since the semantics ought to be documented in the mojom). But, > > javadoc on these members would be helpful, and javadoc on some of the private > > methods certainly wouldn't hurt. > > +1, bit more documentation would be great Done. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:86: // Public methods On 2016/04/25 20:03:14, ncarter wrote: > I don't think this is necessary. Done. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:128: if (!checkIfReady(callback)) return; On 2016/04/26 14:58:27, timvolodine wrote: > should this have a TODO? also below for cancelWatch/cancelAllWatches? Done. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:142: public void suspendNfcOperations() {} On 2016/04/26 14:58:27, timvolodine wrote: > is this intentionally empty or is this a also TODO? (same for resume below) Done. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:153: // Implementation On 2016/04/25 20:03:14, ncarter wrote: > Class comment for PendingPushOperation? Can be a one-liner. Done. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:347: if (record.mediaType.endsWith(CHARSET_UTF16)) return "UTF-16LE"; On 2016/04/26 14:58:27, timvolodine wrote: > why UTF-16LE? CHARSET_UTF16 doesn't specify LE. When 16bit WTF::String data is converted to bytearray, there is no BOM. According to http://developer.android.com/reference/java/nio/charset/Charset.html UTF-16 with no BOM will be interpreted as UTF-16BE. When I tested writing unicode data, I noticed that strings are incorrect, because bytearray that I received from blink side had LE byte order. I also simplified blink side and sending UTF-8 encoded strings whenever possible. https://codereview.chromium.org/1708543002/#ps160001
lgtm
https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:347: if (record.mediaType.endsWith(CHARSET_UTF16)) return "UTF-16LE"; On 2016/04/26 21:56:53, shalamov wrote: > On 2016/04/26 14:58:27, timvolodine wrote: > > why UTF-16LE? CHARSET_UTF16 doesn't specify LE. > > When 16bit WTF::String data is converted to bytearray, there is no BOM. > According to > http://developer.android.com/reference/java/nio/charset/Charset.html UTF-16 with > no BOM will be interpreted as UTF-16BE. When I tested writing unicode data, I > noticed that strings are incorrect, because bytearray that I received from blink > side had LE byte order. I also simplified blink side and sending UTF-8 encoded > strings whenever possible. https://codereview.chromium.org/1708543002/#ps160001 yes utf-16be usually tends to be the default, maybe this warrants a comment like you explained above.
https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:70: * Context that is pasesed to NFC mojo service. Used to check permissions line wrapping for comments should be 100 in java https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:109: mActivity = ApplicationStatus.getLastTrackedFocusedActivity(); we should really avoid using this call if at all possible. With multi-window, you might not get the activity that is associated with the web contents trying to trigger this. tabs can now be moved across activities as well, so this might become stale quickly if we're not careful. Do we have access to the webcontents from this? Ideally, we'd get the current Activity from WebContents -> ContentViewCore -> WindowAndroid. Also in general, how does this NFC support play with the Chrome app's general NFC/beam support to send urls across to other devices? What is the lifetime of this object? Is it bound to the lifetime of a web contents? a renderer frame? Both of those can span activities BTW. https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:135: // todo(shalamov): Should be implemented when watch() is implemented. nit, TODO should be capitalized https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:162: enableReaderMode(); as far as I can tell, push only works if Android is greater than KK. Is that true? Should we check that in the constructor and treat that like a lack of permissions? ah, I see that is done in checkIfReady. do you want callers to get error callbacks on older versions of android (specifically NOT_SUPPORTED) instead of SECURITY ones for the lack of permissions? https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:254: public void close() {} what close is this associated with? To me, it looks like we should be calling disableReaderMode() in some sort of destruction path https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:278: * otherwise, error object with corresponding NfcErrorType must be provided. align w/ should above https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:287: public NfcPushOptions pushOptions() { add a blank line above this. Also, you could make mNfcMessage public if you wanted to avoid this need. just change the name to nfcMessage if you do (m prefix is for non-public variables) https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:316: || Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) { I would still not create the mNfcManager for KK. You have the mHasPermission check above that will return SECURITY correctly vs NOT_SUPPORTED https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:447: private boolean mWasConnected = false; false is the default, so you can drop this https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:533: for (NfcRecord record : message.data) { is message.data an array primitive? In general, the foreach idiom should not be used for Lists as it will allocate an iterator https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:550: * @param record these are a bit terse...you can probably drop these https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:592: * @param error same...you can drop all of these empty javadoc params
Thank you for checking CL Ted. I’m fixing issues that you and @timvolodine have raised, but I need to get your input for integration with default behavior of Chrome / Android Beam. > https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... > device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:109: mActivity > = ApplicationStatus.getLastTrackedFocusedActivity(); > we should really avoid using this call if at all possible. With multi-window, > you might not get the activity that is associated with the web contents trying > to trigger this. tabs can now be moved across activities as well, so this might > become stale quickly if we're not careful. > > Do we have access to the webcontents from this? Ideally, we'd get the current > Activity from WebContents -> ContentViewCore -> WindowAndroid. NFC mojo service is bound to render frame, I can avoid calling ApplicationStatus.getLastTrackedFocusedActivity() and get Activity from WindowAndroid associated with RenderFrameHost, this would require few modifications to ServiceRegistrarAndroid::RegisterFrameHostServices(). > Also in general, how does this NFC support play with the Chrome app's general > NFC/beam support to send urls across to other devices? > This CL is using NFC adapter in reader mode, which disables NFC P2P (Android Beam), therefore, default behavior will be changed when user grants ‘nfc’ permission to a web page that uses WebNFC functionality and there is a NFC push / watch operation is in progress. Initially I had implementation that keeps default functionality of Chrome browser intact, regardless of whether the webpage is using WebNFC API or not. However, if NFC adapter is in “Android Beam” mode and NFC tag is found, it is delivered to Activity through intent dispatch system and I had to use LocalBroadcastManager on browser side to forward intent to mojo NFC service, which was 'local' BroadcastReceiver. I felt that such change is too intrusive, so I decided to use 'reader mode' that is using callback mechanism. Do you have an opinion about what solution is preferable? > What is the lifetime of this object? Is it bound to the lifetime of a web > contents? a renderer frame? Both of those can span activities BTW. > Lifetime of this object is bound to lifetime of a render frame.
> > Also in general, how does this NFC support play with the Chrome app's general > > NFC/beam support to send urls across to other devices? > > Ted, I rebased one of my earlier CLs that use NFC "foregroundDispatch". Doesn't interfere with default Chrome / Android Beam functionality, but requires more changes. https://codereview.chromium.org/1938393003 - not for review, just to show difference between two solutions.
On 2016/05/02 13:24:33, shalamov wrote: > Thank you for checking CL Ted. > I’m fixing issues that you and @timvolodine have raised, but I need to get your > input for integration with default behavior of Chrome / Android Beam. > > > > https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... > > device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:109: > mActivity > > = ApplicationStatus.getLastTrackedFocusedActivity(); > > we should really avoid using this call if at all possible. With multi-window, > > you might not get the activity that is associated with the web contents trying > > to trigger this. tabs can now be moved across activities as well, so this > might > > become stale quickly if we're not careful. > > > > Do we have access to the webcontents from this? Ideally, we'd get the current > > Activity from WebContents -> ContentViewCore -> WindowAndroid. > > NFC mojo service is bound to render frame, I can avoid calling > ApplicationStatus.getLastTrackedFocusedActivity() and get Activity from > WindowAndroid associated with RenderFrameHost, this would require few > modifications to ServiceRegistrarAndroid::RegisterFrameHostServices(). > > > Also in general, how does this NFC support play with the Chrome app's general > > NFC/beam support to send urls across to other devices? > > > > This CL is using NFC adapter in reader mode, which disables NFC P2P (Android > Beam), therefore, default behavior will be changed when user grants ‘nfc’ > permission to a web page that uses WebNFC functionality and there is a NFC push > / watch operation is in progress. > > Initially I had implementation that keeps default functionality of Chrome > browser intact, regardless of whether the webpage is using WebNFC API or not. > However, if NFC adapter is in “Android Beam” mode and NFC tag is found, it is > delivered to Activity through intent dispatch system and I had to use > LocalBroadcastManager on browser side to forward intent to mojo NFC service, > which was 'local' BroadcastReceiver. I felt that such change is too intrusive, > so I decided to use 'reader mode' that is using callback mechanism. > > Do you have an opinion about what solution is preferable? Sorry for the delay was OOO for the last week! Let's just go with the approach you have here. If a site is needing NFC support, I think it is fair to suggest that the user is aware of the interactions with the basic Android support. If we want to improve it later (using the code from your patch you linked below), then that seems like a decent approach and something we can do incrementally. > > > What is the lifetime of this object? Is it bound to the lifetime of a web > > contents? a renderer frame? Both of those can span activities BTW. > > > > Lifetime of this object is bound to lifetime of a render frame.
Patchset #9 (id:240001) has been deleted
Fixed review issues. Also, I've split implementation into separate files, since next patches (e.g. https://crrev.com/1765533004/) will introduce more functionality and it is difficult to manage everything in single file. https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1486043002/diff/180001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:347: if (record.mediaType.endsWith(CHARSET_UTF16)) return "UTF-16LE"; On 2016/04/28 17:06:34, timvolodine wrote: > On 2016/04/26 21:56:53, shalamov wrote: > > On 2016/04/26 14:58:27, timvolodine wrote: > > > why UTF-16LE? CHARSET_UTF16 doesn't specify LE. > > > > When 16bit WTF::String data is converted to bytearray, there is no BOM. > > According to > > http://developer.android.com/reference/java/nio/charset/Charset.html UTF-16 > with > > no BOM will be interpreted as UTF-16BE. When I tested writing unicode data, I > > noticed that strings are incorrect, because bytearray that I received from > blink > > side had LE byte order. I also simplified blink side and sending UTF-8 encoded > > strings whenever possible. > https://codereview.chromium.org/1708543002/#ps160001 > > yes utf-16be usually tends to be the default, maybe this warrants a comment like > you explained above. Done. https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:70: * Context that is pasesed to NFC mojo service. Used to check permissions On 2016/04/28 17:27:22, Ted C wrote: > line wrapping for comments should be 100 in java Done. https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:109: mActivity = ApplicationStatus.getLastTrackedFocusedActivity(); On 2016/04/28 17:27:22, Ted C wrote: > we should really avoid using this call if at all possible. With multi-window, > you might not get the activity that is associated with the web contents trying > to trigger this. tabs can now be moved across activities as well, so this might > become stale quickly if we're not careful. > > Do we have access to the webcontents from this? Ideally, we'd get the current > Activity from WebContents -> ContentViewCore -> WindowAndroid. > > Also in general, how does this NFC support play with the Chrome app's general > NFC/beam support to send urls across to other devices? > > What is the lifetime of this object? Is it bound to the lifetime of a web > contents? a renderer frame? Both of those can span activities BTW. Done. https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:135: // todo(shalamov): Should be implemented when watch() is implemented. On 2016/04/28 17:27:21, Ted C wrote: > nit, TODO should be capitalized Done. https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:254: public void close() {} On 2016/04/28 17:27:22, Ted C wrote: > what close is this associated with? > > To me, it looks like we should be calling disableReaderMode() in some sort of > destruction path Done. https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:278: * otherwise, error object with corresponding NfcErrorType must be provided. On 2016/04/28 17:27:22, Ted C wrote: > align w/ should above Done. https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:287: public NfcPushOptions pushOptions() { On 2016/04/28 17:27:22, Ted C wrote: > add a blank line above this. > > Also, you could make mNfcMessage public if you wanted to avoid this need. just > change the name to nfcMessage if you do (m prefix is for non-public variables) Done. https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:316: || Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) { On 2016/04/28 17:27:21, Ted C wrote: > I would still not create the mNfcManager for KK. You have the mHasPermission > check above that will return SECURITY correctly vs NOT_SUPPORTED Done. https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:447: private boolean mWasConnected = false; On 2016/04/28 17:27:22, Ted C wrote: > false is the default, so you can drop this Done. https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:533: for (NfcRecord record : message.data) { On 2016/04/28 17:27:21, Ted C wrote: > is message.data an array primitive? In general, the foreach idiom should not be > used for Lists as it will allocate an iterator Done. https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:550: * @param record On 2016/04/28 17:27:21, Ted C wrote: > these are a bit terse...you can probably drop these Done. https://codereview.chromium.org/1486043002/diff/200001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:592: * @param error On 2016/04/28 17:27:22, Ted C wrote: > same...you can drop all of these empty javadoc params Done.
tedchoc@chromium.org changed reviewers: + yusufo@chromium.org
+yusufo for general tab reparenting conversations https://codereview.chromium.org/1486043002/diff/260001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1486043002/diff/260001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:56: private final Activity mActivity; this has the same problem I mentioned before. the activity can change for a webcontents. In particular, you can open a tab in a Chrome Custom Tab, then you can reparent (i.e. open in chrome option) the tab into Chrome. This moves the tab from one activity to another. The main method that causes problem is: ContentViewCore#updateWindowAndroid But.....there are some sad problems since that is a relatively new addition and you are one of the first people to hit this not in the chrome layer/ In Chrome, you can listen to changes on a Tab via: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... So I have a few follow up questions/thoughts: 1.) What can device/ depend on via DEPS? I suspect content/ is potentially too broad? How about ui/? 2.) Ideally, you take a ContentViewCore, which would give you access the WindowAndroid. Then we need to add some observer to be notified when the WindowAndroid changes. If you can't access content but could depend on ui/, then I would think we could add a couple interfaces: interface WindowAndroidProvider { WindowAndroid getWindowAndroid(); void addWindowAndroidChangedObserver(WindowAndroidChangedObserver observer); void removeWindowAndroidChangedObserver(WindowAndroidChangedObserver observer); } interface WindowAndroidChangedObserver { void onWindowAndroidChanged(WindowAndroid oldWindowAndroid, WindowAndroid newWindowAndroid); } Then, this class would be pass the window android provider, you would register as a change observer and keep the activity from the active window android. When the window android changes, you'd unregister from old activity and register on the new one. Again, sorry for putting you through the ringer, but you are the first one to hit this at this level. Let me know if this isn't clear or if the dependencies need to be tweaked and I'll help as much as I can.
https://codereview.chromium.org/1486043002/diff/260001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1486043002/diff/260001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:56: private final Activity mActivity; On 2016/05/12 18:23:20, Ted C wrote: > this has the same problem I mentioned before. the activity can change for a > webcontents. > > In particular, you can open a tab in a Chrome Custom Tab, then you can reparent > (i.e. open in chrome option) the tab into Chrome. This moves the tab from one > activity to another. > > The main method that causes problem is: > ContentViewCore#updateWindowAndroid > > But.....there are some sad problems since that is a relatively new addition and > you are one of the first people to hit this not in the chrome layer/ > > In Chrome, you can listen to changes on a Tab via: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... > > So I have a few follow up questions/thoughts: > > 1.) What can device/ depend on via DEPS? I suspect content/ is potentially too > broad? How about ui/? > > 2.) Ideally, you take a ContentViewCore, which would give you access the > WindowAndroid. Then we need to add some observer to be notified when the > WindowAndroid changes. > > If you can't access content but could depend on ui/, then I would think we could > add a couple interfaces: > > interface WindowAndroidProvider { > WindowAndroid getWindowAndroid(); > > void addWindowAndroidChangedObserver(WindowAndroidChangedObserver observer); > > void removeWindowAndroidChangedObserver(WindowAndroidChangedObserver > observer); > } > > interface WindowAndroidChangedObserver { > void onWindowAndroidChanged(WindowAndroid oldWindowAndroid, WindowAndroid > newWindowAndroid); > } > > Then, this class would be pass the window android provider, you would register > as a change observer and keep the activity from the active window android. When > the window android changes, you'd unregister from old activity and register on > the new one. > > Again, sorry for putting you through the ringer, but you are the first one to > hit this at this level. Let me know if this isn't clear or if the dependencies > need to be tweaked and I'll help as much as I can. Another solution would be to construct the impl class with a ContentViewCore(or WebContents) cached instead of caching the activity. There are 4 uses of mActivity here. Two of them (checkPermission and getSystemService) can be used through a cached application context inside the impl class here. The other two is needed to enable and disable reader mode and those two can use the contentviewcore to get the valid windowandroid and the activity through that (similar to what the factory inside the ServiceRegistrar does)
On 2016/05/12 18:23:21, Ted C wrote:
> The main method that causes problem is:
> ContentViewCore#updateWindowAndroid
Yes, thanks for pointing that out. Now I understand what you were concerned
about.
> 1.) What can device/ depend on via DEPS? I suspect content/ is potentially
too
> broad? How about ui/?
I'm not sure this is acceptable, it will break architecture layering.
> 2.) Ideally, you take a ContentViewCore, which would give you access the
> WindowAndroid. Then we need to add some observer to be notified when the
> WindowAndroid changes.
>
> If you can't access content but could depend on ui/, then I would think we
could
> add a couple interfaces:
>
> interface WindowAndroidProvider {
> WindowAndroid getWindowAndroid();
>
> void addWindowAndroidChangedObserver(WindowAndroidChangedObserver
observer);
>
> void removeWindowAndroidChangedObserver(WindowAndroidChangedObserver
> observer);
> }
>
> interface WindowAndroidChangedObserver {
> void onWindowAndroidChanged(WindowAndroid oldWindowAndroid, WindowAndroid
> newWindowAndroid);
> }
>
> Then, this class would be pass the window android provider, you would register
> as a change observer and keep the activity from the active window android.
When
> the window android changes, you'd unregister from old activity and register on
> the new one.
>
> Again, sorry for putting you through the ringer, but you are the first one to
> hit this at this level. Let me know if this isn't clear or if the
dependencies
> need to be tweaked and I'll help as much as I can.
No worries. I implemented small fix to avoid caching, now I need to fix issue
related to window update.
Can I introduce 'interface WindowAndroidProvider' and implement it by
ContentViewCore?
https://codereview.chromium.org/1486043002/diff/280001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ServiceRegistrar.java (right): https://codereview.chromium.org/1486043002/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ServiceRegistrar.java:71: return new NfcImpl(mContext, new Callable<Activity>() { to avoid the awkward layering, maybe we can put the window provider logic here (letting the new interfaces live in ui/ for now)? Ideally, I'd like to avoid having to put the WindowAndroidProvider in device/ because I feel that will quickly not be sufficient as other non-content packages need it. So for now, maybe we still add the provider and make ContentViewCore implement it, but we then build out the dependency here. Instead of the NfcImpl registering for changes to the window android, the factory would and it would push the changes to the impl. Granted, then the factory would need to track the created Nfc instance, which really doesn't fit well with the factory pattern. I guess we could have a shim class that extends NfcImpl here? ContextAwareNfcImpl or something that registers for the observer in the constructor and removes the observer in close() private static class ContextAwareNfcImpl extends NfcImpl implements OnWindowAndroidChangedObserver { ContextAwareNfcImpl(Context context, ContentViewCore cvc) { super(context); cvc.addOnWindowAndroidChangedObserver(this); setActivity(....); } @Override public void close() { super.close(); mCvc.removeOnWindowAndroidChangedObserver(this); } @Override onWindowAndroidChanged(...) { setActivity(getActivityFromNewWindowAndroid-yMethod); } } Then you expose just setActivity in the base class and you can update it from here. Thoughts? The factory wouldn't need to know about it and it is "somewhat" not too ugly.
alexander.shalamov@intel.com changed reviewers: - armansito@chromium.org
https://codereview.chromium.org/1486043002/diff/280001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ServiceRegistrar.java (right): https://codereview.chromium.org/1486043002/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ServiceRegistrar.java:71: return new NfcImpl(mContext, new Callable<Activity>() { On 2016/05/13 22:04:08, Ted C wrote: > to avoid the awkward layering, maybe we can put the window provider logic here > (letting the new interfaces live in ui/ for now)? > > Ideally, I'd like to avoid having to put the WindowAndroidProvider in device/ > because I feel that will quickly not be sufficient as other non-content packages > need it. So for now, maybe we still add the provider and make ContentViewCore > implement it, but we then build out the dependency here. > > Instead of the NfcImpl registering for changes to the window android, the > factory would and it would push the changes to the impl. Granted, then the > factory would need to track the created Nfc instance, which really doesn't fit > well with the factory pattern. > > I guess we could have a shim class that extends NfcImpl here? > ContextAwareNfcImpl or something that registers for the observer in the > constructor and removes the observer in close() > > private static class ContextAwareNfcImpl extends NfcImpl implements > OnWindowAndroidChangedObserver { > ContextAwareNfcImpl(Context context, ContentViewCore cvc) { > super(context); > cvc.addOnWindowAndroidChangedObserver(this); > setActivity(....); > } > > @Override > public void close() { > super.close(); > mCvc.removeOnWindowAndroidChangedObserver(this); > } > > @Override > onWindowAndroidChanged(...) { > setActivity(getActivityFromNewWindowAndroid-yMethod); > } > } > > Then you expose just setActivity in the base class and you can update it from > here. Thoughts? The factory wouldn't need to know about it and it is > "somewhat" not too ugly. Done.
On 2016/05/17 12:30:33, shalamov wrote: > https://codereview.chromium.org/1486043002/diff/280001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/ServiceRegistrar.java > (right): > > https://codereview.chromium.org/1486043002/diff/280001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/ServiceRegistrar.java:71: > return new NfcImpl(mContext, new Callable<Activity>() { > On 2016/05/13 22:04:08, Ted C wrote: > > to avoid the awkward layering, maybe we can put the window provider logic here > > (letting the new interfaces live in ui/ for now)? > > > > Ideally, I'd like to avoid having to put the WindowAndroidProvider in device/ > > because I feel that will quickly not be sufficient as other non-content > packages > > need it. So for now, maybe we still add the provider and make ContentViewCore > > implement it, but we then build out the dependency here. > > > > Instead of the NfcImpl registering for changes to the window android, the > > factory would and it would push the changes to the impl. Granted, then the > > factory would need to track the created Nfc instance, which really doesn't fit > > well with the factory pattern. > > > > I guess we could have a shim class that extends NfcImpl here? > > ContextAwareNfcImpl or something that registers for the observer in the > > constructor and removes the observer in close() > > > > private static class ContextAwareNfcImpl extends NfcImpl implements > > OnWindowAndroidChangedObserver { > > ContextAwareNfcImpl(Context context, ContentViewCore cvc) { > > super(context); > > cvc.addOnWindowAndroidChangedObserver(this); > > setActivity(....); > > } > > > > @Override > > public void close() { > > super.close(); > > mCvc.removeOnWindowAndroidChangedObserver(this); > > } > > > > @Override > > onWindowAndroidChanged(...) { > > setActivity(getActivityFromNewWindowAndroid-yMethod); > > } > > } > > > > Then you expose just setActivity in the base class and you can update it from > > here. Thoughts? The factory wouldn't need to know about it and it is > > "somewhat" not too ugly. > > Done. lgtm ... awesome. thanks!
alexander.shalamov@intel.com changed reviewers: + reillyg@chromium.org
Reilly, could you please check additions to device/nfc/*
https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:55: * Can be Updated when activity associated with web page is changed. @see #setActivity s/Updated/updated/ https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:89: if (mNfcManager != null) { nit: Inverting this conditional will make the code clearer: if (mNfcManager == null) { Log.w(TAG, "NFC is not supported."); } else { mNfcAdapter = mNfcManager.getDefaultAdapter(); } There's no need to set mNfcAdapter to null because this is the constructor. https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:98: mNfcManager = null; Ditto here, these fields will already be null. https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:121: // TODO(shalamov): Should be implemented when watch() is implemented. Please use TODO(crbug.com/XXXXXX) instead of a username so that it is easier to track ownership and completion of this work. https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:167: if (mPendingPushOperation != null) { nit: Invert this condition for better readability. https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:190: // TODO(shalamov): Not implemented. Same here, please file a bug. https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:202: if (!checkIfReady(callback)) return; Bug. https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:215: // TODO(shalamov): Not implemented. Bug (note, this can all be the same bug). https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:267: if (mPushResponseCallback != null) mPushResponseCallback.call(error); Please break this into two lines here and elsewhere.
https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:55: * Can be Updated when activity associated with web page is changed. @see #setActivity On 2016/07/01 20:51:44, Reilly Grant wrote: > s/Updated/updated/ Done. https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:89: if (mNfcManager != null) { On 2016/07/01 20:51:44, Reilly Grant wrote: > nit: Inverting this conditional will make the code clearer: > > if (mNfcManager == null) { > Log.w(TAG, "NFC is not supported."); > } else { > mNfcAdapter = mNfcManager.getDefaultAdapter(); > } > > There's no need to set mNfcAdapter to null because this is the constructor. I inverted conditional. Blank final class variables must be initialized, otherwise, there would be a compile error. https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:98: mNfcManager = null; On 2016/07/01 20:51:45, Reilly Grant wrote: > Ditto here, these fields will already be null. same as above, blank final class members must be definitely assigned in the constructor. https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:121: // TODO(shalamov): Should be implemented when watch() is implemented. On 2016/07/01 20:51:44, Reilly Grant wrote: > Please use TODO(crbug.com/XXXXXX) instead of a username so that it is easier to > track ownership and completion of this work. Done. https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:167: if (mPendingPushOperation != null) { On 2016/07/01 20:51:44, Reilly Grant wrote: > nit: Invert this condition for better readability. Done. https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:190: // TODO(shalamov): Not implemented. On 2016/07/01 20:51:44, Reilly Grant wrote: > Same here, please file a bug. Done. https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:202: if (!checkIfReady(callback)) return; On 2016/07/01 20:51:44, Reilly Grant wrote: > Bug. Done. https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:215: // TODO(shalamov): Not implemented. On 2016/07/01 20:51:44, Reilly Grant wrote: > Bug (note, this can all be the same bug). Done. https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:267: if (mPushResponseCallback != null) mPushResponseCallback.call(error); On 2016/07/01 20:51:44, Reilly Grant wrote: > Please break this into two lines here and elsewhere. I think it is default java style that is used in Chromium, if I do as you mentioned and then, 'git cl format device/nfc', formatter reverts changes.
lgtm https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java (right): https://codereview.chromium.org/1486043002/diff/320001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java:89: if (mNfcManager != null) { On 2016/07/04 at 10:08:16, shalamov wrote: > On 2016/07/01 20:51:44, Reilly Grant wrote: > > nit: Inverting this conditional will make the code clearer: > > > > if (mNfcManager == null) { > > Log.w(TAG, "NFC is not supported."); > > } else { > > mNfcAdapter = mNfcManager.getDefaultAdapter(); > > } > > > > There's no need to set mNfcAdapter to null because this is the constructor. > > I inverted conditional. Blank final class variables must be initialized, otherwise, there would be a compile error. My apologies. I don't review very much Java code.
The CQ bit was checked by alexander.shalamov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from kenneth.r.christiansen@intel.com, nick@chromium.org, tedchoc@chromium.org, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/1486043002/#ps360001 (title: "Rebased to master.")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
alexander.shalamov@intel.com changed reviewers: + dcheng@chromium.org
dcheng, could you please take a look at device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java
Tom, could you please take a look at: device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java
Dcheng, Tom, could you please check device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java whenever you have time? New presubmit filter was introduced (*typeconverter*) that requires lgtm from security team.
https://codereview.chromium.org/1486043002/diff/360001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java (right): https://codereview.chromium.org/1486043002/diff/360001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:38: if (message == null || message.data.length == 0) throw new InvalidNfcMessageException(); Should this be <= 0? https://codereview.chromium.org/1486043002/diff/360001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:60: if (record.mediaType.endsWith(CHARSET_UTF8)) return "UTF-8"; Can we add some comments to the mojom? What is a mediaType? What can it look like? Is it anything like a MIME type? It's not clear at all from the mojom definition. (It also appears the mojom doesn't follow naming conventions: I would expect things in the mojom to be named_with_underscores, and let the mojom compiler handle the underscore -> camel case conversion)
https://codereview.chromium.org/1486043002/diff/360001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java (right): https://codereview.chromium.org/1486043002/diff/360001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:38: if (message == null || message.data.length == 0) throw new InvalidNfcMessageException(); On 2016/08/16 06:27:20, dcheng wrote: > Should this be <= 0? According to java specification, array length guaranteed to be non-negative. https://codereview.chromium.org/1486043002/diff/360001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:60: if (record.mediaType.endsWith(CHARSET_UTF8)) return "UTF-8"; On 2016/08/16 06:27:20, dcheng wrote: > Can we add some comments to the mojom? What is a mediaType? What can it look > like? Is it anything like a MIME type? It's not clear at all from the mojom > definition. Added comment to mojom. > (It also appears the mojom doesn't follow naming conventions: I would expect > things in the mojom to be named_with_underscores, and let the mojom compiler > handle the underscore -> camel case conversion) Would it be fine if I do it in a separate CL? Otherwise this patch would be too big. I would need to change blink part, mock JS service implementation, etc.
LGTM. Do you have a security reviewer for the overall feature? I wonder what our threat model here is, and if we're concerned about the possibility of pushing arbitrary bits to NFC devices. https://codereview.chromium.org/1486043002/diff/360001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcTagHandler.java (right): https://codereview.chromium.org/1486043002/diff/360001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTagHandler.java:120: * tag to be out of ragne. Nit: range https://codereview.chromium.org/1486043002/diff/360001/device/nfc/android/jav... File device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java (right): https://codereview.chromium.org/1486043002/diff/360001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:38: if (message == null || message.data.length == 0) throw new InvalidNfcMessageException(); On 2016/08/16 12:47:15, shalamov wrote: > On 2016/08/16 06:27:20, dcheng wrote: > > Should this be <= 0? > > According to java specification, array length guaranteed to be non-negative. OK, the Java types aren't clear to me. Is there some doc that covers how mojom map to java? https://codereview.chromium.org/1486043002/diff/360001/device/nfc/android/jav... device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java:60: if (record.mediaType.endsWith(CHARSET_UTF8)) return "UTF-8"; On 2016/08/16 12:47:15, shalamov wrote: > On 2016/08/16 06:27:20, dcheng wrote: > > Can we add some comments to the mojom? What is a mediaType? What can it look > > like? Is it anything like a MIME type? It's not clear at all from the mojom > > definition. > > Added comment to mojom. OK, I think similar comments would help for some other fields as well (for example, are there any length limits on data? Is it ever freeform binary data or is it always a string? etc) > > > (It also appears the mojom doesn't follow naming conventions: I would expect > > things in the mojom to be named_with_underscores, and let the mojom compiler > > handle the underscore -> camel case conversion) > > Would it be fine if I do it in a separate CL? Otherwise this patch would be too > big. I would need to change blink part, mock JS service implementation, etc. Yes, that's fine.
On 2016/08/16 at 20:50:09, dcheng wrote: > LGTM. > > Do you have a security reviewer for the overall feature? I wonder what our threat model here is, and if we're concerned about the possibility of pushing arbitrary bits to NFC devices. The feature did get security review by the Google security team at some point and a W3C TAG review as well. https://github.com/w3c/web-nfc/issues/22
On 2016/08/16 20:50:09, dcheng wrote: > https://codereview.chromium.org/1486043002/diff/360001/device/nfc/android/jav... > device/nfc/android/java/src/org/chromium/device/nfc/NfcTagHandler.java:120: * > tag to be out of ragne. > Nit: range Done. > OK, the Java types aren't clear to me. Is there some doc that covers how mojom > map to java? > I'm not sure whether, there is document for mojom<->Java, but I know that mojom array type is mapped to Java primitive (same for JS). I usually check templates for mappings under ./mojo/public/tools/bindings/generators/java_templates > OK, I think similar comments would help for some other fields as well (for > example, are there any length limits on data? Is it ever freeform binary data or > is it always a string? etc) Added comments for other fields as well. The length of the data is usually constrained by HW, biggest NFC tag I have is 8K, normal tags can store few hundred bytes. Data can be in binary format if NFCRecord is 'OPAQUE_RECORD' (similar to Blob), that data is not interpreted in browser (or renderer) code and received / stored as an array buffer. On JS side client can interpret data using media type.
On 2016/08/17 12:54:07, shalamov wrote: > On 2016/08/16 20:50:09, dcheng wrote: > > > https://codereview.chromium.org/1486043002/diff/360001/device/nfc/android/jav... > > device/nfc/android/java/src/org/chromium/device/nfc/NfcTagHandler.java:120: * > > tag to be out of ragne. > > Nit: range > > Done. > > > OK, the Java types aren't clear to me. Is there some doc that covers how mojom > > map to java? > > > > I'm not sure whether, there is document for mojom<->Java, but I know that mojom > array type is mapped to Java primitive (same for JS). > I usually check templates for mappings under > ./mojo/public/tools/bindings/generators/java_templates > > > OK, I think similar comments would help for some other fields as well (for > > example, are there any length limits on data? Is it ever freeform binary data > or > > is it always a string? etc) > > Added comments for other fields as well. > > The length of the data is usually constrained by HW, biggest NFC tag I have is > 8K, normal tags can store few hundred bytes. Data can be in binary format if > NFCRecord is 'OPAQUE_RECORD' (similar to Blob), that data is not interpreted in > browser (or renderer) code and received / stored as an array buffer. On JS side > client can interpret data using media type. It might be nice to assert that getCharset() is only called on valid NfcRecords. Otherwise, LGTM still and thanks for the info. https://codereview.chromium.org/1486043002/diff/400001/device/nfc/nfc.mojom File device/nfc/nfc.mojom (right): https://codereview.chromium.org/1486043002/diff/400001/device/nfc/nfc.mojom#n... device/nfc/nfc.mojom:78: // until it's completion. Nit: its.
Patchset #17 (id:420001) has been deleted
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: Your CL was about to rely on recently removed CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Description was changed from
==========
[webnfc] Implement push method for Android nfc mojo service.
Initial implementation of Android NFC mojo service that
is used by blink nfc module. Follow-up patches will add
support for nfc.watch / nfc.cancelWatch methods.
BUG=520391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
to
==========
[webnfc] Implement push method for Android nfc mojo service.
Initial implementation of Android NFC mojo service that
is used by blink nfc module. Follow-up patches will add
support for nfc.watch / nfc.cancelWatch methods.
BUG=520391
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Patchset #17 (id:440001) has been deleted
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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
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 nick@chromium.org, reillyg@chromium.org, kenneth.r.christiansen@intel.com, tedchoc@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1486043002/#ps480001 (title: "Move WindowAndroidChangedObserver to other observers in content layer")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
[webnfc] Implement push method for Android nfc mojo service.
Initial implementation of Android NFC mojo service that
is used by blink nfc module. Follow-up patches will add
support for nfc.watch / nfc.cancelWatch methods.
BUG=520391
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
[webnfc] Implement push method for Android nfc mojo service.
Initial implementation of Android NFC mojo service that
is used by blink nfc module. Follow-up patches will add
support for nfc.watch / nfc.cancelWatch methods.
BUG=520391
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
Message was sent while issue was closed.
Committed patchset #18 (id:480001)
Message was sent while issue was closed.
Description was changed from
==========
[webnfc] Implement push method for Android nfc mojo service.
Initial implementation of Android NFC mojo service that
is used by blink nfc module. Follow-up patches will add
support for nfc.watch / nfc.cancelWatch methods.
BUG=520391
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
[webnfc] Implement push method for Android nfc mojo service.
Initial implementation of Android NFC mojo service that
is used by blink nfc module. Follow-up patches will add
support for nfc.watch / nfc.cancelWatch methods.
BUG=520391
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/3466b6197a8071043533c7adde2b303438ff565d
Cr-Commit-Position: refs/heads/master@{#413108}
==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/3466b6197a8071043533c7adde2b303438ff565d Cr-Commit-Position: refs/heads/master@{#413108} |
