Fix #396: add section for local type record #451

zolkis · 2019-11-26T16:17:47Z

Signed-off-by: Zoltan Kis zoltan.kis@intel.com

Preview | Diff

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

beaufortfrancois

LGTM with nit

index.html

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

index.html

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

beaufortfrancois

See example comments

index.html

…ple. Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

index.html

…ypes Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

zolkis · 2019-12-06T13:00:09Z

In the last commit, a few changes were necessary.
Since the checks added for local and external types and they could not be added to the "switch on recordType" step, the create algorithm was modified:

id was moved in front, so that the switch could return the "map ... to NDEF" steps
the "map ... to NDEF" steps received another parameter, |ndef|
check for external type added
check for local type added

Then the "create NDEF record" steps were factored out to make it more modular.
Some white-space moving was done so that the "map ... to NDEF" algorithms are subsections to "create NDEF message".

…|ndef| parameter Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

kenchris · 2019-12-09T09:20:21Z

index.html

+    <section><h4>Well-known local types</h4>
+      <p>
+        NFC Forum <dfn>local type</dfn> that are defined by the NFC Forum or
+        by an application, and always start with lowercase letter or a


letter > character?

kenchris · 2019-12-09T09:20:53Z

index.html

+        by an application, and always start with lowercase letter or a
+        number. Those are usually short strings that are unique only within
+        the local context of the containing record. They are used when types
+        meaning doesn't matter outside of the local context of the


when teh meaning of types doesn't matter

kenchris · 2019-12-09T09:21:41Z

index.html

-    </p>
+    <div>
+      <dfn>Smart poster</dfn> is defined in [[NDEF-SMARTPOSTER]] to describe a
+      given Web content as an NDEF record that contains an <a>NDEF message</a>


web lowercase

No, it should be Web content, since webcontent is not a thing.
https://grammarist.com/spelling/web-site-website/

We use "Web" only for "Web NFC". We use "web" for "web page" and "web site" in the spec

kenchris · 2019-12-09T09:24:47Z

index.html

+          <a>action record</a> is missing.
+          <p class="note">
+            At the time of NDEF standardization the value `0` ("do the action") was
+            meant for use cases like send an SMS, make a call or launch browser.


meant - intented

Also corrected elsewhere.

kenchris · 2019-12-09T09:28:12Z

index.html

+        When the domain name is not provided, by default Web NFC uses the
+        <a href="https://html.spec.whatwg.org/multipage/origin.html#concept-origin-effective-domain">
+        effective domain of the origin</a> as domain name in external type
+        names when <a href="#mapping-external-data-to-ndef">creating external type records</a>. Therefore, applications MAY specify only the


kenchris · 2019-12-09T09:29:37Z

index.html

+      </p>
+      <p class="note">
+        The [[NFC-RTD]] mandates that external type names are represented
+        with the URN prefix “`urn:nfc:ext:`”, e.g. when reading


maybe clarify here that that is actually not encoded - so I don;t know how much they "mandate" it. It is more considered an URL with that prefix though that is not even encoded

In the NFC Forum spec it's absolutely normative.

That's why the Note was made, to justify why Web NFC does not comply.

kenchris · 2019-12-09T09:30:24Z

index.html

+        <li>
+          If |input| starts with colon ('`:`'), let |domain:string| be the
+          <a data-cite="url#concept-url-serializer">serialization</a> of the
+          <a href="https://html.spec.whatwg.org/multipage/origin.html#concept-origin-effective-domain">effective domain of


no easier way to quote? check https://respec.org/xref/

<a data-cite="html">effective domain</a> of the {{Document/origin}}

Thanks!

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

beaufortfrancois · 2019-12-09T13:06:01Z

index.html

+    <section><h4>Well-known global types</h4>
+      <p>
+        NFC Forum <dfn>global type</dfn>s are defined and managed by the
+        NFC Forum and usually start with uppercase letter.


For consistency, shall we use "character" as well there?

beaufortfrancois · 2019-12-09T13:09:03Z

