[webnfc] Step 2. Add IDL stubs for NFCRecord, NFCPushOptions, NFCWatchOptions

[webnfc] Step 2.

Add IDL s for NFCRecord, NFCPushOptions, NFCWatchOptions.

enum NFCPushTarget {"tag", "peer"};

dictionary NFCPushOptions {
  NFCPushTarget target;
  unsigned long timeout;
};

enum NFCRecordType { "empty", "text", "url", "json", "opaque" };

dictionary NFCRecord {
  NFCRecordType kind;
  USVString type;
  any data;
};

dictionary NFCWatchOptions {
  USVString    url = "";
  USVString    kind = "";
  USVString    type = "";
  NFCWatchMode mode = "web-nfc-only";
};

enum NFCWatchMode {"web-nfc-only", "any"};

BUG=520391
Committed: https://crrev.com/47f74ffa7fb97338cab582aeffab45d817ea10b6
Cr-Commit-Position: refs/heads/master@{#353717}

[riju_, 2015-09-29 07:23:32]

rijubrata.bhaumik@intel.com changed reviewers:
+ jyasskin@chromium.org, kenneth.r.christiansen@intel.com

[riju_, 2015-09-29 07:23:32]

PTAL.

For NFCRecord idl, I am using "any". Please see ->
https://github.com/w3c/web-nfc/issues/52

[kenneth.christiansen, 2015-09-29 08:20:25]

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

[kenneth.christiansen, 2015-09-29 08:20:26]

Have you filed a blink bug for the issue you linked to? (that you have to use
"any")

https://codereview.chromium.org/1375483002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/nfc/NFCPushOptions.idl (right):

https://codereview.chromium.org/1375483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/nfc/NFCPushOptions.idl:13: unsigned long
timeout;  // in ms, default (and max) is 10sec, UAs MAY shorten it.
Why is that comment relevant here? Would make more sense in the actual code. And
a comment talking about UA MAY shorten it, is weird as Chrome is a UA

[riju_, 2015-09-30 08:09:54]

Patchset #2 (id:20001) has been deleted

[riju_, 2015-09-30 08:19:17]

Blink IDL bug filed : https://crbug.com/537133

https://codereview.chromium.org/1375483002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/nfc/NFCPushOptions.idl (right):

https://codereview.chromium.org/1375483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/nfc/NFCPushOptions.idl:13: unsigned long
timeout;  // in ms, default (and max) is 10sec, UAs MAY shorten it.
On 2015/09/29 08:20:26, kenneth.christiansen wrote:
> Why is that comment relevant here? Would make more sense in the actual code.
And
> a comment talking about UA MAY shorten it, is weird as Chrome is a UA

Done.

[Jeffrey Yasskin, 2015-10-12 22:13:45]

I'm going to be minimally available through the end of october, so please don't
block on my reviews.

https://codereview.chromium.org/1375483002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/nfc/NFCPushOptions.idl (right):

https://codereview.chromium.org/1375483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFCPushOptions.idl:7: enum NFCPushTarget {
"tag", "peer", "any" };
This doesn't match https://w3c.github.io/web-nfc/#the-nfcpushoptions-dictionary
anymore.

https://codereview.chromium.org/1375483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFCPushOptions.idl:12: NFCPushTarget
target; // default "any".
It looks like the default's gone away too.

https://codereview.chromium.org/1375483002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/nfc/NFCWatchOptions.idl (right):

https://codereview.chromium.org/1375483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFCWatchOptions.idl:12: USVString    url;
You should be able to include the defaults here, from
https://w3c.github.io/web-nfc/#dom-nfcwatchoptions.

[riju_, 2015-10-13 10:51:56]

Thanks jeffrey, hopefully the spec is more stable now.

https://codereview.chromium.org/1375483002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/nfc/NFCPushOptions.idl (right):

https://codereview.chromium.org/1375483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFCPushOptions.idl:7: enum NFCPushTarget {
"tag", "peer", "any" };
On 2015/10/12 22:13:45, jyasskin-OOO until Oct 30 wrote:
> This doesn't match
https://w3c.github.io/web-nfc/#the-nfcpushoptions-dictionary
> anymore. 

Done.

https://codereview.chromium.org/1375483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFCPushOptions.idl:12: NFCPushTarget
target; // default "any".
On 2015/10/12 22:13:45, jyasskin-OOO until Oct 30 wrote:
> It looks like the default's gone away too.

Done.

https://codereview.chromium.org/1375483002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/nfc/NFCWatchOptions.idl (right):

https://codereview.chromium.org/1375483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFCWatchOptions.idl:12: USVString    url;
On 2015/10/12 22:13:45, jyasskin-OOO until Oct 30 wrote:
> You should be able to include the defaults here, from
> https://w3c.github.io/web-nfc/#dom-nfcwatchoptions.

Done.

[kenneth.r.christiansen, 2015-10-13 10:57:45]

lgtm

[riju_, 2015-10-13 10:59:35]

The CQ bit was checked by rijubrata.bhaumik@intel.com

[commit-bot: I haz the power, 2015-10-13 11:00:03]

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

[commit-bot: I haz the power, 2015-10-13 11:04:40]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-10-13 11:09:12]

Patchset 3 (id:??) landed as
https://crrev.com/47f74ffa7fb97338cab582aeffab45d817ea10b6
Cr-Commit-Position: refs/heads/master@{#353717}

[Peter Beverloo, 2015-10-13 11:19:57]

peter@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2015-10-13 11:19:58]

A few drive-by nits, sorry :)

I came across this CL because of the subject: "[webnfc] Step 2.". The subject is
intended to be a brief one-liner describing what the change is. It'd be great if
you could try to make that more descriptive than "step 2".

https://codereview.chromium.org/1375483002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/nfc/NFCPushOptions.idl (right):

https://codereview.chromium.org/1375483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFCPushOptions.idl:10:
RuntimeEnabled=WebNFC,
nit: dictionaries are not Web exposed, no need for RuntimeEnabled= on them.

https://codereview.chromium.org/1375483002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/nfc/NFCRecord.idl (right):

https://codereview.chromium.org/1375483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFCRecord.idl:12: NFCRecordType kind;  //
based on TNF + JSON.
micro nit: comments such as this make the code a bit convoluted - one can read
the spec for this (and has to anyway, because I have no idea what TNF means).

https://codereview.chromium.org/1375483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/nfc/NFCRecord.idl:14: // TODO(riju_):
https://crbug.com/537133
nit: Could you please include a textual description with TODOs? The bug should
be referred for additional context, but it should be understandable without
following it.

[kenneth.christiansen, 2015-10-13 11:26:00]

On 2015/10/13 11:19:58, Peter Beverloo wrote:
> A few drive-by nits, sorry :)
> 
> I came across this CL because of the subject: "[webnfc] Step 2.". The subject
is
> intended to be a brief one-liner describing what the change is. It'd be great
if
> you could try to make that more descriptive than "step 2".

Doh, my fault, I should have paid attention to the title. I agree that this
should be more descriptive, like "Add WebIDL stubs for the WebNFC API" or
similar.

> nit: dictionaries are not Web exposed, no need for RuntimeEnabled= on them.

Thanks for that info.

> micro nit: comments such as this make the code a bit convoluted - one can read
> the spec for this (and has to anyway, because I have no idea what TNF means).

I had the same comment - but don't feel strongly about it