Add CustomElementState for Custom Elements v1

Add CustomElementState for Custom Elements v1

This patch adds CustomElementState[1] for Custom Elements v1.

In order for v0 and v1 to co-exist and avoid complexity, the state is
stored separately from V0CustomElementState.

They can be set independently from each other, except DCHECK to ensure
an element can be customized by only by one of them.

[1] https://dom.spec.whatwg.org/#concept-element-custom-element-state

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

[kojii, 2016-05-17 06:21:02]

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

[kojii, 2016-05-17 06:21:03]

I tried to share the "defined/customized" flag for v0/v1, but it introduced more
complexity than imagined to properly invalidate :defined selector for v1 and
:unresolved selector for v0.

Since we still have some more bits available in NodeFlags, I went ahead to add 2
bits for v1 independently from the existing 2 bits for v0.

WDYT?

If we should try to not to consume NodeFlags, ideas are:
* Try to share between v0 and v1, probably can save 1 bit.
* v0 flags can be moved to ElementRareData.
I'm in mild preference to think this when we ran out of NodeFlags before we
deprecate v0?

[dominicc (has gone to gerrit), 2016-05-18 07:56:22]

Using more node flags is fine. We can do compaction later if we need to recover
flags. In all likelihood V0 will be removed before we need more flags anyway.

Some comments inline.

https://codereview.chromium.org/1982973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/1982973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.h:377: bool isDefinedCustomElement()
const { return getCustomElementState() != CustomElementState::Undefined; }
I wouldn't consider <button> to be a defined *custom* element; does the spec
have something to say about this?

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

https://codereview.chromium.org/1982973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:2320:
DCHECK(CustomElementState::Undefined == oldState);
When upgrading, don't you go from undefined -> customized?

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

https://codereview.chromium.org/1982973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NodeTest.cpp:45:
EXPECT_TRUE(div->isDefinedCustomElement());
I think in practice code needs to distinguish three or four states:

Not a custom element
A custom element that's not defined/upgraded (* not sure exactly what we need
here)
A custom element that's "done"

[kojii, 2016-05-18 08:32:15]

Thank you for your review. Replies inline:

https://codereview.chromium.org/1982973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/1982973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.h:377: bool isDefinedCustomElement()
const { return getCustomElementState() != CustomElementState::Undefined; }
On 2016/05/18 at 07:56:21, dominicc wrote:
> I wouldn't consider <button> to be a defined *custom* element; does the spec
have something to say about this?

Does "isDefinedElement()" look better?

The spec says:
| An element whose custom element state is "uncustomized" or "custom" is said to
be "defined"

isDefinedElement()
isDefinedCustomElementState()
isCustomElementStateDefined()

isDefinedElement() is probably more accurate spec-wise.

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

https://codereview.chromium.org/1982973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:2320:
DCHECK(CustomElementState::Undefined == oldState);
On 2016/05/18 at 07:56:21, dominicc wrote:
> When upgrading, don't you go from undefined -> customized?

Do you mean uncustomized -> customized?

From create an element https://dom.spec.whatwg.org/#concept-create-element
* If an element has a valid custom element name or has "is" attribute, it must
be set to "undefined" on its creation (7.3)
* If it's already defined, Construct (6.1.2) should set to "undefined" (7.3) and
then to "custom" (6.1.11)
so there should be no direct path from uncustomized -> customized IIUC, no?

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

https://codereview.chromium.org/1982973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NodeTest.cpp:45:
EXPECT_TRUE(div->isDefinedCustomElement());
On 2016/05/18 at 07:56:22, dominicc wrote:
> I think in practice code needs to distinguish three or four states:
> 
> Not a custom element
> A custom element that's not defined/upgraded (* not sure exactly what we need
here)
> A custom element that's "done"

When you need to distinguish the three, getCustomElementState() is what you
need:
Not a custom element -> uncustomized
A custom element that's not defined/upgraded -> undefined
A custom element that's "done" -> customized

The spec also defines a state group:
"uncustomized" or "custom" -> defined
so isDefinedCustomElement() is when I need to know whether it's defined or not.

...or did I misunderstand your comment? Could you elaborate if I look so?

[dominicc (has gone to gerrit), 2016-05-19 05:42:05]

lgtm

I understand now; what you had was good.

One nit inline about naming but I don't feel strongly about it.

I look forward to seeing :defined! You might find that you want to allow the 0
bits -> custom state transition for the code you'll be adding to
V8HTMLElementCustom/the constructor, but it's up to you.

https://codereview.chromium.org/1982973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.h (right):

https://codereview.chromium.org/1982973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.h:258: :
(getFlag(CustomElementCustomizedFlag) ? CustomElementState::Custom :
CustomElementState::Undefined);
I wonder if this should be CustomElementCustomFlag? There is Uncustomized, and
Custom; there is no Customized.

