Move watch options to NFCReader ctor #180

kenchris · 2018-12-07T10:18:57Z

This is an incremental change, as watch will be removed in future PR

kenchris · 2018-12-07T10:21:12Z

index.html

+          <a>For each</a> <var>key</var> → <var>value</var> of <var>options</var>:
+          <ul>
+            <li>
+              If the <a>NFCReaderOptions</a>


This should probably be NFCReader, because otherwise I will copy signal as well. Maybe I should just list our the attributes? @anssiko

Nit: If you have a normative definition here, then lines 1478-1482 should be a note. But, in WebIDL, attributes are getter/setters, not own properties, so their definition isn't determined by setting the attribute to a value, but saying that, when you read the attribute, you get that value.

zolkis · 2018-12-07T11:22:38Z

index.html

      attribute EventHandler onreading;
+
+      Promise&lt;void&gt; watch();


IMHO removing the argument of watch() and passing it to the constructor is half-breed solution.
I would use either watch() with the options, or constructor with the function passed together with options.

This is an incremental change to make sure it all works, watch() will be removed soon enough and it will be activated when event listener is attached

Incremental changes SGTM. I like the plan of activating when the event listener is attached.

@zolkis You might want to avoid using analogies like "half-breed", which could be considered demeaning.

zolkis · 2018-12-07T11:24:33Z

index.html

          USVString url = "";
          NFCRecordType recordType;
          USVString mediaType = "";
-          NFCWatchMode mode = "web-nfc-only";
+          NFCAccept accept = "web-nfc-only";


Isn't accept a bit overloaded? I feel that mode was better compromise for capturing what it is an accept-policy then just accept. Unless accept has one well defined meaning and usage in other specs.

It is used by http at least. Maybe we can find a better name, but for now, WatchMode doesn't make so much sense as I am removing watch

accept SGTM; the analogy makes sense

zolkis · 2018-12-07T11:25:12Z

index.html

    <p>
      This enum defines the set of known values denoting whether
      <code><a href="#dom-nfcreader-watch">watch()</a></code> operation should
      <a>dispatch NFC content</a> for <a>Web NFC content</a> or any
      <a>NFC content</a>
    </p>
    <pre class="idl">
