[webnfc] Align nfc.push operation with the specification

[webnfc] Align nfc.push operation with the specification

This CL aligns error handling for nfc.push operation, with the
latest changes in the specification. Whenever invlid type is
provided, resulting promise is rejected with 'TypeError' exception.
When error happens due to the invalid sytnax or tokens, resulting
promise is rejected with 'SyntaxError' exception. Layout test
updated accordingly.

BUG=718349
Review-Url: https://codereview.chromium.org/2885813002
Cr-Commit-Position: refs/heads/master@{#474265}
Committed: https://chromium.googlesource.com/chromium/src/+/f33e1d034eabce26a32bf9b05f3ca29a924fd237

[shalamov, 2017-05-16 13:57:05]

The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 13:57:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shalamov, 2017-05-16 14:10:33]

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

[shalamov, 2017-05-16 14:10:34]

Could you please take a look?

[Reilly Grant (use Gerrit), 2017-05-16 15:21:04]

https://codereview.chromium.org/2885813002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/nfc/push.html (right):

https://codereview.chromium.org/2885813002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/nfc/push.html:150: }, 'nfc.push with "empty"
record should succeed.');
Can an empty record with data succeed?

https://codereview.chromium.org/2885813002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/2885813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/nfc/NFC.cpp:706: "Cannot set WebNFC Id.");
Not this change but are "Cannot convert NFCMessage" and "Cannot set WebNFC Id"
actionable for the developer or are they internal errors?

https://codereview.chromium.org/2885813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/nfc/NFC.cpp:712: "NFCMessage exceeds maximum
suppoted size.");
supported

https://codereview.chromium.org/2885813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/nfc/NFC.cpp:773: "Provided watch id, cannot be
found.");
no comma

[commit-bot: I haz the power, 2017-05-16 16:51:37]

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

[commit-bot: I haz the power, 2017-05-16 16:51:37]

Dry run: This issue passed the CQ dry run.

[Alexander Shalamov, 2017-05-16 17:49:10]

https://codereview.chromium.org/2885813002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/nfc/push.html (right):

https://codereview.chromium.org/2885813002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/nfc/push.html:150: }, 'nfc.push with "empty"
record should succeed.');
On 2017/05/16 15:21:03, Reilly Grant wrote:
> Can an empty record with data succeed?

https://w3c.github.io/web-nfc/#mapping-empty-record-to-ndef spec is not strict
about such case, if developer provides data for 'empty' record, data would be
discarded. Do you want me to open an issue for the spec?

https://codereview.chromium.org/2885813002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/2885813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/nfc/NFC.cpp:706: "Cannot set WebNFC Id.");
On 2017/05/16 15:21:04, Reilly Grant wrote:
> Not this change but are "Cannot convert NFCMessage" and "Cannot set WebNFC Id"
> actionable for the developer or are they internal errors?

> "Cannot set WebNFC Id"
https://w3c.github.io/web-nfc/#creating-a-web-nfc-id . Document origin + user
specified path, could cause error due to incorrect input. Need a layout test for
that.

>"Cannot convert NFCMessage"
Could happen when JSON is serialized, I will try to figure out the test for that
tomorrow. Probably, when there is an object, like function or undefined.

[shalamov, 2017-05-17 13:12:26]

https://codereview.chromium.org/2885813002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):

https://codereview.chromium.org/2885813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/nfc/NFC.cpp:706: "Cannot set WebNFC Id.");
On 2017/05/16 17:49:10, Alexander Shalamov wrote:
> On 2017/05/16 15:21:04, Reilly Grant wrote:
> > Not this change but are "Cannot convert NFCMessage" and "Cannot set WebNFC
Id"
> > actionable for the developer or are they internal errors?
> 
> > "Cannot set WebNFC Id"
> https://w3c.github.io/web-nfc/#creating-a-web-nfc-id . Document origin + user
> specified path, could cause error due to incorrect input. Need a layout test
for
> that.
> 
> >"Cannot convert NFCMessage"
> Could happen when JSON is serialized, I will try to figure out the test for
that
> tomorrow. Probably, when there is an object, like function or undefined.
> 

- Added new test for JSON serialization and check when stringifier throws an
exception.
- Added new test for invalid WebNFC id, e.g., when provided path component is
invalid.

https://codereview.chromium.org/2885813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/nfc/NFC.cpp:712: "NFCMessage exceeds maximum
suppoted size.");
On 2017/05/16 15:21:04, Reilly Grant wrote:
> supported

Done.

https://codereview.chromium.org/2885813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/nfc/NFC.cpp:773: "Provided watch id, cannot be
found.");
On 2017/05/16 15:21:04, Reilly Grant wrote:
> no comma

Done.

[shalamov, 2017-05-17 13:12:35]

The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 13:12:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 15:03:08]

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

[commit-bot: I haz the power, 2017-05-17 15:03:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2017-05-17 15:11:22]

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

