Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1759373003:
[webnfc] Implement nfc.watch in blink nfc module. (Closed)
Description[webnfc] Implement nfc.watch in blink nfc module.
Implementation for nfc.watch and nfc.cancelWatch methods in blink nfc module.
https://w3c.github.io/web-nfc/#dom-nfc-watch
https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch
LayoutTests/nfc/watch.html is added to validate input parameters
and conversion between WebNFC and corresponding mojo data structures.
BUG=520391
Committed: https://crrev.com/177f40f726442a7c2b489c3df87933baa5338e0b
Cr-Commit-Position: refs/heads/master@{#434468}
Patch Set 1 #Patch Set 2 : Rebase to master, add tests for new functionality. #Patch Set 3 : Rebased. #
Total comments: 24
Patch Set 4 : Fixes for Kenneth's comments. #Patch Set 5 : Use createBaseCallback instead of custom callback. #Patch Set 6 : Use WeakPersistentThisPointer only for callbacks. #Patch Set 7 : Rebased to master and improved tests #
Total comments: 12
Patch Set 8 : Fixes for comments from dcheng. #Patch Set 9 : Rebased to master #
Total comments: 8
Patch Set 10 : Move max message size check to https://crrev.com/2524743002 #Patch Set 11 : Use ScriptState provided in Nfc::watch #
Total comments: 10
Patch Set 12 : Fixes for comments from haraken #
Messages
Total messages: 48 (28 generated)
Description was changed from ========== [webnfc] Implement nfc.watch in blink module and platform proxy. Implementation for nfc.watch [1] and nfc.cancelWatch [2] methods in blink module and nfc service proxy. [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 in blink module. Implementation for nfc.watch and nfc.cancelWatch methods in blink nfc module. https://w3c.github.io/web-nfc/#dom-nfc-watch https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch LayoutTests/nfc/watch.html is added to validate input parameters and conversion between WebNFC and corresponding mojo data structures. BUG=520391 ==========
Description was changed from
==========
[webnfc] Implement watch in blink module.
Implementation for nfc.watch and nfc.cancelWatch methods in blink nfc module.
https://w3c.github.io/web-nfc/#dom-nfc-watch
https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch
LayoutTests/nfc/watch.html is added to validate input parameters
and conversion between WebNFC and corresponding mojo data structures.
BUG=520391
==========
to
==========
[webnfc] Implement nfc.watch in blink nfc module.
Implementation for nfc.watch and nfc.cancelWatch methods in blink nfc module.
https://w3c.github.io/web-nfc/#dom-nfc-watch
https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch
LayoutTests/nfc/watch.html is added to validate input parameters
and conversion between WebNFC and corresponding mojo data structures.
BUG=520391
==========
alexander.shalamov@intel.com changed reviewers: + kenneth.christiansen@gmail.com
Please take a look.
https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js (right): https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:41: return { url: url, recordType: recordType, mediaType: mediaType, mode: mode}; Actually if you use the same name this is not needed in ES2015: var a = "a" var b = "b" var c = { a, b } c == Object {a: "a", b: "b"}
https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js (right): https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:98: return nfc.NFCWatchMode.ANY; You can add a default here instead of the return below https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:109: nfcMessage.data = new Array; wouldn't = [] do? https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:110: for(let i in message.data) I would add a space after 'for' Don't you mean 'of' instead of 'in' (which may iterate properties) https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:265: this.watchers_ = new Array; = [] ? https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:318: let index = this.watchers_.findIndex(value => { return value.id === id;}); you can remove the "{ return " and the "}" part :) without the { } the return is implicit https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:319: if (index === -1) we usually add {} even for single lines in JS https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:362: return this.watchers_[this.watchers_.length - 1].options; what if there are no watchers? https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:389: if (this.watchers_.length > 0) this definitely needs {} https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/nfc/watch.html (right): https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/watch.html:13: }, 'nfc.watch should succeed if NFC HW is enabled.') hardware https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/watch.html:18: navigator.nfc.watch(message => {}), why not define function noop() {} and use noop everywhere https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/watch.html:81: return new Promise((resolve, reject) => { why do you need a new promise if it already returns a promise
https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js (right): https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:41: return { url: url, recordType: recordType, mediaType: mediaType, mode: mode}; On 2016/04/25 08:22:48, kenneth.christiansen wrote: > Actually if you use the same name this is not needed in ES2015: > > var a = "a" > var b = "b" > > var c = { a, b } > > c == Object {a: "a", b: "b"} Done. https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:98: return nfc.NFCWatchMode.ANY; On 2016/04/25 08:31:47, kenneth.christiansen wrote: > You can add a default here instead of the return below Done. https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:109: nfcMessage.data = new Array; On 2016/04/25 08:31:47, kenneth.christiansen wrote: > wouldn't = [] do? Done. https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:110: for(let i in message.data) On 2016/04/25 08:31:46, kenneth.christiansen wrote: > I would add a space after 'for' > > Don't you mean 'of' instead of 'in' (which may iterate properties) Done. https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:265: this.watchers_ = new Array; On 2016/04/25 08:31:46, kenneth.christiansen wrote: > = [] ? Done. https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:318: let index = this.watchers_.findIndex(value => { return value.id === id;}); On 2016/04/25 08:31:47, kenneth.christiansen wrote: > you can remove the "{ return " and the "}" part :) without the { } the return is > implicit Done. https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:319: if (index === -1) On 2016/04/25 08:31:47, kenneth.christiansen wrote: > we usually add {} even for single lines in JS Done. https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:362: return this.watchers_[this.watchers_.length - 1].options; On 2016/04/25 08:31:46, kenneth.christiansen wrote: > what if there are no watchers? Done. https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:389: if (this.watchers_.length > 0) On 2016/04/25 08:31:47, kenneth.christiansen wrote: > this definitely needs {} Done. https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/nfc/watch.html (right): https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/watch.html:13: }, 'nfc.watch should succeed if NFC HW is enabled.') On 2016/04/25 08:31:47, kenneth.christiansen wrote: > hardware Done. https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/watch.html:18: navigator.nfc.watch(message => {}), On 2016/04/25 08:31:47, kenneth.christiansen wrote: > why not define > > function noop() {} and use noop everywhere Done. https://codereview.chromium.org/1759373003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/nfc/watch.html:81: return new Promise((resolve, reject) => { On 2016/04/25 08:31:47, kenneth.christiansen wrote: > why do you need a new promise if it already returns a promise New promise is a wrapper for successful callback invocation. I need to return pending promise that is resolved only when callback is called and passed and received messages are equal, or reject when nfc.watch fails.
kenneth.r.christiansen@intel.com changed reviewers: + kenneth.r.christiansen@intel.com
lgtm
alexander.shalamov@intel.com changed reviewers: + reillyg@chromium.org
alexander.shalamov@intel.com changed reviewers: + dcheng@chromium.org
dcheng, could you please take a look at device/nfc/nfc.mojom I've added constant that limits max size of the NFC message that can be sent over IPC to 32KB and added check on blink side. I will add similar check when NFC message is read from the tag (after I rebase https://crrev.com/1765533004/)
https://codereview.chromium.org/1759373003/diff/220001/device/nfc/nfc.mojom File device/nfc/nfc.mojom (right): https://codereview.chromium.org/1759373003/diff/220001/device/nfc/nfc.mojom#n... device/nfc/nfc.mojom:70: const uint32 kMaxNFCMessageSize = 32768; Nit: omit NFCMessage here, since the constant is embedded in a struct called NFCMessage. https://codereview.chromium.org/1759373003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/nfc/NFC.cpp (right): https://codereview.chromium.org/1759373003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:353: return watchOptionsPtr; Please use a typemap instead (see https://www.chromium.org/developers/design-documents/mojo/type-mapping) https://codereview.chromium.org/1759373003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:500: v8::Local<v8::Value> toV8(const nfc::NFCRecordPtr& record, ScriptState* scriptState) Why do we need to do this manually? I would expect the mojo type to be typemapped to something in Blink, and then whatever magic we use for Blink types to JS Just Works. https://codereview.chromium.org/1759373003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:619: if (getNFCMessageSize(message) > nfc::NFCMessage::kMaxNFCMessageSize) Do we have an equivalent check on the handling side in the browser? I'd rather just handle this uniformly in one place. https://codereview.chromium.org/1759373003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:734: ScriptState* scriptState = ScriptState::forMainWorld(toLocalFrame(page()->mainFrame())); Why does this always use the main frame? Can this API be used in subframes? Why isn't this just using the frame associated with the execution context it's observing? https://codereview.chromium.org/1759373003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/nfc/NFC.h (right): https://codereview.chromium.org/1759373003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.h:70: ScriptPromise RejectIfNotSupported(ScriptState*); Nit: blink naming convention is still camelCaseWithFirstLetterLowercase. Let's be consistent with that for the things that aren't implementing mojo methods.
alexander.shalamov@intel.com changed reviewers: - reillyg@chromium.org
https://codereview.chromium.org/1759373003/diff/220001/device/nfc/nfc.mojom File device/nfc/nfc.mojom (right): https://codereview.chromium.org/1759373003/diff/220001/device/nfc/nfc.mojom#n... device/nfc/nfc.mojom:70: const uint32 kMaxNFCMessageSize = 32768; On 2016/09/08 04:42:38, dcheng wrote: > Nit: omit NFCMessage here, since the constant is embedded in a struct called > NFCMessage. Done. https://codereview.chromium.org/1759373003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/nfc/NFC.cpp (right): https://codereview.chromium.org/1759373003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:353: return watchOptionsPtr; On 2016/09/08 04:42:38, dcheng wrote: > Please use a typemap instead (see > https://www.chromium.org/developers/design-documents/mojo/type-mapping) I don't think blink generated bindings <=> mojo types is supported by typemap system. All other modules use mojo::TypeConverter or just custom functions. https://codereview.chromium.org/1759373003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:500: v8::Local<v8::Value> toV8(const nfc::NFCRecordPtr& record, ScriptState* scriptState) On 2016/09/08 04:42:38, dcheng wrote: > Why do we need to do this manually? I would expect the mojo type to be > typemapped to something in Blink, and then whatever magic we use for Blink types > to JS Just Works. NFCRecord is a kind of container Type,Mime,Data. V8 doesn't know how to do conversion based on those fields. For magic, I would need to copy toV8 method to V8BindingForModules.h / cpp and this will introduce dependency to mojo and would not add any real value. https://codereview.chromium.org/1759373003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:619: if (getNFCMessageSize(message) > nfc::NFCMessage::kMaxNFCMessageSize) On 2016/09/08 04:42:38, dcheng wrote: > Do we have an equivalent check on the handling side in the browser? I'd rather > just handle this uniformly in one place. We need two checks, one for push (write), one for watch (read), when device is waiting for tag in proximity and finds NFC message that is too big, then it is discarded. https://codereview.chromium.org/1759373003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:734: ScriptState* scriptState = ScriptState::forMainWorld(toLocalFrame(page()->mainFrame())); On 2016/09/08 04:42:38, dcheng wrote: > Why does this always use the main frame? Can this API be used in subframes? Why > isn't this just using the frame associated with the execution context it's > observing? I added comment, according to the specification, api only accessible for secure context (https) and only for top browsing context (main frame) https://codereview.chromium.org/1759373003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/nfc/NFC.h (right): https://codereview.chromium.org/1759373003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.h:70: ScriptPromise RejectIfNotSupported(ScriptState*); On 2016/09/08 04:42:38, dcheng wrote: > Nit: blink naming convention is still camelCaseWithFirstLetterLowercase. Let's > be consistent with that for the things that aren't implementing mojo methods. Done.
Patchset #11 (id:200001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #7 (id:180001) 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: This issue passed the CQ dry run.
alexander.shalamov@intel.com changed reviewers: + tsepez@chromium.org
Tom, could you please check mojom modifications. @haraken, could you take a look at modules/nfc changes.
alexander.shalamov@intel.com changed reviewers: + haraken@chromium.org
Otherwise looks good. https://codereview.chromium.org/1759373003/diff/260001/device/nfc/nfc.mojom File device/nfc/nfc.mojom (right): https://codereview.chromium.org/1759373003/diff/260001/device/nfc/nfc.mojom#n... device/nfc/nfc.mojom:71: const uint32 kMaxSize = 32768; Where do we check this (on the receiving side)?
This CL is doing multiple CLs in one go -- would it be possible to split the CL into two or three pieces? https://codereview.chromium.org/1759373003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/nfc/NFC.cpp (right): https://codereview.chromium.org/1759373003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:536: nfcRecord.setData(ScriptValue(scriptState, toV8(record, scriptState))); Would you help me understand why you need to pass in a V8 value to Mojo? https://codereview.chromium.org/1759373003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:756: ScriptState::forMainWorld(toLocalFrame(page()->mainFrame())); What happens if OnWatch is called on an isolated world? Instead of retrieving a ScriptState from page()->mainFrame(), can we explicitly pass in the ScriptState to OnWatch?
Moved max message size check to https://codereview.chromium.org/2524743002/ https://codereview.chromium.org/1759373003/diff/260001/device/nfc/nfc.mojom File device/nfc/nfc.mojom (right): https://codereview.chromium.org/1759373003/diff/260001/device/nfc/nfc.mojom#n... device/nfc/nfc.mojom:71: const uint32 kMaxSize = 32768; On 2016/11/21 18:12:34, Tom Sepez wrote: > Where do we check this (on the receiving side)? It is in another CL: https://codereview.chromium.org/1765533004 device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java 496 if (message.getByteArrayLength() > NfcMessage.MAX_SIZE) Based on feedback from @haraken I'm splitting this CL. The mojom modifications are in new CL: https://codereview.chromium.org/2524743002 https://codereview.chromium.org/1759373003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/nfc/NFC.cpp (right): https://codereview.chromium.org/1759373003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:536: nfcRecord.setData(ScriptValue(scriptState, toV8(record, scriptState))); On 2016/11/22 02:38:14, haraken wrote: > > Would you help me understand why you need to pass in a V8 value to Mojo? This is a helper function that converts from mojom NFCRecordPtr to V8 NFCRecord dictionary https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/nfc/NF... https://codereview.chromium.org/1759373003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:756: ScriptState::forMainWorld(toLocalFrame(page()->mainFrame())); On 2016/11/22 02:38:14, haraken wrote: > > What happens if OnWatch is called on an isolated world? > > Instead of retrieving a ScriptState from page()->mainFrame(), can we explicitly > pass in the ScriptState to OnWatch? Unfortunately OnWatch() is a part of mojo client interface, so, it is not invoked from JS script context, thus, I can't pass ScriptState there. To create V8 objects that are passed to JS side through callback, I need script state, is there a better way to get it? According to spec navigator.nfc object should be bound to main frame of execution context, that is why I'm getting it from main frame.
https://codereview.chromium.org/1759373003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/nfc/NFC.cpp (right): https://codereview.chromium.org/1759373003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:756: ScriptState::forMainWorld(toLocalFrame(page()->mainFrame())); On 2016/11/22 12:50:41, shalamov wrote: > On 2016/11/22 02:38:14, haraken wrote: > > > > What happens if OnWatch is called on an isolated world? > > > > Instead of retrieving a ScriptState from page()->mainFrame(), can we > explicitly > > pass in the ScriptState to OnWatch? > > Unfortunately OnWatch() is a part of mojo client interface, so, it is not > invoked from JS script context, thus, I can't pass ScriptState there. > > To create V8 objects that are passed to JS side through callback, I need script > state, is there a better way to get it? > > According to spec navigator.nfc object should be bound to main frame of > execution context, that is why I'm getting it from main frame. If an extension uses the nfc object, we need to use a ScriptState for an isolated world of the extension. In any case, it's weird to assume some ScriptState here. I think that the ScriptState we should use here is the one that is passed in NFC::watch. Can we store the ScriptState on the MessageCallback? The code would look like: for (const auto& id : ids) { auto it = m_callbacks.find(id); if (it != m_callbacks.end()) { MessageCallback* callback = it->value; callback->handleMessage(toNFSMessage(message, callback->scriptState)); } }
Nothing left for me to review in this CL, I'll chime in on the others. Removing myself as reviewer. Thanks.
alexander.shalamov@intel.com changed reviewers: - tsepez@chromium.org
https://codereview.chromium.org/1759373003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/nfc/NFC.cpp (right): https://codereview.chromium.org/1759373003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:756: ScriptState::forMainWorld(toLocalFrame(page()->mainFrame())); On 2016/11/22 14:39:09, haraken wrote: > On 2016/11/22 12:50:41, shalamov wrote: > > On 2016/11/22 02:38:14, haraken wrote: > > > > > > What happens if OnWatch is called on an isolated world? > > > > > > Instead of retrieving a ScriptState from page()->mainFrame(), can we > > explicitly > > > pass in the ScriptState to OnWatch? > > > > Unfortunately OnWatch() is a part of mojo client interface, so, it is not > > invoked from JS script context, thus, I can't pass ScriptState there. > > > > To create V8 objects that are passed to JS side through callback, I need > script > > state, is there a better way to get it? > > > > According to spec navigator.nfc object should be bound to main frame of > > execution context, that is why I'm getting it from main frame. > > If an extension uses the nfc object, we need to use a ScriptState for an > isolated world of the extension. In any case, it's weird to assume some > ScriptState here. > > I think that the ScriptState we should use here is the one that is passed in > NFC::watch. Can we store the ScriptState on the MessageCallback? > > The code would look like: > > for (const auto& id : ids) { > auto it = m_callbacks.find(id); > if (it != m_callbacks.end()) { > MessageCallback* callback = it->value; > callback->handleMessage(toNFSMessage(message, callback->scriptState)); > } > } Done.
LGTM on my side. https://codereview.chromium.org/1759373003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/nfc/NFC.cpp (right): https://codereview.chromium.org/1759373003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:490: ScriptState* scriptState) { Move ScriptState to the first parameter. https://codereview.chromium.org/1759373003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:506: .ToLocalChecked(); Is it guaranteed that the stringData has a valid JSON? Otherwise, v8::JSON::Parse can throw. https://codereview.chromium.org/1759373003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:532: ScriptState* scriptState) { Ditto. https://codereview.chromium.org/1759373003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:541: ScriptState* scriptState) { Ditto. https://codereview.chromium.org/1759373003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:761: if (!scriptState || !scriptState->contextIsValid()) DCHECK(!scriptState). scriptState should not be null here.
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...
https://codereview.chromium.org/1759373003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/nfc/NFC.cpp (right): https://codereview.chromium.org/1759373003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:490: ScriptState* scriptState) { On 2016/11/23 13:35:15, haraken wrote: > > Move ScriptState to the first parameter. Done. https://codereview.chromium.org/1759373003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:506: .ToLocalChecked(); On 2016/11/23 13:35:15, haraken wrote: > > Is it guaranteed that the stringData has a valid JSON? Otherwise, > v8::JSON::Parse can throw. Done. https://codereview.chromium.org/1759373003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:532: ScriptState* scriptState) { On 2016/11/23 13:35:15, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/1759373003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:541: ScriptState* scriptState) { On 2016/11/23 13:35:15, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/1759373003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/nfc/NFC.cpp:761: if (!scriptState || !scriptState->contextIsValid()) On 2016/11/23 13:35:15, haraken wrote: > > DCHECK(!scriptState). scriptState should not be null here. > Done.
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 kenneth.r.christiansen@intel.com, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1759373003/#ps320001 (title: "Fixes for comments from haraken")
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": 320001, "attempt_start_ts": 1480058504393940,
"parent_rev": "84588c7340644b57b2eb05036b44aad9e1357580", "commit_rev":
"23ff6c6dc8d8ed8f4f488cc9d291fc51b92398c3"}
Message was sent while issue was closed.
Description was changed from
==========
[webnfc] Implement nfc.watch in blink nfc module.
Implementation for nfc.watch and nfc.cancelWatch methods in blink nfc module.
https://w3c.github.io/web-nfc/#dom-nfc-watch
https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch
LayoutTests/nfc/watch.html is added to validate input parameters
and conversion between WebNFC and corresponding mojo data structures.
BUG=520391
==========
to
==========
[webnfc] Implement nfc.watch in blink nfc module.
Implementation for nfc.watch and nfc.cancelWatch methods in blink nfc module.
https://w3c.github.io/web-nfc/#dom-nfc-watch
https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch
LayoutTests/nfc/watch.html is added to validate input parameters
and conversion between WebNFC and corresponding mojo data structures.
BUG=520391
==========
Message was sent while issue was closed.
Committed patchset #12 (id:320001)
Message was sent while issue was closed.
Description was changed from
==========
[webnfc] Implement nfc.watch in blink nfc module.
Implementation for nfc.watch and nfc.cancelWatch methods in blink nfc module.
https://w3c.github.io/web-nfc/#dom-nfc-watch
https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch
LayoutTests/nfc/watch.html is added to validate input parameters
and conversion between WebNFC and corresponding mojo data structures.
BUG=520391
==========
to
==========
[webnfc] Implement nfc.watch in blink nfc module.
Implementation for nfc.watch and nfc.cancelWatch methods in blink nfc module.
https://w3c.github.io/web-nfc/#dom-nfc-watch
https://w3c.github.io/web-nfc/#dom-nfc-cancelwatch
LayoutTests/nfc/watch.html is added to validate input parameters
and conversion between WebNFC and corresponding mojo data structures.
BUG=520391
Committed: https://crrev.com/177f40f726442a7c2b489c3df87933baa5338e0b
Cr-Commit-Position: refs/heads/master@{#434468}
==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/177f40f726442a7c2b489c3df87933baa5338e0b Cr-Commit-Position: refs/heads/master@{#434468} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