[kojii, 2016-05-19 05:43:59]

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

[kojii, 2016-05-19 05:52:35]

The CQ bit was checked by kojii@chromium.org

[kojii, 2016-05-19 05:52:36]

Thanks. Yeah, I might need to allow that transition, I'll add when I figure out
it's needed.

https://codereview.chromium.org/1982973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.h (right):

https://codereview.chromium.org/1982973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.h:258: :
(getFlag(CustomElementCustomizedFlag) ? CustomElementState::Custom :
CustomElementState::Undefined);
On 2016/05/19 at 05:42:05, dominicc wrote:
> I wonder if this should be CustomElementCustomFlag? There is Uncustomized, and
Custom; there is no Customized.

Done.

[kojii, 2016-05-19 05:52:36]

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/1982973002/#ps80001
(title: "Rename CustomElementCustomizedFlag to CustomElementCustomFlag")

[commit-bot: I haz the power, 2016-05-19 05:53:05]

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

[yosin_UTC9, 2016-05-19 06:00:35]

Let's introduce printer, operator<<(ostream&, CustomeElementState). This makes
us to ease of debugging and testing, e.g. DCHECK_EQ(), EXPECT_EQ() print
symbolic value instead of numeric.

https://codereview.chromium.org/1982973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/1982973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:2325: DCHECK(V0Upgraded !=
getV0CustomElementState());
nit: Use DCHECK_NE(getV0CustomElementState(), V0Upgraded);

https://codereview.chromium.org/1982973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:2330: DCHECK(newState ==
getCustomElementState());
Use DCHECK_EQ(newState, getCustomElementState());

https://codereview.chromium.org/1982973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:2356:
DCHECK(CustomElementState::Custom != getCustomElementState());
Use DCHECK_NE(CustomElementState::Custom, getCustomElementState())

https://codereview.chromium.org/1982973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.h (right):

https://codereview.chromium.org/1982973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.h:254: CustomElementState
getCustomElementState() const
This should be in "Node.cpp". This implementation doesn't fit inline function .h
file.

[kojii, 2016-05-19 06:07:22]

Unfortunately, DCHECK_OP families fail for enum class, so we need to live with
DCHECK or give up enum class to enum.

[kojii, 2016-05-19 06:08:59]

The CQ bit was unchecked by kojii@chromium.org

[kojii, 2016-05-19 06:52:15]

The CQ bit was checked by kojii@chromium.org

[kojii, 2016-05-19 06:52:15]

Done.

https://codereview.chromium.org/1982973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.h (right):

https://codereview.chromium.org/1982973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.h:254: CustomElementState
getCustomElementState() const
On 2016/05/19 at 06:00:35, Yosi_UTC9 wrote:
> This should be in "Node.cpp". This implementation doesn't fit inline function
.h file.

Done.

[kojii, 2016-05-19 06:52:16]

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/1982973002/#ps120001
(title: "operator<<(std::ostream&, CustomElementState)")

[commit-bot: I haz the power, 2016-05-19 06:52:42]

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

[commit-bot: I haz the power, 2016-05-19 07:12:56]

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

[commit-bot: I haz the power, 2016-05-19 07:12:57]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[kojii, 2016-05-19 07:18:50]

The CQ bit was checked by kojii@chromium.org

[kojii, 2016-05-19 07:18:51]

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/1982973002/#ps140001
(title: "build fix and UNREACHABLE -> NOTREACHED")

[commit-bot: I haz the power, 2016-05-19 07:19:08]

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

[commit-bot: I haz the power, 2016-05-19 11:11:26]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-19 11:12:23]

Description was changed from

==========
Add CustomElementState for Custom Elements v1

This patch adds CustomElementState[1] for Custom Elements v1.

In order for v0 and v1 to co-exist and avoid complexity, the state is
stored separately from V0CustomElementState.

They can be set independently from each other, except DCHECK to ensure
an element can be customized by only by one of them.

[1] https://dom.spec.whatwg.org/#concept-element-custom-element-state

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

to

==========
Add CustomElementState for Custom Elements v1

This patch adds CustomElementState[1] for Custom Elements v1.

In order for v0 and v1 to co-exist and avoid complexity, the state is
stored separately from V0CustomElementState.

They can be set independently from each other, except DCHECK to ensure
an element can be customized by only by one of them.

[1] https://dom.spec.whatwg.org/#concept-element-custom-element-state

BUG=594918

Committed: https://crrev.com/f15adc94e6c7c22940472feefcee2e59071fec0c
Cr-Commit-Position: refs/heads/master@{#394734}
==========

[commit-bot: I haz the power, 2016-05-19 11:12:24]

Patchset 8 (id:??) landed as
https://crrev.com/f15adc94e6c7c22940472feefcee2e59071fec0c
Cr-Commit-Position: refs/heads/master@{#394734}