Implement PotentialCustomElementName

Implement PotentialCustomElementName

This patch implements PotentialCustomElementName, which is part of
validating custom element name.

A valid custom element name must be PotentialCustomElementName and also
exclude all hyphen-containing element names. It will be in a following
patch.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#prod-potentialcustomelementname

BUG=594918
Committed: https://crrev.com/6b316d166bc70f33bd342a00236a8adda68994ef
Cr-Commit-Position: refs/heads/master@{#390039}

[kojii, 2016-04-27 01:15:43]

Description was changed from

==========
PCEN

BUG=
==========

to

==========
Implement PotentialCustomElementName

This patch implements PotentialCustomElementName, which is part of
validating custom element name.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#prod-potentialcustomele...

BUG=594918
==========

[kojii, 2016-04-27 01:16:00]

kojii@chromium.org changed reviewers:
+ dominicc@chromium.org, hayato@chromium.org

[kojii, 2016-04-27 01:16:01]

PTAL.

[dominicc (has gone to gerrit), 2016-04-27 01:41:29]

On 2016/04/27 at 01:16:01, kojii wrote:
> PTAL.

Nice work, some comments inline.

[dominicc (has gone to gerrit), 2016-04-27 01:42:14]

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp (right):

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:23: if (ch == '-')
Maybe hasHyphens |= ch == '-' is more succinct?

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:28: return
hasHyphens;
This also needs to filter out

annotation-xml
color-profile
font-face
font-face-src
font-face-uri
font-face-format
font-face-name
missing-glyph

BTW, in the past some overzealous contributor (I think Deepak; Samsung) came
along and removed a bunch of these tests because the string constants were not
used elsewhere, and caused Custom Elements v0 to be too permissive. So I suggest
adding a comment saying even though Blink does not support these names (eg
MathML) elsewhere, we NEED to filter them here.

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp (right):

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:27:
EXPECT_TRUE(CustomElement::isPotentialCustomElementName(str)) << str;
WDYT about making this message more descriptive, eg 'x' should be a potential
Custom Element name character; ditto below?

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:48: { true,
"aa-a" },
WDYT about including some multibyte strings here?

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:57: struct {
Let's split these into two tests at this point; and put this vvv right next to
the helper function.

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:60: { '-', '.'
},
Bit subtle given how the spec treats these, but OK :)

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:63: { 0xB7, 0xB7
},
What about good old a-z? And how about an explicit test rejecting A-Z?

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:66: { 0xF8,
0x37D },
See my comment on CharacterPropertyDataGenerator.h

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/text/Character.h (right):

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/Character.h:83: static bool
isPotentialCustomElementNameChar(UChar32 character);
This might be a good place to put a link to the spec.

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/text/CharacterPropertyData.cpp (left):

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/CharacterPropertyData.cpp:5: int32_t
serializedCharacterDataSize = 10288;
I had no idea these tables existed. This is cool. TIL

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/text/CharacterPropertyDataGenerator.h
(right):

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/CharacterPropertyDataGenerator.h:245:
0xF8, 0x37D,
Today the spec says 0xF8-0x2FF, 0x300-0x37D. If there's some subtlety here it's
worth a comment.

[kojii, 2016-04-27 02:35:38]

Description was changed from

==========
Implement PotentialCustomElementName

This patch implements PotentialCustomElementName, which is part of
validating custom element name.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#prod-potentialcustomele...

BUG=594918
==========

to

==========
Implement PotentialCustomElementName

This patch implements PotentialCustomElementName, which is part of
validating custom element name.

A valid custom element name must be PotentialCustomElementName and also
exclude all hyphen-containing element names. It will be in a following
patch.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#prod-potentialcustomele...

BUG=594918
==========

[kojii, 2016-04-27 03:52:14]

Thank you for the prompt review. Replies inline.

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp (right):

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:23: if (ch == '-')
On 2016/04/27 at 01:42:14, dominicc wrote:
> Maybe hasHyphens |= ch == '-' is more succinct?

We don't have to call Character::isPotentialCustomElementNameChar() if '-', so
this saves a bit of cycles.

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:28: return
hasHyphens;
On 2016/04/27 at 01:42:14, dominicc wrote:
> This also needs to filter out
> 
> annotation-xml
> color-profile
> font-face
> font-face-src
> font-face-uri
> font-face-format
> font-face-name
> missing-glyph
> 
> BTW, in the past some overzealous contributor (I think Deepak; Samsung) came
along and removed a bunch of these tests because the string constants were not
used elsewhere, and caused Custom Elements v0 to be too permissive. So I suggest
adding a comment saying even though Blink does not support these names (eg
MathML) elsewhere, we NEED to filter them here.

Agreed. If I read the spec correctly,
  valid custom element name = PotentialCustomElementName && exclude-them
https://html.spec.whatwg.org/multipage/scripting.html#prod-potentialcustomele...

so having another method (e.g., isValidName) looks better to me. Does this look
reasonable? I've updated the description.

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp (right):

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:27:
EXPECT_TRUE(CustomElement::isPotentialCustomElementName(str)) << str;
On 2016/04/27 at 01:42:14, dominicc wrote:
> WDYT about making this message more descriptive, eg 'x' should be a potential
Custom Element name character; ditto below?

Done, thanks for the suggestion.

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:48: { true,
"aa-a" },
On 2016/04/27 at 01:42:14, dominicc wrote:
> WDYT about including some multibyte strings here?

Done.

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:57: struct {
On 2016/04/27 at 01:42:14, dominicc wrote:
> Let's split these into two tests at this point; and put this vvv right next to
the helper function.

Done.

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:63: { 0xB7, 0xB7
},
On 2016/04/27 at 01:42:14, dominicc wrote:
> What about good old a-z? And how about an explicit test rejecting A-Z?

Done.

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:66: { 0xF8,
0x37D },
On 2016/04/27 at 01:42:14, dominicc wrote:
> See my comment on CharacterPropertyDataGenerator.h

Done, added comments.

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/text/Character.h (right):

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/Character.h:83: static bool
isPotentialCustomElementNameChar(UChar32 character);
On 2016/04/27 at 01:42:14, dominicc wrote:
> This might be a good place to put a link to the spec.

Done.

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/text/CharacterPropertyData.cpp (left):

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/CharacterPropertyData.cpp:5: int32_t
serializedCharacterDataSize = 10288;
On 2016/04/27 at 01:42:14, dominicc wrote:
> I had no idea these tables existed. This is cool. TIL

;-) FYI http://crbug.com/571943

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/text/CharacterPropertyDataGenerator.h
(right):

https://codereview.chromium.org/1916013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/CharacterPropertyDataGenerator.h:245:
0xF8, 0x37D,
On 2016/04/27 at 01:42:14, dominicc wrote:
> Today the spec says 0xF8-0x2FF, 0x300-0x37D. If there's some subtlety here
it's worth a comment.

This is build-time data (except some embedders run once at run-time) so I
changed to follow the spec.

[dominicc (has gone to gerrit), 2016-04-27 05:29:35]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-04-27 05:29:35]