index.html

-    </p>
+    <div>
+      <dfn>Smart poster</dfn> is defined in [[NDEF-SMARTPOSTER]] to describe a
+      given Web content as an NDEF record that contains an <a>NDEF message</a>


We use "Web" only for "Web NFC". We use "web" for "web page" and "web site" in the spec

beaufortfrancois · 2019-12-09T13:10:08Z

index.html

+        </li>
+        <li>
+          Zero or more <a>Text records</a> that act as a <dfn>title record</dfn>
+          related to the content. When there are more than one title records


s/records/record/ ?

Not sure about "web content". English grammar seems to prefer "Web content". At least it's disputed. But indeed if we can say "we" use "web content", that's what "we" use I guess :).

US English dictionaries changed... it used to be e-mail, now email is preferred and it used to be Web always, but now web is totally fine.

beaufortfrancois · 2019-12-09T13:12:40Z

index.html

+    <section><h3>Validating external type</h3>
+      <p>
+        The [[NFC-RTD]] specifies that external types MUST contain
+        the domain name of the issuing organization, a a colon and a type


Typo: Remove one "a"

beaufortfrancois · 2019-12-09T13:14:31Z

index.html

+        domain name in external type names when
+        <a href="#mapping-external-data-to-ndef">
+        creating external type records</a>. Therefore, applications MAY specify
+        only the colon and the external type name.


I'd love a JS example of this for readers in a follow-up CL. Please track this.

The example with "example.com:sp" could be written as ":sp" if the app wanted the Web NFC implementation prefix it with the effective domain name of the document origin. Should that be another example (when the difference is so small)?

A simple comment in the existing example could be enough.

beaufortfrancois · 2019-12-09T13:15:17Z

index.html

+      <ol class=algorithm id="validate-local-type">
+        <li>
+          If |input| is not a {{USVString}} or its length exceeds 255 bytes,
+          return `false` and terminate these steps.


Please use "abort" instead of "terminate".

Noted for the future as well. Is there a difference in terminate vs abort? Abort (for me) means we do clean-up steps as well, possibly rolling back a transaction. Terminate means jumping to the end.

Terminology is not specified that precisely to my knowledge, but abort is what most specs use today

I'm not sure if there's a difference to be honest.
FYI https://github.com/search?q=org%3AWICG+%22terminate+these+steps%22&type=Code vs https://github.com/search?q=org%3AWICG+%22abort+these+steps%22&type=Code

We used both in manifest but mostly moved to abort now

OK, so it's internal terminology (not aligned with terms outside web specs).

beaufortfrancois · 2019-12-09T13:15:40Z

index.html

+          return `false` and terminate these steps.
+        </li>
+        <li>
+          If |input| does not start with a lowercase letter or a number,


s/letter/character/

beaufortfrancois · 2019-12-09T13:17:56Z

index.html

-        To <dfn>map empty record to NDEF</dfn> given a |record:NDEFRecordInit|,
-        run these steps:
+        To <dfn>map empty record to NDEF</dfn> given a |record:NDEFRecordInit|
+        and |ndef|, run these steps:


Algorithm below should not state "Let ndefRecord be the notation for the NDEF record to be created by the UA." anymore if I'm reading this correctly.

Hmm, it doesn't.

But it did say "Let |ndef| be ...", right. Thanks for noticing.

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

beaufortfrancois

Still LGTM

leonhsl · 2019-12-11T08:30:30Z

I do suggest not to commit 12 commits from one PR, the repo's commit list looks kind of a mess and hard to understand what's going on. Could we squash and commit?
I'm unsure there's some well-accepted rule when working on github though. On Chromium each CL is supposed to complete one task cleary with a helpful commit message.

beaufortfrancois · 2019-12-11T08:39:08Z

+1 to @leonhsl's comment
Please squash with a nice commit message.

leonhsl · 2019-12-11T08:39:51Z

Also, I'm confused about why I was not notified to check whether my comments are addressed, seems due to some magical operations (I'm not clear) on multiple commits... Usually, once people started a review they want to follow along the way.

