Custom Elements: HTMLElement constructor tests

Custom Elements: HTMLElement constructor tests

1. Removed constructor checks from define-element.html and add them in
construct.html
2. Constructing a custom element with new should change the state
to "custom".
3. Creating or upgrading an element that is an already constructed
marker should throw "InvalidStateError"DOMException.
However, when an element is constructed with new, the construction
stack is empty the whole time, thus it will not throw
"InvalidStateError".

BUG=643052
Committed: https://crrev.com/6147d4d4ffe38e7b0d292677e9c309b5b3509dab
Cr-Commit-Position: refs/heads/master@{#422031}

[davaajav, 2016-09-07 10:29:57]

davaajav@google.com changed reviewers:
+ dominicc@chromium.org, kojii@chromium.org

[davaajav, 2016-09-07 10:29:57]

PTL

[dominicc (has gone to gerrit), 2016-09-07 16:18:12]

On 2016/09/07 at 10:29:57, davaajav wrote:
> PTL

It's great you're cleaning up define-element.html. A bunch of tests got lumped
together in there.

OK, before anything else, can you check the description for spelling? I noticed
we committed something with customelElements in the description yesterday. Also
write 'custom elements' with a space; don't slam the words together like that.
Let me know when you have done that and I'll start the review.

[davaajav, 2016-09-08 02:01:46]

Description was changed from

==========
CustomElements: HTMLElement constructor tests

1. Removed constructor checks from define-element.html and add them in
construct.html
2. Consturcting a custom element with new should change the state
to "custom".
3. Constructing an element that an already constructed marker should
throw "InvalidStateError"DOMException.
Currently, when constructing an element with new, it never reaches
Step 9, where already constructed marker is checked. Hence, the
state of the element with a faulty constructor, but constructed
with new, remains "custom".

BUG=643052
==========

to

==========
Custom Elements: HTMLElement constructor tests

1. Removed constructor checks from define-element.html and add them in
construct.html
2. Constructing a custom element with new should change the state
to "custom".
3. Constructing an element that is an already constructed marker should
throw "InvalidStateError"DOMException.
Currently, when constructing an element with new, it never reaches
Step 9, where already constructed marker is checked. Hence, the
state of the element with a faulty constructor, but constructed
with new, remains "custom".

BUG=643052
==========

[davaajav, 2016-09-08 02:03:29]

PTL 

Oops, Indeed we have committed something like that, fixed that. Also, fixed
spell errors in the description.

[dominicc (has gone to gerrit), 2016-09-08 04:13:22]

Excellent work. One question about one of the tests inline, I think one test may
be wrong.

https://codereview.chromium.org/2320553002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/custom-elements/spec/construct.html (right):

https://codereview.chromium.org/2320553002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/custom-elements/spec/construct.html:108: },
'Already constructed marker, construct with new');
Can you point to the key points in the spec that mean this should throw? I don't
think this should throw, because new/super may *read* from the construction
stack, but it never *writes* to the construction stack. So new within new, the
construction stack is empty the whole time, I believe this is meant to just
work?

[davaajav, 2016-09-08 05:00:38]

> Can you point to the key points in the spec that mean this should throw? I
don't think this should throw, because new/super may *read* from the
construction stack, but it never *writes* to the construction stack. So new
within new, the construction stack is empty the whole time, I believe this is
meant to just work?

Ah, I see. If you follow the spec it would not throw and the state is "custom".
I was wondering whether it was the intended behavior not to throw for new. I
will change the test and delete the failure expectation.

[davaajav, 2016-09-08 05:18:42]

Description was changed from

==========
Custom Elements: HTMLElement constructor tests

1. Removed constructor checks from define-element.html and add them in
construct.html
2. Constructing a custom element with new should change the state
to "custom".
3. Constructing an element that is an already constructed marker should
throw "InvalidStateError"DOMException.
Currently, when constructing an element with new, it never reaches
Step 9, where already constructed marker is checked. Hence, the
state of the element with a faulty constructor, but constructed
with new, remains "custom".

BUG=643052
==========

to

