Process Custom Elements in post-order.

Process Custom Elements in post-order.

Custom Element upgrade + created callback invocation used to be
processed in reverse-creation order, which is an expedient way to
ensure that child elements are processed before parents (assuming no
tree modification) so authors can access an element's content in the
created callback.

This changes the order. Descendants are still processed first, so
authors can access an element's content in the created callback, but
siblings are processed in creation order. Because Custom Element
callbacks can have side effects, this makes Custom Elements behave
more like script tags: If authors build constructs like

  <x-definition ...></>
  <x-use ...></>

they can rely on the side effects of "definition" at the point of
"use".

This is based on a proposal described here:
<https://www.w3.org/Bugs/Public/show_bug.cgi?id=22899>;

BUG=234509
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156141

[dominicc (has gone to gerrit), 2013-08-13 05:31:33]

PTAL

[dglazkov, 2013-08-13 16:22:06]

https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
File Source/core/dom/CustomElement.cpp (right):

https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
Source/core/dom/CustomElement.cpp:96: if
(!elementsBeingParsed().contains(element))
I am trying to figure out why we need this check and the hashset. Can you
explain this a bit more?

https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
File Source/core/dom/CustomElementRegistrationContext.cpp (right):

https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
Source/core/dom/CustomElementRegistrationContext.cpp:147: if (CreatedByParser ==
mode)
why'd you flip the condition like that? :)

https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
File Source/core/dom/CustomElementRegistrationContext.h (right):

https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
Source/core/dom/CustomElementRegistrationContext.h:62: CreatedByCloning,
Sounds like this guy is not being used at the moment. There are plans for it,
though -- right?

https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
File Source/core/dom/CustomElementUpgradeCandidateMap.cpp (right):

https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
Source/core/dom/CustomElementUpgradeCandidateMap.cpp:105:
ElementObserverMap::iterator it = elementObservers().find(element);
The observer terminology is much better. Though it is a change that's only
vaguely connected to the goal of the patch :)

[dominicc (has gone to gerrit), 2013-08-13 22:54:45]

Thanks for the feedback.

On 2013/08/13 16:22:06, Dimitri Glazkov wrote:
>
https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
> File Source/core/dom/CustomElement.cpp (right):
> 
>
https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
> Source/core/dom/CustomElement.cpp:96: if
> (!elementsBeingParsed().contains(element))
> I am trying to figure out why we need this check and the hashset. Can you
> explain this a bit more?

Basically, we need to hold off on doing anything with elements during parsing.
Based on our chat, let me throw caution to the wind and set the "upgrade
candidate" bit on these, but hold off putting them in the map.

>
https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
> File Source/core/dom/CustomElementRegistrationContext.cpp (right):
> 
>
https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
> Source/core/dom/CustomElementRegistrationContext.cpp:147: if (CreatedByParser
==
> mode)
> why'd you flip the condition like that? :)
> 
>
https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
> File Source/core/dom/CustomElementRegistrationContext.h (right):
> 
>
https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
> Source/core/dom/CustomElementRegistrationContext.h:62: CreatedByCloning,
> Sounds like this guy is not being used at the moment. There are plans for it,
> though -- right?

We only care if the parser is acting or not. Should I retreat to the venerable,
mysterious bool byParser? Or should I go for

enum CreationMode {
    CreatedByParser,
    CreatedBySomethingThatIsNotTheParser
};

really not sure what to name that second member.
 
>
https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
> File Source/core/dom/CustomElementUpgradeCandidateMap.cpp (right):
> 
>
https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
> Source/core/dom/CustomElementUpgradeCandidateMap.cpp:105:
> ElementObserverMap::iterator it = elementObservers().find(element);
> The observer terminology is much better. Though it is a change that's only
> vaguely connected to the goal of the patch :)

I have split off https://codereview.chromium.org/22847004/ for this part.

[dglazkov, 2013-08-13 22:56:17]

On 2013/08/13 22:54:45, dominicc wrote:

