Fix isLocal algorithm #493

beaufortfrancois · 2019-12-25T09:00:25Z

As suggested by @leonhsl at #491 (comment) and #491 (comment), this PR refactors "Creating NDEF record" algorithm for isLocal and makes sure we set isLocal=true only for TNF=1 records.

Preview | Diff

leonhsl · 2019-12-25T09:41:09Z

index.html

@@ -3752,6 +3752,10 @@ <h3>Parsing content</h3>
      <li>
        If |ndef|'s |typeNameField| is `1` ([=well-known type record=]):
        <ol>
+          <li>
+            Set |record|'s <a>isLocal</a> to `true` if |ndef| is a payload


I suppose {U, T, Sp} types here should not be set isLocal=true.

Not all sub-records must be local types.

We need to decide what isLocal means: to denote that a record is in the payload of another record, of if the type is a local type.

From a previous comment of @leonhsl it seems we want that isLocal means only a local type, not sub-recordness. However, when creating records, a local type may only exist in a containing record. But other than local types may be part of a message that is payload to a record.

Yeah, that's what I read from the discussion you had in the issue, isLocal is to denote the record is a local type, to differentiate a "text" local type record (sub-record of course) from a TEXT sub-record.

I think we allow users to create local type records at all times, but we reject the push() promise when we found the top-level message contains a local type record.

I was also thinking if we could consider an implicit context (no conflicts within a message), but the NFC Forum RTD clearly says "NFC Forum Local Types are available for use within the context of another record". So local type may only occur in the payload of a record, but we should set isLocal only for those sub-records that actually have a local type. Done :).

zolkis · 2019-12-25T11:00:42Z

index.html

-            to another <a>NDEF record</a>, reject |promise| with a {{TypeError}}
-            and abort these steps.
-          </li>
+            If |record|'s <a>isLocal</a> is `true`:


Looks like you moved this to the front, but the validate local type steps were written with the old algorithm in mind. If we check local types up-front, we need to update/change the validation algorithm as well.

Hmm, on a second look, the current validation algorithm should fare pretty well here as well.

zolkis · 2019-12-26T10:11:52Z

I think I will merge this and continue from there. Thanks!

Some notable points: 1) Local type in WebNFC APIs is always prefixed by ':', but, the ':' will be omitted when it's actually written into the nfc tag. ":act" --> "act" to be written as the TYPE field into the nfc tag. ":text" --> "text" The reading direction is vice versa. "act" --> ":act" to be exposed as NDEFRecord#recordType. "text" --> ":text" 2) Only "smart-poster", external, and local type records are supposed to be able to carry a ndef message as payload. 3) Local type is only expected to exist inside a ndef message that is another ndef record's payload. Top level ndef message is not allowed to have a local type record. The spec changes: w3c/web-nfc#491 w3c/web-nfc#493 w3c/web-nfc#495 w3c/web-nfc#502 w3c/web-nfc#506 BUG=520391 Change-Id: Ic2890c031109aa583437ac93a8901ff71992af78

Some notable points: 1) Local type in WebNFC APIs is always prefixed by ':', but, the ':' will be omitted when it's actually written into the nfc tag. ":act" --> "act" to be written as the TYPE field into the nfc tag. ":text" --> "text" The reading direction is vice versa. "act" --> ":act" to be exposed as NDEFRecord#recordType. "text" --> ":text" 2) Only "smart-poster", external, and local type records are supposed to be able to carry a ndef message as payload. 3) Local type is only expected to exist inside a ndef message that is another ndef record's payload. Top level ndef message is not allowed to have a local type record. The spec changes: w3c/web-nfc#491 w3c/web-nfc#493 w3c/web-nfc#495 w3c/web-nfc#502 w3c/web-nfc#506 BUG=520391 Change-Id: Ic2890c031109aa583437ac93a8901ff71992af78 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1996946 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Reilly Grant <reillyg@chromium.org> Commit-Queue: Leon Han <leon.han@intel.com> Cr-Commit-Position: refs/heads/master@{#737290}

…ype records, a=testonly Automatic update from web-platform-tests [webnfc] Support writing/reading local type records Some notable points: 1) Local type in WebNFC APIs is always prefixed by ':', but, the ':' will be omitted when it's actually written into the nfc tag. ":act" --> "act" to be written as the TYPE field into the nfc tag. ":text" --> "text" The reading direction is vice versa. "act" --> ":act" to be exposed as NDEFRecord#recordType. "text" --> ":text" 2) Only "smart-poster", external, and local type records are supposed to be able to carry a ndef message as payload. 3) Local type is only expected to exist inside a ndef message that is another ndef record's payload. Top level ndef message is not allowed to have a local type record. The spec changes: w3c/web-nfc#491 w3c/web-nfc#493 w3c/web-nfc#495 w3c/web-nfc#502 w3c/web-nfc#506 BUG=520391 Change-Id: Ic2890c031109aa583437ac93a8901ff71992af78 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1996946 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Reilly Grant <reillyg@chromium.org> Commit-Queue: Leon Han <leon.han@intel.com> Cr-Commit-Position: refs/heads/master@{#737290} -- wpt-commits: a1652e2faab5c92db0d3a2523ef9a94003e1e8f5 wpt-pr: 21157

