[webnfc] Implement push() method in blink nfc module.

[webnfc] Implement push() method in blink nfc module.

This patch implements nfc.push() and nfc.cancelPush() methods in nfc module.

https://w3c.github.io/web-nfc/#dom-nfc-push
https://w3c.github.io/web-nfc/#dom-nfc-cancelpush

Mock mojo service and simple layout test is added to validate input parameters
as well as conversion between WebNFC and corresponding mojo data structures.

BUG=520391
Committed: https://crrev.com/5058e32db5582978f60a28bc0ccbeed0a7421283
Committed: https://crrev.com/5c52ef129e520de01aa36728a4b0529b31e8467b
Cr-Original-Commit-Position: refs/heads/master@{#398538}
Cr-Commit-Position: refs/heads/master@{#399139}

[shalamov, 2016-03-04 09:08:33]

Description was changed from

==========
[webnfc] WIP: Implement push() method in blink nfc module.

Initial implementation of nfc.push method and simple layout test
that checks if invalid NFCMessage objects are rejected.

BUG=520391
==========

to

==========
[webnfc] Implement push() method in blink nfc module.
    
This patch implements nfc.push() method in nfc module and in
plafrorm nfc proxy. Mojo type converters are added for conversion 
between proxy nfc types and mojo NFC structures.
Simple layout test added to check if invalid NFCMessage objects are rejected.
==========

[shalamov, 2016-03-04 09:12:22]

Description was changed from

==========
[webnfc] Implement push() method in blink nfc module.
    
This patch implements nfc.push() method in nfc module and in
plafrorm nfc proxy. Mojo type converters are added for conversion 
between proxy nfc types and mojo NFC structures.
Simple layout test added to check if invalid NFCMessage objects are rejected.
==========

to

==========
[webnfc] Implement push() method in blink nfc module.
    
This patch implements nfc.push() method in nfc module and in
plafrorm nfc proxy. Mojo type converters are added for conversion 
between proxy nfc types and mojo NFC structures.
Simple layout test added to check if invalid NFCMessage objects are rejected.

BUG=520391
==========

[shalamov, 2016-04-14 13:44:36]

Description was changed from

==========
[webnfc] Implement push() method in blink nfc module.
    
This patch implements nfc.push() method in nfc module and in
plafrorm nfc proxy. Mojo type converters are added for conversion 
between proxy nfc types and mojo NFC structures.
Simple layout test added to check if invalid NFCMessage objects are rejected.

BUG=520391
==========

to

==========
[webnfc] Implement push() method in blink nfc module.
    
This patch implements nfc.push() and nfc.cancelPush() methods in nfc module.

https://w3c.github.io/web-nfc/#dom-nfc-push
https://w3c.github.io/web-nfc/#dom-nfc-cancelpush
    
Mock mojo service and simple layout test is added to validate input parameters
as well as conversion between WebNFC and corresponding mojo data structures.
    
BUG=520391
==========

[shalamov, 2016-04-14 13:46:58]

Patchset #2 (id:20001) has been deleted

[shalamov, 2016-04-14 13:47:12]

Patchset #1 (id:1) has been deleted

[kenneth.christiansen, 2016-04-14 14:29:26]

kenneth.christiansen@gmail.com changed reviewers:
+ kenneth.christiansen@gmail.com

[kenneth.christiansen, 2016-04-14 14:29:27]

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/nfc/push.html (right):

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/nfc/push.html:11: var invalid_messages =
Guess we could use const/let here

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/nfc/push.html:65:
invalid_messages.forEach((message) => {
no need to put () around message

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js (right):

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:19: buffer[i] =
string.charCodeAt(i);
can't you use TextEncoder instead?

new TextEncoder().encode("hello").buffer()

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:67: if (type ===
'text')
switch() on type

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:99: byteArray = new
Uint8Array(tmpData.length);
TextEncoder seems better here :)

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFC.cpp:303: pushOptionsPtr->ignoreRead =
true;
link to spec saying the default is true?

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFC.cpp:550: if (m_nfc)
do you intent to initialize this multiple times? or LOG?

[shalamov, 2016-04-15 07:56:40]

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/nfc/push.html (right):

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/nfc/push.html:11: var invalid_messages =
On 2016/04/14 14:29:27, kenneth.christiansen wrote:
> Guess we could use const/let here

Done.

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/nfc/push.html:65:
invalid_messages.forEach((message) => {
On 2016/04/14 14:29:27, kenneth.christiansen wrote:
> no need to put () around message

Done.

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js (right):

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:19: buffer[i] =
string.charCodeAt(i);
On 2016/04/14 14:29:27, kenneth.christiansen wrote:
> can't you use TextEncoder instead?
> 
> new TextEncoder().encode("hello").buffer()

Done.

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:67: if (type ===
'text')
On 2016/04/14 14:29:27, kenneth.christiansen wrote:
> switch() on type

Done.

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:99: byteArray = new
Uint8Array(tmpData.length);
On 2016/04/14 14:29:27, kenneth.christiansen wrote:
> TextEncoder seems better here :)

Done.

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFC.cpp:303: pushOptionsPtr->ignoreRead =
true;
On 2016/04/14 14:29:27, kenneth.christiansen wrote:
> link to spec saying the default is true?

Done.

https://codereview.chromium.org/1708543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFC.cpp:550: if (m_nfc)
On 2016/04/14 14:29:27, kenneth.christiansen wrote:
> do you intent to initialize this multiple times? or LOG?

I want to lazy initialize mojo service only when one of navigator.nfc.*
functions is called. 

if(m_nfc) checks whether mojo proxy is bound.

[Mikhail, 2016-04-15 11:20:33]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-15 11:20:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1708543002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1708543002/80001

[commit-bot: I haz the power, 2016-04-15 12:46:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-15 12:46:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shalamov, 2016-04-18 04:55:25]

alexander.shalamov@intel.com changed reviewers:
+ rijubrata.bhaumik@intel.com, tsepez@chromium.org

[shalamov, 2016-04-18 04:55:25]

Please take a look.

Android nfc mojo service implementation for nfc.push / nfc.cancelPush:
https://crrev.com/1486043002/

[Tom Sepez, 2016-04-20 17:25:15]

lgtm

[shalamov, 2016-04-25 14:31:05]

alexander.shalamov@intel.com changed reviewers:
+ mlamouri@chromium.org, reillyg@chromium.org, rockot@chromium.org

[shalamov, 2016-04-25 14:31:06]

Added device/ and modules/ owners to review. Could you please take a look?

[mlamouri (slow - plz ping), 2016-04-25 15:55:30]

What do you want me to review? Everything in modules/ or just some specific
files?

[Reilly Grant (use Gerrit), 2016-04-25 20:12:55]

https://codereview.chromium.org/1708543002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/1708543002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:436: class NFCCallback final :
public NFCMojoCallback::Runnable {
I've added a createBaseCallback() function in MojoHelpers.h which means you
don't need to implement Runnable to create a mojo::Callback from Blink.

See https://codereview.chromium.org/1850023002 for an example.

[shalamov, 2016-04-26 07:54:00]

On 2016/04/25 15:55:30, Mounir Lamouri wrote:
> What do you want me to review? Everything in modules/ or just some specific
> files?

Would be nice if you could check changes for build files:

third_party/WebKit/Source/modules/BUILD.gn
third_party/WebKit/Source/modules/modules.gyp

[shalamov, 2016-04-26 13:17:50]

https://codereview.chromium.org/1708543002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/1708543002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:436: class NFCCallback final :
public NFCMojoCallback::Runnable {
On 2016/04/25 20:12:54, Reilly Grant wrote:
> I've added a createBaseCallback() function in MojoHelpers.h which means you
> don't need to implement Runnable to create a mojo::Callback from Blink.
> 
> See https://codereview.chromium.org/1850023002 for an example.

Done.

[mlamouri (slow - plz ping), 2016-04-26 14:05:40]

modules/*.{gn,gyp,gypi} lgtm

[Reilly Grant (use Gerrit), 2016-04-27 23:32:54]

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:112: return array;
We try to avoid calling memcpy. Instead declare array as a WTF::Vector<uint8_t>,
call array.append(utf8String.data(), utf8String.length()) to copy the data and
then return mojo::WTFArray<uint8_t>(array).

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:122: return array;
Same here.

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:408: if (!message.isNFCMessage()
&& !message.isString() && !message.isArrayBuffer())
I recommend supporting ArrayBufferView in addition to ArrayBuffer. This combined
type is known as a BufferSource:
https://heycam.github.io/webidl/#common-BufferSource

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:543:
m_nfc.set_connection_error_handler([this]() {
Instead of a C++ lambda use WTF::bind to bind to a WeakPersistentThisPointer.
Using C++ lambdas turned out to not interact well with Oilpan:
https://codereview.chromium.org/1912223002

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFC.h (right):

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.h:8: #include
"bindings/core/v8/CallbackPromiseAdapter.h"
Unused header.

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.h:69:
mojo::Binding<device::wtf::NFCClient> m_client;
When building the USB Mojo interface rockot@ recommended against creating a
client interface like this because it introduces a vulnerability: the render
process can cause the message queue in the browser process to back up by
refusing to read messages. A "get next event" interface like I have in
device::usb::DeviceManager::GetDeviceChanges was recommended instead.

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFCError.h (right):

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFCError.h:9: #include
"platform/heap/Handle.h"
Unused header.

[shalamov, 2016-04-28 14:16:14]

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:112: return array;
On 2016/04/27 23:32:53, Reilly Grant wrote:
> We try to avoid calling memcpy. Instead declare array as a
WTF::Vector<uint8_t>,
> call array.append(utf8String.data(), utf8String.length()) to copy the data and
> then return mojo::WTFArray<uint8_t>(array).

Done.

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:122: return array;
On 2016/04/27 23:32:53, Reilly Grant wrote:
> Same here.

Done.

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:408: if (!message.isNFCMessage()
&& !message.isString() && !message.isArrayBuffer())
On 2016/04/27 23:32:53, Reilly Grant wrote:
> I recommend supporting ArrayBufferView in addition to ArrayBuffer. This
combined
> type is known as a BufferSource:
> https://heycam.github.io/webidl/#common-BufferSource

I will talk to Kenneth and Zoltan who are spec authors and create issue for the
specification.

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:543:
m_nfc.set_connection_error_handler([this]() {
On 2016/04/27 23:32:53, Reilly Grant wrote:
> Instead of a C++ lambda use WTF::bind to bind to a WeakPersistentThisPointer.
> Using C++ lambdas turned out to not interact well with Oilpan:
> https://codereview.chromium.org/1912223002

Done.

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFC.h (right):

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.h:8: #include
"bindings/core/v8/CallbackPromiseAdapter.h"
On 2016/04/27 23:32:53, Reilly Grant wrote:
> Unused header.

Done.

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.h:69:
mojo::Binding<device::wtf::NFCClient> m_client;
On 2016/04/27 23:32:53, Reilly Grant wrote:
> When building the USB Mojo interface rockot@ recommended against creating a
> client interface like this because it introduces a vulnerability: the render
> process can cause the message queue in the browser process to back up by
> refusing to read messages. A "get next event" interface like I have in
> device::usb::DeviceManager::GetDeviceChanges was recommended instead.

Done.

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFCError.h (right):

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFCError.h:9: #include
"platform/heap/Handle.h"
On 2016/04/27 23:32:53, Reilly Grant wrote:
> Unused header.

Done.

[Reilly Grant (use Gerrit), 2016-04-28 18:05:02]

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFC.h (right):

https://codereview.chromium.org/1708543002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.h:69:
mojo::Binding<device::wtf::NFCClient> m_client;
On 2016/04/28 at 14:16:14, shalamov wrote:
> On 2016/04/27 23:32:53, Reilly Grant wrote:
> > When building the USB Mojo interface rockot@ recommended against creating a
> > client interface like this because it introduces a vulnerability: the render
> > process can cause the message queue in the browser process to back up by
> > refusing to read messages. A "get next event" interface like I have in
> > device::usb::DeviceManager::GetDeviceChanges was recommended instead.
> 
> Done.

After checking again with rockot@ I was told that updates to the Mojo EDK make
this style of interface no longer an issue. A client interface for callbacks
such as this is now recommended. Sorry for the confusion.

[Reilly Grant (use Gerrit), 2016-04-28 18:09:26]

https://codereview.chromium.org/1708543002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/1708543002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:477: auto callback =
createBaseCallback(bind<device::wtf::NFCErrorPtr>(&NFC::OnRequestCompleted,
WeakPersistentThisPointer<NFC>(this), resolver));
You do not want to use WeakPersistentThisPointer here. The promise may outlive
references to the NFC object held by scripts and should be still be resolved as
long as the execution context is active.

https://codereview.chromium.org/1708543002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:495: auto callback =
createBaseCallback(bind<device::wtf::NFCErrorPtr>(&NFC::OnRequestCompleted,
WeakPersistentThisPointer<NFC>(this), resolver));
Same here.

[shalamov, 2016-04-29 10:00:21]

https://codereview.chromium.org/1708543002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/1708543002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:477: auto callback =
createBaseCallback(bind<device::wtf::NFCErrorPtr>(&NFC::OnRequestCompleted,
WeakPersistentThisPointer<NFC>(this), resolver));
On 2016/04/28 18:09:26, Reilly Grant wrote:
> You do not want to use WeakPersistentThisPointer here. The promise may outlive
> references to the NFC object held by scripts and should be still be resolved
as
> long as the execution context is active.

Done.

https://codereview.chromium.org/1708543002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:495: auto callback =
createBaseCallback(bind<device::wtf::NFCErrorPtr>(&NFC::OnRequestCompleted,
WeakPersistentThisPointer<NFC>(this), resolver));
On 2016/04/28 18:09:26, Reilly Grant wrote:
> Same here.

Done.

[shalamov, 2016-05-10 10:34:31]

Patchset #14 (id:300001) has been deleted

[shalamov, 2016-05-20 14:50:58]

Reilly, could you please take a look?
I think I fixed the issues you've found.

Android part https://crrev.com/1486043002/ is reviewed by Ted, Tim and Nick.

[Reilly Grant (use Gerrit), 2016-05-20 18:31:22]

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js (right):

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:216:
this.router_.setIncomingReceiver(this);
Check usb-helpers.js for the new way to bind to a pipe using bindings.js.

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:77: // If NFCRecord type is not
provided, deduct NFCRecord type from JS data type:
You mean "deduce" here and elsewhere.

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:103: void
setMediaType(NFCRecordPtr&  recordPtr, const WTF::String& recordMediaType, const
WTF::String& defaultMediaType)
s/NFCRecordPtr&  recordPtr/NFCRecordPtr& recordPtr/

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:124:
array.append(static_cast<const uint8_t*>(buffer->data()), buffer->byteLength());
nit: const is unnecessary here

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:200: recordPtr->recordType =
toNFCRecordType(record.recordType());
This seems to allow the caller to specify a record type that does not match the
value that will be deduced from |record.data()| by the type converter. Is that
alright?

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:324: if (!value->IsString() &&
!(value->IsNumber() && !std::isnan(value.As<v8::Number>()->Value())))
nit: The parenthesis around the right hand side of the first && are unnecessary.

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:338: return
isValidTextRecord(record);
You may want to attempt to parse the URL and return KURL::IsValid().

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:555: m_requests.remove(resolver);
Only resolve() or reject() if |m_requests| still contains |resolver|.

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:568: for (ScriptPromiseResolver*
resolver : m_requests) {
nit: Unnecessary braces.

[shalamov, 2016-05-24 16:09:08]

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js (right):

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:216:
this.router_.setIncomingReceiver(this);
On 2016/05/20 18:31:21, Reilly Grant wrote:
> Check usb-helpers.js for the new way to bind to a pipe using bindings.js.

Done.

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:77: // If NFCRecord type is not
provided, deduct NFCRecord type from JS data type:
On 2016/05/20 18:31:22, Reilly Grant wrote:
> You mean "deduce" here and elsewhere.

Done.

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:124:
array.append(static_cast<const uint8_t*>(buffer->data()), buffer->byteLength());
On 2016/05/20 18:31:21, Reilly Grant wrote:
> nit: const is unnecessary here

Done.

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:200: recordPtr->recordType =
toNFCRecordType(record.recordType());
On 2016/05/20 18:31:21, Reilly Grant wrote:
> This seems to allow the caller to specify a record type that does not match
the
> value that will be deduced from |record.data()| by the type converter. Is that
> alright?

Conversion would be done only if type<->data combination is valid. Check is done
before mojo converter is used, in
NFC::push...isValidNFCPushMessage(pushMessage);

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:324: if (!value->IsString() &&
!(value->IsNumber() && !std::isnan(value.As<v8::Number>()->Value())))
On 2016/05/20 18:31:21, Reilly Grant wrote:
> nit: The parenthesis around the right hand side of the first && are
unnecessary.

I wanted to check whether number is NaN, only when v8 value is a number. If I
remove brackets, std::isnan(value.As<v8::Number>(); would be always executed.

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:338: return
isValidTextRecord(record);
On 2016/05/20 18:31:22, Reilly Grant wrote:
> You may want to attempt to parse the URL and return KURL::IsValid().

Done.

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:555: m_requests.remove(resolver);
On 2016/05/20 18:31:21, Reilly Grant wrote:
> Only resolve() or reject() if |m_requests| still contains |resolver|.

Done.

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:568: for (ScriptPromiseResolver*
resolver : m_requests) {
On 2016/05/20 18:31:21, Reilly Grant wrote:
> nit: Unnecessary braces.

Done.

[Reilly Grant (use Gerrit), 2016-05-24 19:25:39]

lgtm with minor corrections

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/1708543002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:324: if (!value->IsString() &&
!(value->IsNumber() && !std::isnan(value.As<v8::Number>()->Value())))
On 2016/05/24 at 16:09:08, shalamov wrote:
> On 2016/05/20 18:31:21, Reilly Grant wrote:
> > nit: The parenthesis around the right hand side of the first && are
unnecessary.
> 
> I wanted to check whether number is NaN, only when v8 value is a number. If I
remove brackets, std::isnan(value.As<v8::Number>(); would be always executed.

I see. I misread it as (!value->IsNumber() &&
!std::isnan(value.As<v8::Number>()->Value())).

https://codereview.chromium.org/1708543002/diff/360001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/nfc/push.html (right):

https://codereview.chromium.org/1708543002/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/nfc/push.html:59: promise_test(test => {
return promise_test

https://codereview.chromium.org/1708543002/diff/360001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js (right):

https://codereview.chromium.org/1708543002/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:170: class MockNFC
extends nfc.NFC.stubClass {
No need to extend nfc.NFC.stubClass.

https://codereview.chromium.org/1708543002/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:193: &&
!this.push_completed_) {
nit: put && on the end of the previous line

https://codereview.chromium.org/1708543002/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/nfc/resources/nfc-helpers.js:206: ||
(this.push_options_.target === target))) {
nit: put || on the end of the previous line

https://codereview.chromium.org/1708543002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/1708543002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/nfc/NFC.cpp:508: if
(!scriptState->getExecutionContext()->isSecureContext(errorMessage)) {
nit: unnecessary braces

[shalamov, 2016-05-26 14:09:06]

alexander.shalamov@intel.com changed reviewers:
+ nick@chromium.org

[shalamov, 2016-05-26 14:09:07]

nick@chromium.org: PTAL

Nick, would you have a time to check content/test/BUILD.gn ?

We agreed with Reilly (https://crrev.com/1912363002#msg16) to drop changes that
are not needed at the moment and temporarily introduce dependency for
content/tests, so that bindings that are used for testing would be generated.

[ncarter (slow), 2016-05-26 16:31:31]

lgtm

[shalamov, 2016-06-03 14:20:01]

The CQ bit was checked by alexander.shalamov@intel.com

[shalamov, 2016-06-03 14:20:03]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
mlamouri@chromium.org, reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/1708543002/#ps380001
(title: "Minor fixes for Reilly's comments.")

[commit-bot: I haz the power, 2016-06-03 14:20:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1708543002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1708543002/380001

[commit-bot: I haz the power, 2016-06-03 14:26:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 14:26:54]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[riju_, 2016-06-03 15:20:02]

third_party/WebKit/public/platform/modules/nfc/* LGTM

[shalamov, 2016-06-03 15:21:47]

The CQ bit was checked by alexander.shalamov@intel.com

[commit-bot: I haz the power, 2016-06-03 15:22:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1708543002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1708543002/380001

[commit-bot: I haz the power, 2016-06-03 15:29:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 15:29:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[shalamov, 2016-06-07 12:02:33]

alexander.shalamov@intel.com changed reviewers:
+ jochen@chromium.org

[shalamov, 2016-06-07 12:09:51]

@jochen, could you please take a look?

Looks like all files are 'approved', but presubmit bot still doesn't like
something.
Maybe I need to get green light from WebKit/public/OWNERS, for structural
changes?

[jochen (gone - plz use gerrit), 2016-06-07 18:57:28]

lgtm

[shalamov, 2016-06-08 12:25:46]

The CQ bit was checked by alexander.shalamov@intel.com

[commit-bot: I haz the power, 2016-06-08 12:26:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1708543002/380001

[commit-bot: I haz the power, 2016-06-08 13:44:42]

Description was changed from

==========
[webnfc] Implement push() method in blink nfc module.
    
This patch implements nfc.push() and nfc.cancelPush() methods in nfc module.

https://w3c.github.io/web-nfc/#dom-nfc-push
https://w3c.github.io/web-nfc/#dom-nfc-cancelpush
    
Mock mojo service and simple layout test is added to validate input parameters
as well as conversion between WebNFC and corresponding mojo data structures.
    
BUG=520391
==========

to

==========
[webnfc] Implement push() method in blink nfc module.
    
This patch implements nfc.push() and nfc.cancelPush() methods in nfc module.

https://w3c.github.io/web-nfc/#dom-nfc-push
https://w3c.github.io/web-nfc/#dom-nfc-cancelpush
    
Mock mojo service and simple layout test is added to validate input parameters
as well as conversion between WebNFC and corresponding mojo data structures.
    
BUG=520391
==========

[commit-bot: I haz the power, 2016-06-08 13:44:44]

Committed patchset #17 (id:380001)

[commit-bot: I haz the power, 2016-06-08 13:46:28]

Description was changed from

==========
[webnfc] Implement push() method in blink nfc module.
    
This patch implements nfc.push() and nfc.cancelPush() methods in nfc module.

https://w3c.github.io/web-nfc/#dom-nfc-push
https://w3c.github.io/web-nfc/#dom-nfc-cancelpush
    
Mock mojo service and simple layout test is added to validate input parameters
as well as conversion between WebNFC and corresponding mojo data structures.
    
BUG=520391
==========

to

==========
[webnfc] Implement push() method in blink nfc module.

This patch implements nfc.push() and nfc.cancelPush() methods in nfc module.

https://w3c.github.io/web-nfc/#dom-nfc-push
https://w3c.github.io/web-nfc/#dom-nfc-cancelpush

Mock mojo service and simple layout test is added to validate input parameters
as well as conversion between WebNFC and corresponding mojo data structures.

BUG=520391

Committed: https://crrev.com/5058e32db5582978f60a28bc0ccbeed0a7421283
Cr-Commit-Position: refs/heads/master@{#398538}
==========

[commit-bot: I haz the power, 2016-06-08 13:46:30]

Patchset 17 (id:??) landed as
https://crrev.com/5058e32db5582978f60a28bc0ccbeed0a7421283
Cr-Commit-Position: refs/heads/master@{#398538}

[xidachen, 2016-06-08 14:38:53]

A revert of this CL (patchset #17 id:380001) has been created in
https://codereview.chromium.org/2052473002/ by xidachen@chromium.org.

The reason for reverting is: The tests:
nfc/mock-nfc.html
nfc/push.html

added by this CL timed out on these two bots:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/6...

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/build....

[shalamov_, 2016-06-08 16:26:59]

On 2016/06/08 14:38:53, xidachen wrote:
> A revert of this CL (patchset #17 id:380001) has been created in
> https://codereview.chromium.org/2052473002/ by mailto:xidachen@chromium.org.
> 
> The reason for reverting is: The tests:
> nfc/mock-nfc.html
> nfc/push.html
> 
> added by this CL timed out on these two bots:
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/6...
> 
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/build....

@reillyg Looks like some bots are not generating js bindings. Dependency that
I've added to content/test/BUILD.gn is not enough. Do you have any suggestions
how can I fix this?

[Reilly Grant (use Gerrit), 2016-06-08 18:29:01]

On 2016/06/08 at 16:26:59, alexander.shalamov wrote:
> On 2016/06/08 14:38:53, xidachen wrote:
> > A revert of this CL (patchset #17 id:380001) has been created in
> > https://codereview.chromium.org/2052473002/ by mailto:xidachen@chromium.org.
> > 
> > The reason for reverting is: The tests:
> > nfc/mock-nfc.html
> > nfc/push.html
> > 
> > added by this CL timed out on these two bots:
> >
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/6...
> > 
> >
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/build....
> 
> @reillyg Looks like some bots are not generating js bindings. Dependency that
I've added to content/test/BUILD.gn is not enough. Do you have any suggestions
how can I fix this?

Are you sure the generated bindings are missing? The WebUSB tests are simply
slow and so these may be as well. You just need to add them to
third_party/WebKit/LayoutTests/SlowTests.

[Reilly Grant (use Gerrit), 2016-06-08 19:33:35]

https://codereview.chromium.org/1708543002/diff/380001/content/content_tests....
File content/content_tests.gypi (right):

https://codereview.chromium.org/1708543002/diff/380001/content/content_tests....
content/content_tests.gypi:1387:
'../device/nfc/nfc.gyp:device_nfc_mojo_bindings',
This dependency should be up in layouttest_support_content (and its GN
equivalent). The Webkit Tests bots don't build content_browsertests.

[shalamov, 2016-06-09 09:20:52]

Description was changed from

==========
[webnfc] Implement push() method in blink nfc module.

This patch implements nfc.push() and nfc.cancelPush() methods in nfc module.

https://w3c.github.io/web-nfc/#dom-nfc-push
https://w3c.github.io/web-nfc/#dom-nfc-cancelpush

Mock mojo service and simple layout test is added to validate input parameters
as well as conversion between WebNFC and corresponding mojo data structures.

BUG=520391

Committed: https://crrev.com/5058e32db5582978f60a28bc0ccbeed0a7421283
Cr-Commit-Position: refs/heads/master@{#398538}
==========

to

==========
[webnfc] Implement push() method in blink nfc module.

This patch implements nfc.push() and nfc.cancelPush() methods in nfc module.

https://w3c.github.io/web-nfc/#dom-nfc-push
https://w3c.github.io/web-nfc/#dom-nfc-cancelpush

Mock mojo service and simple layout test is added to validate input parameters
as well as conversion between WebNFC and corresponding mojo data structures.

BUG=520391

Committed: https://crrev.com/5058e32db5582978f60a28bc0ccbeed0a7421283
Cr-Commit-Position: refs/heads/master@{#398538}
==========

[shalamov, 2016-06-10 07:56:28]

The CQ bit was checked by alexander.shalamov@intel.com

[shalamov, 2016-06-10 07:56:28]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
jochen@chromium.org, rijubrata.bhaumik@intel.com, mlamouri@chromium.org,
tsepez@chromium.org, reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/1708543002/#ps400001
(title: "Move dependency from content_browsertests to
layouttest_support_content")

[commit-bot: I haz the power, 2016-06-10 07:56:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1708543002/400001

[commit-bot: I haz the power, 2016-06-10 09:03:08]

Description was changed from

==========
[webnfc] Implement push() method in blink nfc module.

This patch implements nfc.push() and nfc.cancelPush() methods in nfc module.

https://w3c.github.io/web-nfc/#dom-nfc-push
https://w3c.github.io/web-nfc/#dom-nfc-cancelpush

Mock mojo service and simple layout test is added to validate input parameters
as well as conversion between WebNFC and corresponding mojo data structures.

BUG=520391

Committed: https://crrev.com/5058e32db5582978f60a28bc0ccbeed0a7421283
Cr-Commit-Position: refs/heads/master@{#398538}
==========

to

==========
[webnfc] Implement push() method in blink nfc module.

This patch implements nfc.push() and nfc.cancelPush() methods in nfc module.

https://w3c.github.io/web-nfc/#dom-nfc-push
https://w3c.github.io/web-nfc/#dom-nfc-cancelpush

Mock mojo service and simple layout test is added to validate input parameters
as well as conversion between WebNFC and corresponding mojo data structures.

BUG=520391

Committed: https://crrev.com/5058e32db5582978f60a28bc0ccbeed0a7421283
Cr-Commit-Position: refs/heads/master@{#398538}
==========

[commit-bot: I haz the power, 2016-06-10 09:03:10]

Committed patchset #18 (id:400001)

[commit-bot: I haz the power, 2016-06-10 09:05:11]

Description was changed from

==========
[webnfc] Implement push() method in blink nfc module.

This patch implements nfc.push() and nfc.cancelPush() methods in nfc module.

https://w3c.github.io/web-nfc/#dom-nfc-push
https://w3c.github.io/web-nfc/#dom-nfc-cancelpush

Mock mojo service and simple layout test is added to validate input parameters
as well as conversion between WebNFC and corresponding mojo data structures.

BUG=520391

Committed: https://crrev.com/5058e32db5582978f60a28bc0ccbeed0a7421283
Cr-Commit-Position: refs/heads/master@{#398538}
==========

to

==========
[webnfc] Implement push() method in blink nfc module.

This patch implements nfc.push() and nfc.cancelPush() methods in nfc module.

https://w3c.github.io/web-nfc/#dom-nfc-push
https://w3c.github.io/web-nfc/#dom-nfc-cancelpush

Mock mojo service and simple layout test is added to validate input parameters
as well as conversion between WebNFC and corresponding mojo data structures.

BUG=520391

Committed: https://crrev.com/5058e32db5582978f60a28bc0ccbeed0a7421283
Committed: https://crrev.com/5c52ef129e520de01aa36728a4b0529b31e8467b
Cr-Original-Commit-Position: refs/heads/master@{#398538}
Cr-Commit-Position: refs/heads/master@{#399139}
==========

[commit-bot: I haz the power, 2016-06-10 09:05:13]

Patchset 18 (id:??) landed as
https://crrev.com/5c52ef129e520de01aa36728a4b0529b31e8467b
Cr-Commit-Position: refs/heads/master@{#399139}