(CANCELED) Fix V8HTMLElement::constructorCustom not to recurse to itself

Fix V8HTMLElement::constructorCustom not to recurse to itself

This patch changes V8HTMLElement::constructorCustom() to call
HTMLElement::create() to create HTMLElement.

Document.createElement() can recurse to itself when
CustomElement::createCustomElement() was implemented.

BUG=594918

[kojii, 2016-06-03 07:46:37]

Description was changed from

==========
WIP: Add DisableCustomElements

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

to

==========
(NOT FOR LANDING) Add DisableCustomElements

CL to land is https://codereview.chromium.org/2036983002
==========

[kojii, 2016-06-07 04:29:24]

Description was changed from

==========
(NOT FOR LANDING) Add DisableCustomElements

CL to land is https://codereview.chromium.org/2036983002
==========

to

==========
Add CreateElementFlags::DisableCustomElements

This patch adds CreateElementFlags::DisableCustomElements so that
constructors can create element without checking defined custom
elements.
==========

[kojii, 2016-06-07 06:38:35]

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

[kojii, 2016-06-07 06:38:36]

PTAL.

[dominicc (has gone to gerrit), 2016-06-07 10:26:22]

On 2016/06/07 at 06:38:36, kojii wrote:
> PTAL.

I'm still not sure this is right.

This change adds a flag called DisableCustomElements and it affects V0 and V1,
any caller could specify this flag, but it is really only used by the
HTMLElement constructor. Note that some callers do V0 processing outside
CustomElement::createCustomElement right after they call it, which seems tricky.

I think it would be simpler if HTMLElementFactory::createElement did not kicking
off custom element steps. The most it should be doing is identifying that the
element may be a custom element and setting the undefined bit.

If there's practically one set of flags per caller, it would make more sense to
me for core element creation to have less responsibility, and make the caller do
the rest. So:

V8HTMLElementConstructor should do the work with wrappers, set the custom
element bit, etc.

createElement DOM callback should queue callbacks, if possible. It will have the
old V0 code paths intertwined with V1 as it must. It will be simpler to
understand what's happening if these are put together, I think.

The treebuilder/parser should run the constructor or queue callbacks.

The call sites are all very different; I don't think it makes sense to add modes
to element creation for all of these cases.

Here's what I think would be good:
- Elements are created in the uncustomized state.
- When core element creation sees a valid local name, it sets undefined state.
That's the one place it is set.
- The one exception *may be* (not sure) parserSetAttributes or the parser. It
may make sense to set the undefined bit that late for is=.
- When upgrade runs, the spec has a guard now that the state is not "custom". So
HTMLElement constructor should always find elements with the undefined bit
(whether from the construction stack or createElement) and set the custom bit.
This is the one place we should set the custom bit.
- The create element DOM method should queue upgrades and not do anything else.
- The sychronous "create an element" only happens when parsing documents. The
parser should manage running the constructor synchronously; it already manages
running script synchronously. It has all the flags that need to flip when
running these constructors.

Now, I could be totally wrong about this. But this seems simpler to me.

I think we should keep the CreateElementFlags enum, because it is more readable
than the bool createdByParser, but we should go back to CreatedByParser and
maybe the specific values for callers which specify CreatedByParser or not. I
think we can ditch the custom elements flags. AFAIK we're not switching any
behavior on those flags yet.

[kojii, 2016-06-07 13:02:56]

Patchset #3 (id:40001) has been deleted

[kojii, 2016-06-07 17:57:37]

Thank you for your review. PTAL.

On 2016/06/07 at 10:26:22, dominicc wrote:
> On 2016/06/07 at 06:38:36, kojii wrote:
> > PTAL.
> 
> I'm still not sure this is right.
> 
> This change adds a flag called DisableCustomElements and it affects V0 and V1,
any caller could specify this flag, but it is really only used by the
HTMLElement constructor. Note that some callers do V0 processing outside
CustomElement::createCustomElement right after they call it, which seems tricky.

I was under impression that V8HTMLElementCustom.cpp is only for V1 (is that
correct?) and since it's the only caller as of now, V0 should be ok, but I guess
you're talking about when we used this flag somewhere else?


> I think it would be simpler if HTMLElementFactory::createElement did not
kicking off custom element steps. The most it should be doing is identifying
that the element may be a custom element and setting the undefined bit.

We need a bit more. HTMLElementFactory::createElement will return
HTMLUnknownElement, while we need HTMLElement. That said, until we figure out
what to do with type extensions, I think we can simply use HTMLElement::create()
without adding a new flag (so avoid your concern above.) PS3 did this, and I
confirmed it works good with my local WIP.


> If there's practically one set of flags per caller, it would make more sense
to me for core element creation to have less responsibility, and make the caller
do the rest. So:
> 
> V8HTMLElementConstructor should do the work with wrappers, set the custom
element bit, etc.
> 
> createElement DOM callback should queue callbacks, if possible. It will have
the old V0 code paths intertwined with V1 as it must. It will be simpler to
understand what's happening if these are put together, I think.
> 
> The treebuilder/parser should run the constructor or queue callbacks.
> 
> The call sites are all very different; I don't think it makes sense to add
modes to element creation for all of these cases.

Understood and agree not to add modes for all of these cases.


> Here's what I think would be good:
> - Elements are created in the uncustomized state.

Agreed. The initial state is uncustomized now.

> - When core element creation sees a valid local name, it sets undefined state.
That's the one place it is set.

If this CL works, it'd be V8HTMLElementCustom.cpp, which matches to the spec
IIUC, no? We have another in CustomElement, this probably needs to be cleaned up
to match to the spec, I'll think a bit more when I implement other steps.

> - The one exception *may be* (not sure) parserSetAttributes or the parser. It
may make sense to set the undefined bit that late for is=.

Right...

> - When upgrade runs, the spec has a guard now that the state is not "custom".
So HTMLElement constructor should always find elements with the undefined bit
(whether from the construction stack or createElement) and set the custom bit.
This is the one place we should set the custom bit.
> - The create element DOM method should queue upgrades and not do anything
else.
> - The sychronous "create an element" only happens when parsing documents. The
parser should manage running the constructor synchronously; it already manages
running script synchronously. It has all the flags that need to flip when
running these constructors.

Do you mean if we get a consensus for createElement() to use upgrade rather than
sync? I don't have opinions here, but it looks like consensus isn't easy.

> Now, I could be totally wrong about this. But this seems simpler to me.
> 
> I think we should keep the CreateElementFlags enum, because it is more
readable than the bool createdByParser, but we should go back to CreatedByParser
and maybe the specific values for callers which specify CreatedByParser or not.
I think we can ditch the custom elements flags. AFAIK we're not switching any
behavior on those flags yet.

I haven't given enough thoughts on type extensions and went too far in this CL,
sorry about that. Agree that less flags is better.

[kojii, 2016-06-07 19:06:30]

Description was changed from

==========
Add CreateElementFlags::DisableCustomElements

This patch adds CreateElementFlags::DisableCustomElements so that
constructors can create element without checking defined custom
elements.
==========

to

==========
Fix V8HTMLElement::constructorCustom not to recurse to itself

This patch changes V8HTMLElement::constructorCustom() to call
HTMLElement::create() to create HTMLElement.

Document.createElement() can recurse to itself when
CustomElement::createCustomElement() was implemented.

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

[kojii, 2016-06-08 16:54:50]

Allow me to cancel this, I think it's easier to review with sync CL.