zolkis · 2019-12-11T08:47:10Z

Some commits are worth squashing, some others aren't.
If there are a lot of changes in a PR, they have to go somewhere. It does not make sense to squash into a single commit that changes half the spec. It won't help any readability.
However, review comment fixes could be squashed.

So now should I make a PR for modifying upstream history?

zolkis · 2019-12-11T08:50:08Z

I'm confused about why I was not notified to check whether my comments are addressed

github should notify you about new commits made on PR's where you commented.

leonhsl · 2019-12-11T09:09:20Z

I did not receive any updates for this PR until today. I guess the scenario is like, I added some comments on your commit (or a branch in your own repo?), after that you added that commit into this PR as part of it, so strictly speaking I was not commenting on this PR.

At least for me, I do not know how to modify the history commit list.. I'm just suggesting to manage each PR (or commit) clearly to be something logical, it's a general rule I suppose, and the writer has the freedom to do that flexibly.

Fix w3c#396: add section for local type record

6d3209f

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

beaufortfrancois approved these changes Nov 27, 2019

View reviewed changes

index.html Outdated Show resolved Hide resolved

zolkis added 3 commits November 30, 2019 20:00

Add links to global and local types

47612be

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

Merge branch 'gh-pages' of github.com:w3c/web-nfc into gh-pages

6270a3a

Fix w3c#458, handling local types

e7f47e2

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

kenchris reviewed Dec 2, 2019

View reviewed changes

index.html Outdated Show resolved Hide resolved

Add (draft) algorithms to validate external and local types

e4b97cd

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

zolkis requested a review from beaufortfrancois December 2, 2019 15:11

beaufortfrancois reviewed Dec 3, 2019

View reviewed changes

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

Move local type section. Fix local type bugs. Clarify Sp. Fix Sp exam…

7ee2c8e

…ple. Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

zolkis force-pushed the gh-pages branch from e0592b0 to 7ee2c8e Compare December 3, 2019 14:20

Update the validate external/local types and the create NDEF algorithm

fe6862f

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

beaufortfrancois reviewed Dec 6, 2019

View reviewed changes

index.html Outdated Show resolved Hide resolved

index.html Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

zolkis added 2 commits December 6, 2019 14:49

Factor out 'create NDEF record', update handling local and external t…

2963412

…ypes Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

Merge branch 'gh-pages' of github.com:w3c/web-nfc into gh-pages

b581443

Fix some of the 'map ... to NDEF' algorithms to work with additional …

bba4944

…|ndef| parameter Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

kenchris reviewed Dec 9, 2019

View reviewed changes

Small editorial fixes

ab0877e

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

zolkis force-pushed the gh-pages branch from 5393091 to ab0877e Compare December 9, 2019 11:46

beaufortfrancois reviewed Dec 9, 2019

View reviewed changes

kenchris approved these changes Dec 9, 2019

View reviewed changes

Other small editorial fixes

feb6abd

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

beaufortfrancois approved these changes Dec 9, 2019

View reviewed changes

zolkis merged commit 80a2cdb into w3c:gh-pages Dec 11, 2019

Fix #396: add section for local type record #451

Fix #396: add section for local type record #451

Conversation

zolkis commented Nov 26, 2019 • edited by pr-preview bot

beaufortfrancois left a comment

Choose a reason for hiding this comment

beaufortfrancois left a comment

Choose a reason for hiding this comment

zolkis commented Dec 6, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenchris Dec 9, 2019 • 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

beaufortfrancois left a comment

Choose a reason for hiding this comment

leonhsl commented Dec 11, 2019

beaufortfrancois commented Dec 11, 2019

leonhsl commented Dec 11, 2019

zolkis commented Dec 11, 2019

zolkis commented Dec 11, 2019

leonhsl commented Dec 11, 2019

zolkis commented Nov 26, 2019 •

edited by pr-preview bot

kenchris Dec 9, 2019 •

edited