https://codereview.chromium.org/2885813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFC.cpp:201: if
(!stringify_result.ToLocal(&jsonString) || try_catch.HasCaught()) {

I prefer avoiding using a MaybeLocal handle.

v8::Local<v8::String> jsonString;
if (!Stringify(...).ToLocal(&jsonString)) {
  DCHECK(try_catch.HasCaught());
  ...;
}

https://codereview.chromium.org/2885813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFC.cpp:205: WTF::String wtfString =
blink::V8StringToWebCoreString<WTF::String>(

WTF:: won't be needed.

blink:: won't be needed.

[shalamov, 2017-05-18 08:30:56]

The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 08:31:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shalamov, 2017-05-18 08:34:04]

Thanks for looking.

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

https://codereview.chromium.org/2885813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFC.cpp:201: if
(!stringify_result.ToLocal(&jsonString) || try_catch.HasCaught()) {
On 2017/05/17 15:11:22, haraken wrote:
> 
> I prefer avoiding using a MaybeLocal handle.
> 
> v8::Local<v8::String> jsonString;
> if (!Stringify(...).ToLocal(&jsonString)) {
>   DCHECK(try_catch.HasCaught());
>   ...;
> }

I removed temporary MaybeLocal, but I left try_catch without DCHECK. Spec states
that if serialization throws, throw 'SyntaxError' and abort algorithm steps
(https://w3c.github.io/web-nfc/#mapping-json-to-ndef).

https://codereview.chromium.org/2885813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFC.cpp:205: WTF::String wtfString =
blink::V8StringToWebCoreString<WTF::String>(
On 2017/05/17 15:11:22, haraken wrote:
> 
> WTF:: won't be needed.
> 
> blink:: won't be needed.

WTF:: is not needed. Removed in other places as well. blink:: is still needed,
since this code is in mojo namespace.

[commit-bot: I haz the power, 2017-05-18 10:53:46]

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

[commit-bot: I haz the power, 2017-05-18 10:53:47]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Reilly Grant (use Gerrit), 2017-05-18 21:49:37]

lgtm

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

https://codereview.chromium.org/2885813002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFC.cpp:384: "The data for the 'text'
NFCRecords must be of "
"for the 'text' NFCRecords" -> "for 'text' NFCRecords"

(And below, remove the word "the".)

[shalamov, 2017-05-19 07:02:06]

The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 07:02:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shalamov, 2017-05-19 07:03:03]

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

https://codereview.chromium.org/2885813002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFC.cpp:384: "The data for the 'text'
NFCRecords must be of "
On 2017/05/18 21:49:37, Reilly Grant wrote:
> "for the 'text' NFCRecords" -> "for 'text' NFCRecords"
> 
> (And below, remove the word "the".)

Done.

[commit-bot: I haz the power, 2017-05-19 10:51:50]

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

[commit-bot: I haz the power, 2017-05-19 10:51:51]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[shalamov, 2017-05-23 11:17:56]

On 2017/05/19 07:03:03, shalamov wrote:
>
https://codereview.chromium.org/2885813002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/nfc/NFC.cpp (right):
> 
>
https://codereview.chromium.org/2885813002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/nfc/NFC.cpp:384: "The data for the 'text'
> NFCRecords must be of "
> On 2017/05/18 21:49:37, Reilly Grant wrote:
> > "for the 'text' NFCRecords" -> "for 'text' NFCRecords"
> > 
> > (And below, remove the word "the".)
> 
> Done.

haraken@ Would you have time to check updated CL? Thanks.

[haraken, 2017-05-23 15:16:18]

LGTM

[shalamov, 2017-05-24 06:54:51]

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

[shalamov, 2017-05-24 06:54:51]

The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2885813002/#ps60001
(title: "Removed 'the' articles")

[commit-bot: I haz the power, 2017-05-24 06:55:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 06:58:27]

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

[commit-bot: I haz the power, 2017-05-24 06:58:28]

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...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shalamov, 2017-05-24 08:17:52]

The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 08:18:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 08:20:39]

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

[commit-bot: I haz the power, 2017-05-24 08:20:41]

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shalamov, 2017-05-24 08:22:53]

Patchset #5 (id:80001) has been deleted

[shalamov, 2017-05-24 11:02:52]

The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 11:03:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 12:38:26]

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

[commit-bot: I haz the power, 2017-05-24 12:38:27]

Dry run: This issue passed the CQ dry run.

[shalamov, 2017-05-24 12:47:58]

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

[shalamov, 2017-05-24 12:47:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2885813002/#ps100001
(title: "Rebased to master")

[commit-bot: I haz the power, 2017-05-24 12:48:17]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 12:52:21]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495630078008780,
"parent_rev": "ffd476b19f33472d40f4c48905561b28f1ca7ab6", "commit_rev":
"f33e1d034eabce26a32bf9b05f3ca29a924fd237"}

[commit-bot: I haz the power, 2017-05-24 12:52:35]

Description was changed from

==========
[webnfc] Align nfc.push operation with the specification

This CL aligns error handling for nfc.push operation, with the
latest changes in the specification. Whenever invlid type is
provided, resulting promise is rejected with 'TypeError' exception.
When error happens due to the invalid sytnax or tokens, resulting
promise is rejected with 'SyntaxError' exception. Layout test
updated accordingly.

BUG=718349
==========

to

==========
[webnfc] Align nfc.push operation with the specification

This CL aligns error handling for nfc.push operation, with the
latest changes in the specification. Whenever invlid type is
provided, resulting promise is rejected with 'TypeError' exception.
When error happens due to the invalid sytnax or tokens, resulting
promise is rejected with 'SyntaxError' exception. Layout test
updated accordingly.

BUG=718349

Review-Url: https://codereview.chromium.org/2885813002
Cr-Commit-Position: refs/heads/master@{#474265}
Committed:
https://chromium.googlesource.com/chromium/src/+/f33e1d034eabce26a32bf9b05f3c...
==========

[commit-bot: I haz the power, 2017-05-24 12:52:37]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f33e1d034eabce26a32bf9b05f3c...