-      enum NFCWatchMode {
+      enum NFCAccept {


Same comment as above with accept being overloaded term in comms.

This name isn't web-exposed, so I don't think it matters so much to decide the perfect one right now.

littledan · 2018-12-07T12:21:03Z

index.html

+  <p>
+    The <dfn>NFCReader.url</dfn>, <dfn>NFCReader.recordType</dfn>,
+    <dfn>NFCReader.mediaType</dfn> and <dfn>NFCReader.accept</dfn> correlates to
+    the members of <a>NFCReaderOptions</a>.


Nit: s/correlates the/have the values of the corresponding/ (when correlates is used as a transitive verb, the subject is the person doing the analysis)

littledan · 2018-12-07T12:24:16Z

index.html

+          <ul>
+            <li>
+              If the <a>NFCReaderOptions</a>
+              <a data-lt="list-contain">contains</a> <var>key</var>,


WebIDL will already filter out the irrelevant keys, so I don't think this conditional is necessary. Or did you mean to check whether NFCReader contains the key, to filter out signal?

This was based of generic sensor spec (geolocation sensor)

littledan · 2018-12-07T12:26:45Z

index.html

@@ -2633,7 +2674,7 @@ <h2>The <dfn>NFCWatchMode</dfn> enum</h2>
      </p>
      <p>
        An <dfn>NFC watch</dfn> is referring to a tuple of a <code><a>Promise</a></code>,


English nit: s/is referring to/refers to/

littledan · 2018-12-07T12:28:44Z

index.html

@@ -2710,7 +2748,7 @@ <h2>The <dfn>NFCWatchMode</dfn> enum</h2>
                <code>"NotSupportedError"</code> and return <var>p</var>.
              </li>
              <li>
-                If the <var>options</var>.url is not an empty <code>String</code>
+                If the <var>reader_instance</var>.url is not an empty <code>String</code>


Hmm, somehow this text should clarify, you don't mean Get(reader_instance, "url") (since that could be modified using Object.defineProperty), but you mean the original value. You could specify this using internal slots, or a single big internal slot for the options dictionary.

littledan · 2018-12-07T12:32:33Z

index.html

+      readonly attribute USVString url;
+      readonly attribute NFCRecordType recordType;
+      readonly attribute USVString mediaType;
+      readonly attribute NFCAccept accept;


Just curious, what is the motivation for exposing these to read out? It seems like this will just be the options that were passed in, so I don't really see the use case. Did you just add these attributes so that they were stored internally in the NFCReader? You might want to use internal slots for that purpose, if so.

Maybe you are right... I was thinking about it being nice to store if you had multiple readers but I guess we can just drop that.

littledan

The biggest thing I'd want clarified before approving is whether you intended to expose the options as attributes; besides that, all comments are editorial and would be fine to clean up later.

littledan · 2018-12-07T14:33:27Z

index.html

            <var>message</var>.url as a <code><a>Web NFC Id</a></code>. If
            <a href="#dfn-match-web-nfc-id-with-url-pattern">URL pattern match</a>
            algorithm returns <code>false</code>, skip to the next <a>NFC watch</a>.
          </li>
          <li>
-            If <var>options</var>.recordType is <a href=#dfn-not-present>present</a> and
-            it is not equal to any <var>record</var>.recordType where <var>record</var>
+            If <var>reader_instance</var>.[[\RecordType] is <a href=#dfn-not-present>present</a> and


Nit: missing ]

littledan · 2018-12-07T15:51:17Z

index.html

-            If <var>options</var>.recordType is <a href=#dfn-not-present>present</a> and
-            it is not equal to any <var>record</var>.recordType where <var>record</var>
+            If <var>reader_instance</var>.[[\RecordType] is <a href=#dfn-not-present>present</a> and
+            it is not equal to any <var>record</var>.[[\recordType]] where <var>record</var>


Where is the type of these records defined?

Probably either both this line and line 3041 should use internal slot notation or property notation, but not one of each.

These are internal slots on the NFCReader. But the NFCRecord (the data we read) has these as properties

http://localhost:8000/#dom-nfcrecord

Maybe we need to define the internal slots for the NFCReader somewhere. Can be a follow-up

kenchris commented Dec 7, 2018

View reviewed changes

zolkis reviewed Dec 7, 2018

View reviewed changes

littledan reviewed Dec 7, 2018

View reviewed changes

kenchris force-pushed the gh-pages branch from 09e86c0 to 55f8a0f Compare December 7, 2018 13:37

littledan approved these changes Dec 7, 2018

View reviewed changes

Move watch options to NFCReader ctor

1ea8243

kenchris force-pushed the gh-pages branch from 55f8a0f to 1ea8243 Compare December 7, 2018 16:03

kenchris merged commit 023d6c3 into w3c:gh-pages Dec 7, 2018

Honry mentioned this pull request Dec 13, 2018

[web-nfc] Update tests to use new NFCReader and NFCWriter object web-platform-tests/wpt#14389

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move watch options to NFCReader ctor #180

Move watch options to NFCReader ctor #180

kenchris commented Dec 7, 2018 •

edited

kenchris Dec 7, 2018

littledan Dec 7, 2018

zolkis Dec 7, 2018

kenchris Dec 7, 2018

littledan Dec 7, 2018

zolkis Dec 7, 2018

kenchris Dec 7, 2018

littledan Dec 7, 2018

zolkis Dec 7, 2018

littledan Dec 7, 2018

littledan Dec 7, 2018

littledan Dec 7, 2018 •

edited

kenchris Dec 7, 2018

littledan Dec 7, 2018

littledan Dec 7, 2018

littledan Dec 7, 2018

kenchris Dec 7, 2018

littledan left a comment

littledan Dec 7, 2018

littledan Dec 7, 2018

kenchris Dec 7, 2018

kenchris Dec 7, 2018

kenchris Dec 7, 2018

Move watch options to NFCReader ctor #180

Move watch options to NFCReader ctor #180

Conversation

kenchris commented Dec 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan Dec 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenchris commented Dec 7, 2018 •

edited

littledan Dec 7, 2018 •

edited