https://codereview.chromium.org/23009004/diff/1/Source/core/dom/CustomElement...
> > Source/core/dom/CustomElementRegistrationContext.h:62: CreatedByCloning,
> > Sounds like this guy is not being used at the moment. There are plans for
it,
> > though -- right?
> 
> We only care if the parser is acting or not. Should I retreat to the
venerable,
> mysterious bool byParser? Or should I go for
> 
> enum CreationMode {
>     CreatedByParser,
>     CreatedBySomethingThatIsNotTheParser
> };
> 
> really not sure what to name that second member.

Nah. Let's leave it as you have it.

[dominicc (has gone to gerrit), 2013-08-14 05:10:32]

PTAL.

This assumes Issue 23130005 goes first.

The big change here is avoiding the set of elements being parsed; 0.5 of the
Custom Elements NodeFlags bits is repurposed for this now. The implementation
strategy is basically:

This is to do with matching instances to definitions, so the registration
context plays the primary role.

The reg. context knows if an element was created by the parser. If so, it gets
the WaitingForParser bit.

When a definition is available, it is published to the
CustomElement::DefinitionMap. If we weren't WaitingForParser, we go ahead and
schedule upgrade.

When parsing is done, we must have been WaitingForParser. (Any parsed Custom
Element is WaitingForParser.) Either a definition became available in the
meantime [1] or was always available [2], we schedule upgrade; or no definition
is available yet, we move into the WaitingForUpgrade state and continue to hang
out in the upgrade candidate map.

Another way to look at this at a very high level is that we removed the Defined
state (Issue 2313005) and instead use CustomElement::definitionFor to detect
that state.

[1] Example. Definition becomes available before parsing is done.

<x-a>
  <script>
    document.register('x-a', ...);
  </script>
</x-a>

[2] Example. Definition was always available.

<script>
  document.register('x-a', ...);
</script>
<x-a></x-a>

[dominicc (has gone to gerrit), 2013-08-14 05:52:18]

My fuzzer tells me this is not good enough yet. The first design had the
advantage of the thing watching parsing being completely static, so the lifetime
is very simple. This approach crashes because the CERC may die during parsing
(eg document.open).

Maybe I can use the different NodeFlags of this latter try but the static
callback of the former to make a hybrid of these two approaches. A vegemite and
jelly sandwich, if you will. Nutritious... and delicious. Well, maybe one of
those. Or neither.

[dominicc (has gone to gerrit), 2013-08-14 09:33:00]

OK, patch set 5 is more reliable. The fuzzer identifies one corner case in
window destruction I need to look into but I think the shape of this is
reviewable. PTAL.

[dglazkov, 2013-08-14 20:22:34]

lgtm

https://codereview.chromium.org/23009004/diff/20001/Source/core/dom/CustomEle...
File Source/core/dom/CustomElement.cpp (right):

https://codereview.chromium.org/23009004/diff/20001/Source/core/dom/CustomEle...
Source/core/dom/CustomElement.cpp:116: return definition;
Switching definitions().get to definitionFor is a great (though unrelated
change).

https://codereview.chromium.org/23009004/diff/20001/Source/core/dom/CustomEle...
File Source/core/dom/CustomElement.h (right):

https://codereview.chromium.org/23009004/diff/20001/Source/core/dom/CustomEle...
Source/core/dom/CustomElement.h:86: CustomElementDefinition* get(Element*
element) { return m_definitions.get(element); }
is this a const function?

https://codereview.chromium.org/23009004/diff/20001/Source/core/dom/CustomEle...
File Source/core/dom/CustomElementRegistrationContext.h (right):

https://codereview.chromium.org/23009004/diff/20001/Source/core/dom/CustomEle...
Source/core/dom/CustomElementRegistrationContext.h:60: PassRefPtr<Element>
createCustomTagElement(Document*, const QualifiedName&, bool byParser = false);
Can we switch to use enums here?

[commit-bot: I haz the power, 2013-08-15 04:41:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominicc@chromium.org/23009004/28001

[commit-bot: I haz the power, 2013-08-15 06:01:41]

Change committed as 156141