-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #470: remove adding effective domain #474
Conversation
index.html
Outdated
<a data-cite="url#concept-url-serializer">serialization</a> of the | ||
<a data-cite="html">effective domain</a> of the {{Document/origin}}. | ||
If |type| does not match the `custom-type` definition from the | ||
<a>external type name ABNF</a>, return `false`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we improve this text? "match ABNF" stinks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, its basically ASCII without whitespaces and a few extra special chars. You can write an algorithm.
- Is ASCII
- has whitespaces
- Contains other non-valid characters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And no control characters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually they accept a very weird selection!
0x24 $
0x27 '
0x28 (
0x29 )
0x2A *
0x2B +
0x2C ,
0x2D -
0x2E .
0x3B ;
0x3D =
0x40 @
0x5F _
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URL spec uses wording like
Otherwise, if byte is 0x25 (%) and the next two bytes after byte in input are not in the ranges 0x30 (0) to 0x39 (9), 0x41 (A) to 0x46 (F), and 0x61 (a) to 0x66 (f), all inclusive, append byte to output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and
The URL code points are ASCII alphanumeric, U+0021 (!), U+0024 ($), U+0026 (&), U+0027 ('), U+0028 LEFT PARENTHESIS, U+0029 RIGHT PARENTHESIS, U+002A (*), U+002B (+), U+002C (,), U+002D (-), U+002E (.), U+002F (/), U+003A (:), U+003B (;), U+003D (=), U+003F (?), U+0040 (@), U+005F (_), U+007E (~), and code points in the range U+00A0 to U+10FFFD, inclusive, excluding surrogates and noncharacters.
That can be used for description :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type code points are ASCII alphanumeric, U+0024 ($), U+0027 ('), U+0028 LEFT PARENTHESIS, U+0029 RIGHT PARENTHESIS, U+002A (*), U+002B (+), U+002C (,), U+002D (-), U+002E (.), U+003B (;), U+003D (=), U+0040 (@), U+005F (_).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated (amended) the steps, please check again.
7185fec
to
ca5af2d
Compare
ca5af2d
to
46c9bc7
Compare
index.html
Outdated
In addition to [=well known type names=] it is also possible for | ||
organizations to create a own custom <dfn>external type name</dfn> that | ||
which is a string consisting of a domain name and a custom ASCII name, | ||
separated by a colon (`':'`). The string may not contain any whitespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my other comment is probably better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the place we specify the algorithm, and we should avoid repeating that complex thing. So either we say "satisfies the validate external type name steps", or we give a short summary in words.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to [=well known type names=] it is also possible for organizations to create a own custom external type name which is a string consisting of a domain name and a developer created type.
The developer created type code points are ASCII alphanumeric, U+0024 ($), U+0027 ('), U+0028 LEFT PARENTHESIS, U+0029 RIGHT PARENTHESIS, U+002A (*), U+002B (+), U+002C (,), U+002D (-), U+002E (.), U+003B (;), U+003D (=), U+0040 (@), U+005F (_).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems fine and aligned with other specs.
The NFC spec made it complex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep that enumeration in the validate external type steps only, and refer to them from here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URL specs uses U+00** when describing and 0x** when used in algos. I thikn this is fine. It is a spec, not documentation :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I took another attempt on it (amend).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the validation specific part.
46c9bc7
to
1c6461d
Compare
1c6461d
to
78d776c
Compare
index.html
Outdated
<a data-cite="url#concept-url-serializer">serialization</a> of the | ||
<a data-cite="html">effective domain</a> of the {{Document/origin}}. | ||
If |type| contains code points that are not [=ASCII alphanumeric=], or | ||
`U+0024` (`$`), `U+0027` (`'`), `U+0028` (`(`), `U+0029` (`)`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interestingly URL specs writes out (() as LEFT PARENTESIS etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably follow for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Strange. Should we use the same? (outsider reader might be confused why)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can say U+0029 RIGHT PARENTHESIS ())
- that is allowed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I agree that it is ugly and that U+0029 RIGHT PARENTHESIS
seems fine. We will link code points to INFRA spec anyway so we are good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely need to follow INFRA spec... everyone is moving to do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
index.html
Outdated
If not found, return `false`. | ||
</li> | ||
<li> | ||
Let |domain| be the part of |input| preceding the last colon, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cannot refer to colon here. Need to store the index above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let colonIndex be the last ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's still pseudocode, and it's not more clear than before (when we just want to say use the last colon), but fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
index.html
Outdated
organizations to create a own custom <dfn>external type name</dfn> | ||
which is a string consisting of a domain name and a custom ASCII name, | ||
separated by a colon (`':'`) that satisfies the | ||
<a>validate external type</a> steps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it makes more sense to just write it out here instead of the satisfied wording
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
index.html
Outdated
In addition to [=well known type names=] it is also possible for | ||
organizations to create a own custom <dfn>external type name</dfn> | ||
which is a string consisting of a domain name and a custom ASCII name, | ||
separated by a colon (`':'`) that satisfies the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we write the U+00 number here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed everywhere.
a467abf
to
3373cc3
Compare
index.html
Outdated
organizations to create a own custom <dfn>external type name</dfn> | ||
which is a string consisting of a domain name and a custom ASCII name, | ||
separated by a colon (`':'`) that satisfies the | ||
<a>validate external type</a> steps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
index.html
Outdated
</dl> | ||
<p> | ||
In addition to [=well known type names=] it is also possible for | ||
organizations to create a own custom <dfn>external type name</dfn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove own
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! Actually I removed it recently, strange.
index.html
Outdated
Validating domain name needs an algorithm. | ||
</p> | ||
Let |colonIndex| be the last occurrence of `U+003A` (`:`) in |input|. | ||
If a `U+003A` (`:`) is not found, return `false`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Shall we swap the order of those two sentences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about that, but the way you write code, it's an error/exception handling. If I put it to the front, it's double pass if someone takes the steps literally. Maybe there is a better formulation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we could do like url spec:
If bytes contains a 0x3D (=), then let name be the bytes from the start of bytes up to but excluding its first 0x3D (=), and let value be the bytes, if any, after the first 0x3D (=) up to the end of bytes. If 0x3D (=) is the first byte, then name will be the empty byte sequence. If it is the last, then value will be the empty byte sequence.
If |input| contains a U+003A
(:
), then let |domain| be the |input| from the start of |input| up to but excluding its first U+003A
(:
), and let |type| be the |input|, if any, after the first U+003A
(:
) up to the end of |input|.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If |input| does not contains a U+003A (:), return false
Let |domain| be the |input| from the start of |input| up to but excluding its first U+003A (:), and let |type| be the |input|, if any, after the first U+003A (:) up to the end of |input|.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except that it's the last occurrence of U+003A
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then need to check if |domain| and |type| are defined, i.e. in the steps above we need to include "or otherwise null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it last though? We allow URLs but the scheme and port etc is not written down.
so we accept people to write "https://intel.com:8080:coolio" or actually "intel.com:coolio" - in the latter one there is only one : ever
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If |input| does not contains a U+003A (:), return false
Let |domain| be the |input| from the start of |input| up to but excluding its last U+003A (:), and let |type| be the |input|, if any, after the last U+003A (:) up to the end of |input|.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs another round now. The sentence above doesn't handle the case when a colon is not found.
index.html
Outdated
<a data-cite="url#concept-url-serializer">serialization</a> of the | ||
<a data-cite="html">effective domain</a> of the {{Document/origin}}. | ||
If |type| contains code points that are not [=ASCII alphanumeric=], or | ||
`U+0024` (`$`), `U+0027` (`'`), `U+0028` (`(`), `U+0029` (`)`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
3373cc3
to
87f5311
Compare
Sorry for all the comments, this is getting there |
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
23cd28b
to
3bd2204
Compare
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
The spec change: w3c/web-nfc#474 BUG=520391 Change-Id: If1e4a6513410f3705bec4da7b2cc514c209a3cd9
The spec change: w3c/web-nfc#474 BUG=520391 Change-Id: If1e4a6513410f3705bec4da7b2cc514c209a3cd9
The spec change: w3c/web-nfc#474 BUG=520391 Change-Id: If1e4a6513410f3705bec4da7b2cc514c209a3cd9
The spec change: w3c/web-nfc#474 BUG=520391 Change-Id: If1e4a6513410f3705bec4da7b2cc514c209a3cd9
The spec change: w3c/web-nfc#474 BUG=520391 Change-Id: If1e4a6513410f3705bec4da7b2cc514c209a3cd9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1978622 Commit-Queue: Leon Han <leon.han@intel.com> Reviewed-by: Rijubrata Bhaumik <rijubrata.bhaumik@intel.com> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: François Beaufort <beaufort.francois@gmail.com> Cr-Commit-Position: refs/heads/master@{#727853}
The spec change: w3c/web-nfc#474 BUG=520391 Change-Id: If1e4a6513410f3705bec4da7b2cc514c209a3cd9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1978622 Commit-Queue: Leon Han <leon.han@intel.com> Reviewed-by: Rijubrata Bhaumik <rijubrata.bhaumik@intel.com> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: François Beaufort <beaufort.francois@gmail.com> Cr-Commit-Position: refs/heads/master@{#727853}
…ng external types, a=testonly Automatic update from web-platform-tests [webnfc] Implement the algo for validating external types The spec change: w3c/web-nfc#474 BUG=520391 Change-Id: If1e4a6513410f3705bec4da7b2cc514c209a3cd9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1978622 Commit-Queue: Leon Han <leon.han@intel.com> Reviewed-by: Rijubrata Bhaumik <rijubrata.bhaumik@intel.com> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: François Beaufort <beaufort.francois@gmail.com> Cr-Commit-Position: refs/heads/master@{#727853} -- wpt-commits: 74efe51f1faaa2bf3ff91c10f05a4ad9615a1553 wpt-pr: 20885
…ng external types, a=testonly Automatic update from web-platform-tests [webnfc] Implement the algo for validating external types The spec change: w3c/web-nfc#474 BUG=520391 Change-Id: If1e4a6513410f3705bec4da7b2cc514c209a3cd9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1978622 Commit-Queue: Leon Han <leon.han@intel.com> Reviewed-by: Rijubrata Bhaumik <rijubrata.bhaumik@intel.com> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: François Beaufort <beaufort.francois@gmail.com> Cr-Commit-Position: refs/heads/master@{#727853} -- wpt-commits: 74efe51f1faaa2bf3ff91c10f05a4ad9615a1553 wpt-pr: 20885
Signed-off-by: Zoltan Kis zoltan.kis@intel.com
Preview | Diff