==========
Custom Elements: HTMLElement constructor tests

1. Removed constructor checks from define-element.html and add them in
construct.html
2. Constructing a custom element with new should change the state
to "custom".
3. Creating or upgrading an element that is an already constructed 
marker should throw "InvalidStateError"DOMException.
However, when an element is constructed with new, the construction
stack is empty the whole time, thus it will not throw
"InvalidStateError".

BUG=643052
==========

[dominicc (has gone to gerrit), 2016-09-12 05:36:06]

On 2016/09/08 at 05:00:38, davaajav wrote:
> > Can you point to the key points in the spec that mean this should throw? I
don't think this should throw, because new/super may *read* from the
construction stack, but it never *writes* to the construction stack. So new
within new, the construction stack is empty the whole time, I believe this is
meant to just work?
> 
> Ah, I see. If you follow the spec it would not throw and the state is
"custom". I was wondering whether it was the intended behavior not to throw for
new. I will change the test and delete the failure expectation.

I think the spec is this way because for 'new' the only time we can set the
custom element state is in super(). Imagine you're implementing a JavaScript
virtual machine like V8; you don't want to add a hook that says "I finished
running a constructor" because that would be slow. So the only time we get to
act on custom elements created with 'new' is in how they're defined and when
super runs. That's why it is this way.

createElement, parsing, etc. are different because we're the ones creating the
element. So then our code continues to run and we can set a more precise state
like failed, etc.

Is this ready for another review now?

[davaajav, 2016-09-12 05:58:09]

> Is this ready for another review now?

 Yes, please take a look.

[dominicc (has gone to gerrit), 2016-09-13 08:27:32]

Splendid, LGTM. Please fix the one long line and then this is set to commit!

https://codereview.chromium.org/2320553002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/custom-elements/spec/construct.html (right):

https://codereview.chromium.org/2320553002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/custom-elements/spec/construct.html:79:
test_with_window((w) => {
This is a good test. We pass this "by accident" now because HTMLButtonElement
constructor *always* throws a TypeError. It is good to have the test though!

https://codereview.chromium.org/2320553002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/custom-elements/spec/construct.html:147:
assert_array_equals(errors, ['InvalidStateError'], 'Upgrading an element that is
already constructed marker should throw InvalidStateError');
Try to stick to lines less than 80 cols; maybe use + to break the long string
literal up.

[davaajav, 2016-09-15 02:14:31]

PTL

Wrapped up the long string literal.

[davaajav, 2016-09-29 09:54:40]

The CQ bit was checked by davaajav@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 09:55:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 10:40:49]

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

[commit-bot: I haz the power, 2016-09-29 10:40:50]

Dry run: This issue passed the CQ dry run.

[dominicc (has gone to gerrit), 2016-09-30 04:45:50]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-09-30 04:45: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/2320553002/#ps100001
(title: "wraping up long stringl literal")

[commit-bot: I haz the power, 2016-09-30 04:45:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 04:51:06]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-30 04:52:34]

Description was changed from

==========
Custom Elements: HTMLElement constructor tests

1. Removed constructor checks from define-element.html and add them in
construct.html
2. Constructing a custom element with new should change the state
to "custom".
3. Creating or upgrading an element that is an already constructed 
marker should throw "InvalidStateError"DOMException.
However, when an element is constructed with new, the construction
stack is empty the whole time, thus it will not throw
"InvalidStateError".

BUG=643052
==========

to

==========
Custom Elements: HTMLElement constructor tests

1. Removed constructor checks from define-element.html and add them in
construct.html
2. Constructing a custom element with new should change the state
to "custom".
3. Creating or upgrading an element that is an already constructed
marker should throw "InvalidStateError"DOMException.
However, when an element is constructed with new, the construction
stack is empty the whole time, thus it will not throw
"InvalidStateError".

BUG=643052

Committed: https://crrev.com/6147d4d4ffe38e7b0d292677e9c309b5b3509dab
Cr-Commit-Position: refs/heads/master@{#422031}
==========

[commit-bot: I haz the power, 2016-09-30 04:52:35]

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