…ype records, a=testonly Automatic update from web-platform-tests [webnfc] Support writing/reading local type records Some notable points: 1) Local type in WebNFC APIs is always prefixed by ':', but, the ':' will be omitted when it's actually written into the nfc tag. ":act" --> "act" to be written as the TYPE field into the nfc tag. ":text" --> "text" The reading direction is vice versa. "act" --> ":act" to be exposed as NDEFRecord#recordType. "text" --> ":text" 2) Only "smart-poster", external, and local type records are supposed to be able to carry a ndef message as payload. 3) Local type is only expected to exist inside a ndef message that is another ndef record's payload. Top level ndef message is not allowed to have a local type record. The spec changes: w3c/web-nfc#491 w3c/web-nfc#493 w3c/web-nfc#495 w3c/web-nfc#502 w3c/web-nfc#506 BUG=520391 Change-Id: Ic2890c031109aa583437ac93a8901ff71992af78 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1996946 Reviewed-by: Daniel Cheng <dchengchromium.org> Reviewed-by: Reilly Grant <reillygchromium.org> Commit-Queue: Leon Han <leon.hanintel.com> Cr-Commit-Position: refs/heads/master{#737290} -- wpt-commits: a1652e2faab5c92db0d3a2523ef9a94003e1e8f5 wpt-pr: 21157 UltraBlame original commit: db0bf96c8b8840896e7a762605492f3f55594981

…ype records, a=testonly Automatic update from web-platform-tests [webnfc] Support writing/reading local type records Some notable points: 1) Local type in WebNFC APIs is always prefixed by ':', but, the ':' will be omitted when it's actually written into the nfc tag. ":act" --> "act" to be written as the TYPE field into the nfc tag. ":text" --> "text" The reading direction is vice versa. "act" --> ":act" to be exposed as NDEFRecord#recordType. "text" --> ":text" 2) Only "smart-poster", external, and local type records are supposed to be able to carry a ndef message as payload. 3) Local type is only expected to exist inside a ndef message that is another ndef record's payload. Top level ndef message is not allowed to have a local type record. The spec changes: w3c/web-nfc#491 w3c/web-nfc#493 w3c/web-nfc#495 w3c/web-nfc#502 w3c/web-nfc#506 BUG=520391 Change-Id: Ic2890c031109aa583437ac93a8901ff71992af78 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1996946 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Reilly Grant <reillyg@chromium.org> Commit-Queue: Leon Han <leon.han@intel.com> Cr-Commit-Position: refs/heads/master@{#737290} -- wpt-commits: a1652e2faab5c92db0d3a2523ef9a94003e1e8f5 wpt-pr: 21157

Fix isLocal algorithm

3834de0

beaufortfrancois requested review from kenchris and zolkis and removed request for kenchris December 25, 2019 09:00

beaufortfrancois mentioned this pull request Dec 25, 2019

Add isLocal to NDEFRecord and NDEFRecordInit #491

Merged

leonhsl reviewed Dec 25, 2019

View reviewed changes

zolkis reviewed Dec 25, 2019

View reviewed changes

zolkis merged commit 5a3d580 into w3c:gh-pages Dec 26, 2019

chromium-wpt-export-bot mentioned this pull request Jan 14, 2020

[webnfc] Support writing/reading local type records web-platform-tests/wpt#21157

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix isLocal algorithm #493

Fix isLocal algorithm #493

beaufortfrancois commented Dec 25, 2019 •

edited by pr-preview bot

leonhsl Dec 25, 2019

zolkis Dec 25, 2019

zolkis Dec 25, 2019

zolkis Dec 25, 2019

leonhsl Dec 26, 2019

zolkis Dec 26, 2019

zolkis Dec 25, 2019

zolkis Dec 25, 2019

zolkis commented Dec 26, 2019

Fix isLocal algorithm #493

Fix isLocal algorithm #493

Conversation

beaufortfrancois commented Dec 25, 2019 • edited by pr-preview bot

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

zolkis commented Dec 26, 2019

beaufortfrancois commented Dec 25, 2019 •

edited by pr-preview bot