Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
493713002:
Invalid chars should not be allowed as first character in custom element name (Closed)
|
Created:
6 years, 4 months ago by deepak.sa Modified:
6 years, 1 month ago CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, webcomponents-bugzilla_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionInvalid chars should not be allowed as first character in custom element name
This patch *disallows* usage of remaining combining chars as first
character in custom element name. Also, usage of colon (":") is not
allowed in custom element name. This restriction is according to the
NCName production mentioned in the specs.
BUG=325673
TEST=fast/dom/custom/invalid-first-char-combinators.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184957
Patch Set 1 #
Total comments: 3
Patch Set 2 : Addrressing comments #
Total comments: 5
Messages
Total messages: 23 (2 generated)
PTAL? Thanks!
Could you review this? Thanks!
On 2014/08/21 07:37:19, deepak.sa wrote: > Could you review this? > Thanks! I'm unsure about this. It is not clear to me whether it is the Custom Elements spec or the implementation that needs to change.
On 2014/08/27 at 21:16:42, dominicc wrote: > On 2014/08/21 07:37:19, deepak.sa wrote: > > Could you review this? > > Thanks! > > I'm unsure about this. It is not clear to me whether it is the Custom Elements spec or the implementation that needs to change. The no-colon rule doesn't make sense, html doesn't have that restriction on element names. a:b-c:d should probably be a valid custom element name.
On 2014/08/27 21:20:16, esprehn wrote: > On 2014/08/27 at 21:16:42, dominicc wrote: > > On 2014/08/21 07:37:19, deepak.sa wrote: > > > Could you review this? > > > Thanks! > > > > I'm unsure about this. It is not clear to me whether it is the Custom Elements > spec or the implementation that needs to change. > > The no-colon rule doesn't make sense, html doesn't have that restriction on > element names. a:b-c:d should probably be a valid custom element name. May we need to update the custom elements spec then. Valid name for custom element is defined here : http://w3c.github.io/webcomponents/spec/custom/#dfn-custom-element-type
On 2014/08/28 04:43:46, deepak.sa wrote: > On 2014/08/27 21:20:16, esprehn wrote: > > On 2014/08/27 at 21:16:42, dominicc wrote: > > > On 2014/08/21 07:37:19, deepak.sa wrote: > > > > Could you review this? > > > > Thanks! > > > > > > I'm unsure about this. It is not clear to me whether it is the Custom > Elements > > spec or the implementation that needs to change. > > > > The no-colon rule doesn't make sense, html doesn't have that restriction on > > element names. a:b-c:d should probably be a valid custom element name. > > May we need to update the custom elements spec then. > Valid name for custom element is defined here : > http://w3c.github.io/webcomponents/spec/custom/#dfn-custom-element-type Is this patch of continued interest or should we close it?
On 2014/09/11 06:20:35, dominicc wrote: > On 2014/08/28 04:43:46, deepak.sa wrote: > > On 2014/08/27 21:20:16, esprehn wrote: > > > On 2014/08/27 at 21:16:42, dominicc wrote: > > > > On 2014/08/21 07:37:19, deepak.sa wrote: > > > > > Could you review this? > > > > > Thanks! > > > > > > > > I'm unsure about this. It is not clear to me whether it is the Custom > > Elements > > > spec or the implementation that needs to change. > > > > > > The no-colon rule doesn't make sense, html doesn't have that restriction on > > > element names. a:b-c:d should probably be a valid custom element name. > > > > May we need to update the custom elements spec then. > > Valid name for custom element is defined here : > > http://w3c.github.io/webcomponents/spec/custom/#dfn-custom-element-type > > Is this patch of continued interest or should we close it? Yes, we are waiting for the response of dglazkov, on the bug. It is yet to decide whether spec is right about the "NCName" production or not? I have listed the w3 bug which had asked for the NCName production. For reference, i am also listing it here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=20973
deepak.sa@samsung.com changed reviewers: + dglazkov@chromium.org
+dglazkov Could you review this? Thanks!
I think NCName is okay. https://codereview.chromium.org/493713002/diff/1/Source/core/dom/custom/Custo... File Source/core/dom/custom/CustomElement.cpp (right): https://codereview.chromium.org/493713002/diff/1/Source/core/dom/custom/Custo... Source/core/dom/custom/CustomElement.cpp:91: if (c == 0x0B83 || c == 0x0F88 || c == 0x0F89 || c == 0x0F8A || c == 0x0F8B) This set of character comparisons seems out of place. Should it not be the same as Document::isValidName? There's a tricky balance between what spec says and what Blink implements in Document::isValidName. Do you know if there's a difference there?
On 2014/10/07 15:13:13, dglazkov wrote: > I think NCName is okay. > > https://codereview.chromium.org/493713002/diff/1/Source/core/dom/custom/Custo... > File Source/core/dom/custom/CustomElement.cpp (right): > > https://codereview.chromium.org/493713002/diff/1/Source/core/dom/custom/Custo... > Source/core/dom/custom/CustomElement.cpp:91: if (c == 0x0B83 || c == 0x0F88 || c > == 0x0F89 || c == 0x0F8A || c == 0x0F8B) > This set of character comparisons seems out of place. Should it not be the same > as Document::isValidName? > > There's a tricky balance between what spec says and what Blink implements in > Document::isValidName. Do you know if there's a difference there? Thanks dglazkov for the review. Please have a look at this comment : https://code.google.com/p/chromium/issues/detail?id=325673#c9 I have tried to explain the difference there.
lgtm https://codereview.chromium.org/493713002/diff/1/Source/core/dom/custom/Custo... File Source/core/dom/custom/CustomElement.cpp (right): https://codereview.chromium.org/493713002/diff/1/Source/core/dom/custom/Custo... Source/core/dom/custom/CustomElement.cpp:74: if ((validNames & StandardNames) && kNotFound != name.find('-') && kNotFound == name.find(':')) { This change is good. https://codereview.chromium.org/493713002/diff/1/Source/core/dom/custom/Custo... Source/core/dom/custom/CustomElement.cpp:91: if (c == 0x0B83 || c == 0x0F88 || c == 0x0F89 || c == 0x0F8A || c == 0x0F8B) What I meant to say here is that you should not be stuffing character comparisons into this function. This function just wants to call another function that tells it whether the name is valid.
On 2014/10/16 17:41:26, dglazkov wrote: > lgtm > > https://codereview.chromium.org/493713002/diff/1/Source/core/dom/custom/Custo... > File Source/core/dom/custom/CustomElement.cpp (right): > > https://codereview.chromium.org/493713002/diff/1/Source/core/dom/custom/Custo... > Source/core/dom/custom/CustomElement.cpp:74: if ((validNames & StandardNames) && > kNotFound != name.find('-') && kNotFound == name.find(':')) { > This change is good. > > https://codereview.chromium.org/493713002/diff/1/Source/core/dom/custom/Custo... > Source/core/dom/custom/CustomElement.cpp:91: if (c == 0x0B83 || c == 0x0F88 || c > == 0x0F89 || c == 0x0F8A || c == 0x0F8B) > What I meant to say here is that you should not be stuffing character > comparisons into this function. This function just wants to call another > function that tells it whether the name is valid.
On 2014/11/04 11:11:02, deepak.sa wrote: > On 2014/10/16 17:41:26, dglazkov wrote: > > lgtm > > > > > https://codereview.chromium.org/493713002/diff/1/Source/core/dom/custom/Custo... > > File Source/core/dom/custom/CustomElement.cpp (right): > > > > > https://codereview.chromium.org/493713002/diff/1/Source/core/dom/custom/Custo... > > Source/core/dom/custom/CustomElement.cpp:74: if ((validNames & StandardNames) > && > > kNotFound != name.find('-') && kNotFound == name.find(':')) { > > This change is good. > > > > > https://codereview.chromium.org/493713002/diff/1/Source/core/dom/custom/Custo... > > Source/core/dom/custom/CustomElement.cpp:91: if (c == 0x0B83 || c == 0x0F88 || > c > > == 0x0F89 || c == 0x0F8A || c == 0x0F8B) > > What I meant to say here is that you should not be stuffing character > > comparisons into this function. This function just wants to call another > > function that tells it whether the name is valid.
Ignore above msgs. They are sent due to some issue with my system. Sorry for the spam! Sorry for the late response as I was out of office for few weeks. Please have another look. Thanks!
Ping!
https://codereview.chromium.org/493713002/diff/20001/LayoutTests/fast/dom/cus... File LayoutTests/fast/dom/custom/element-names.html (right): https://codereview.chromium.org/493713002/diff/20001/LayoutTests/fast/dom/cus... LayoutTests/fast/dom/custom/element-names.html:20: 'annotation-xml', What about test expectations? https://codereview.chromium.org/493713002/diff/20001/LayoutTests/fast/dom/cus... File LayoutTests/fast/dom/custom/invalid-first-char-combinators.html (right): https://codereview.chromium.org/493713002/diff/20001/LayoutTests/fast/dom/cus... LayoutTests/fast/dom/custom/invalid-first-char-combinators.html:17: }, 'registering invalid first letter combinators, not covered in Document::validNames'); and here too?
Thanks for the quick review! Comments inline. https://codereview.chromium.org/493713002/diff/20001/LayoutTests/fast/dom/cus... File LayoutTests/fast/dom/custom/element-names.html (right): https://codereview.chromium.org/493713002/diff/20001/LayoutTests/fast/dom/cus... LayoutTests/fast/dom/custom/element-names.html:20: 'annotation-xml', On 2014/11/06 05:52:43, dglazkov wrote: > What about test expectations? Test expectation file for this file has been removed earlier. CL: https://codereview.chromium.org/332583002/ This is done inorder to prevent file comparison when test runner can directly check the output. https://codereview.chromium.org/493713002/diff/20001/LayoutTests/fast/dom/cus... File LayoutTests/fast/dom/custom/invalid-first-char-combinators.html (right): https://codereview.chromium.org/493713002/diff/20001/LayoutTests/fast/dom/cus... LayoutTests/fast/dom/custom/invalid-first-char-combinators.html:17: }, 'registering invalid first letter combinators, not covered in Document::validNames'); On 2014/11/06 05:52:43, dglazkov wrote: > and here too? As the expected files for such test cases has been removed earlier, *-expected file is not added here. Reference CL: https://codereview.chromium.org/332583002/
https://codereview.chromium.org/493713002/diff/20001/LayoutTests/fast/dom/cus... File LayoutTests/fast/dom/custom/element-names.html (right): https://codereview.chromium.org/493713002/diff/20001/LayoutTests/fast/dom/cus... LayoutTests/fast/dom/custom/element-names.html:20: 'annotation-xml', On 2014/11/06 06:20:02, deepak.sa wrote: > On 2014/11/06 05:52:43, dglazkov wrote: > > What about test expectations? > > Test expectation file for this file has been removed earlier. > CL: https://codereview.chromium.org/332583002/ > > This is done inorder to prevent file comparison when test runner can directly > check the output. CL in which test expectations were removed : https://codereview.chromium.org/328243002
lgtm
The CQ bit was checked by deepak.sa@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/493713002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as 184957 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