lgtm

Will we ever call isPotentialCustomElementName and *not*
isValidCustomElementName? I think we probably want these together. But this LGTM
for now.

[commit-bot: I haz the power, 2016-04-27 05:29:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1916013004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1916013004/40001

[commit-bot: I haz the power, 2016-04-27 05:40:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 05:40:18]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kojii, 2016-04-27 08:11:55]

kojii@chromium.org changed reviewers:
+ tkent@chromium.org

[kojii, 2016-04-27 08:11:55]

tkent@, PTAL for platform

[tkent, 2016-04-27 08:21:18]

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/core.gypi (right):

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/core.gypi:2670: 'dom/custom/CustomElement.cpp',
File list in dom/custom/ isn't sorted though it wasn't caused by this CL.

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp (right):

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:7: #include
"platform/Logging.h"
Please don't use platform/Logging.h.

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:12: static void
TestIsPotentialCustomElementName(const AtomicString& str, bool expected)
Function name should start with lower case latter in the current coding style.

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:23: static void
TestIsPotentialCustomElementNameChar(UChar32 c, bool expected)
Ditto.

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:69: 
This blank line looks unnecessary.

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:101: 
This blank line looks unnecessary.

[kojii, 2016-04-27 08:45:21]

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/core.gypi (right):

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/core.gypi:2670: 'dom/custom/CustomElement.cpp',
On 2016/04/27 at 08:21:18, tkent wrote:
> File list in dom/custom/ isn't sorted though it wasn't caused by this CL.

Done.

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp (right):

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:7: #include
"platform/Logging.h"
On 2016/04/27 at 08:21:18, tkent wrote:
> Please don't use platform/Logging.h.

Done, sorry, copied from somewhere.

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:12: static void
TestIsPotentialCustomElementName(const AtomicString& str, bool expected)
On 2016/04/27 at 08:21:18, tkent wrote:
> Function name should start with lower case latter in the current coding style.

Done, confused with test names, sorry.

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:23: static void
TestIsPotentialCustomElementNameChar(UChar32 c, bool expected)
On 2016/04/27 at 08:21:18, tkent wrote:
> Ditto.

Done.

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:69: 
On 2016/04/27 at 08:21:18, tkent wrote:
> This blank line looks unnecessary.

Done.

https://codereview.chromium.org/1916013004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:101: 
On 2016/04/27 at 08:21:18, tkent wrote:
> This blank line looks unnecessary.

Done.

[tkent, 2016-04-27 08:46:36]

lgtm

[kojii, 2016-04-27 10:14:49]

The CQ bit was checked by kojii@chromium.org

[kojii, 2016-04-27 10:14:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominicc@chromium.org
Link to the patchset: https://codereview.chromium.org/1916013004/#ps60001
(title: "tkent review")

[commit-bot: I haz the power, 2016-04-27 10:15:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1916013004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1916013004/60001

[commit-bot: I haz the power, 2016-04-27 10:23:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-27 10:24:17]

Description was changed from

==========
Implement PotentialCustomElementName

This patch implements PotentialCustomElementName, which is part of
validating custom element name.

A valid custom element name must be PotentialCustomElementName and also
exclude all hyphen-containing element names. It will be in a following
patch.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#prod-potentialcustomele...

BUG=594918
==========

to

==========
Implement PotentialCustomElementName

This patch implements PotentialCustomElementName, which is part of
validating custom element name.

A valid custom element name must be PotentialCustomElementName and also
exclude all hyphen-containing element names. It will be in a following
patch.

Spec:
https://html.spec.whatwg.org/multipage/scripting.html#prod-potentialcustomele...

BUG=594918

Committed: https://crrev.com/6b316d166bc70f33bd342a00236a8adda68994ef
Cr-Commit-Position: refs/heads/master@{#390039}
==========

[commit-bot: I haz the power, 2016-04-27 10:24:18]

Patchset 4 (id:??) landed as
https://crrev.com/6b316d166bc70f33bd342a00236a8adda68994ef
Cr-Commit-Position: refs/heads/master@{#390039}