Prevent recursion in critical part of CustomElementRegistry::define.

Prevent recursion in critical part of CustomElementRegistry::define.

Custom element registry used to allow recursion, but this complicated
the check for whether a name was in use. Now define [1] has an simpler
explicit recursion guard called the "element definition is running
flag." [2]

[1] https://html.spec.whatwg.org/#dom-customelementregistry-define
[2] https://html.spec.whatwg.org/#element-definition-is-running

BUG=643051
Committed: https://crrev.com/fade7ae22a3ea448947c5c90cb1a2ac2b5c30781
Cr-Commit-Position: refs/heads/master@{#416460}

[dominicc (has gone to gerrit), 2016-09-02 07:23:42]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 07:23:53]

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

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

dominicc@chromium.org changed reviewers:
+ domenic@chromium.org, kochi@chromium.org

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

PTAL

[commit-bot: I haz the power, 2016-09-02 11:04:00]

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

[commit-bot: I haz the power, 2016-09-02 11:04:03]

Dry run: This issue passed the CQ dry run.

[domenic, 2016-09-02 16:19:29]

On 2016/09/02 at 07:24:12, dominicc wrote:
> PTAL

LGTM with some test nits and suggestions.

The pattern of storing the flag in CustomElementsRegistry and using a bool& to
modify it from ElementDefinitionIsRunning seems a bit weird to me, but I don't
do a lot of C++, so if that's the way to do things I'll just say that today I
learned something :)

[dominicc (has gone to gerrit), 2016-09-03 22:17:20]

On 2016/09/02 at 16:19:29, domenic wrote:
> On 2016/09/02 at 07:24:12, dominicc wrote:
> > PTAL
> 
> LGTM with some test nits and suggestions.

I can't see the feedback on the tests; could you hit publish again? Thanks!

> The pattern of storing the flag in CustomElementsRegistry and using a bool& to
modify it from ElementDefinitionIsRunning seems a bit weird to me, but I don't
do a lot of C++, so if that's the way to do things I'll just say that today I
learned something :)

[domenic, 2016-09-03 22:50:35]

https://codereview.chromium.org/2306923002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html
(right):

https://codereview.chromium.org/2306923002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:96:
w2.customElements.define('second-name', X);
This test is very interesting. Maybe also add a test that before doing
w2.customElements.define, new X() throws (since it will go to look up the name
in w2 and not find it)?

https://codereview.chromium.org/2306923002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:127:
return new Object();
Nit: `new Object()` is better written as `{}`

https://codereview.chromium.org/2306923002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:150:
'the first definition should have worked, too');
typo: second

https://codereview.chromium.org/2306923002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:167: },
'Element definition flag reset',
A more descriptive test name would be "Element definition flag resets before
upgrades happen" or similar.

[dominicc (has gone to gerrit), 2016-09-03 23:23:41]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-09-03 23:23:42]

The patchset sent to the CQ was uploaded after l-g-t-m from domenic@chromium.org
Link to the patchset: https://codereview.chromium.org/2306923002/#ps20001
(title: "Feedback")

[commit-bot: I haz the power, 2016-09-03 23:23:50]

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-04 01:01:16]

Committed patchset #2 (id:20001)

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

Description was changed from

==========
Prevent recursion in critical part of CustomElementRegistry::define.

Custom element registry used to allow recursion, but this complicated
the check for whether a name was in use. Now define [1] has an simpler
explicit recursion guard called the "element definition is running
flag." [2]

[1] https://html.spec.whatwg.org/#dom-customelementregistry-define
[2] https://html.spec.whatwg.org/#element-definition-is-running

BUG=643051
==========

to

==========
Prevent recursion in critical part of CustomElementRegistry::define.

Custom element registry used to allow recursion, but this complicated
the check for whether a name was in use. Now define [1] has an simpler
explicit recursion guard called the "element definition is running
flag." [2]

[1] https://html.spec.whatwg.org/#dom-customelementregistry-define
[2] https://html.spec.whatwg.org/#element-definition-is-running

BUG=643051

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

[commit-bot: I haz the power, 2016-09-04 01:02:56]

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

[kochi, 2016-09-05 02:28:47]

lgtm sorry for the delay, missed this on Friday.

domenic@, the pattern ElementDefinitionIsRunning class is
using is so-called RAII; often used in C++ code.

[domenic, 2016-09-06 18:14:21]

On 2016/09/05 at 02:28:47, kochi wrote:
> lgtm sorry for the delay, missed this on Friday.
> 
> domenic@, the pattern ElementDefinitionIsRunning class is
> using is so-called RAII; often used in C++ code.

Right, I'm very aware of RAII; it was taking a bool& reference to another
class's private members and mutating it as part of the RAII lifecycle that I
hadn't seen before.