Create wrappers for unresolved Custom Elements at the correct type

Create wrappers for unresolved Custom Elements at the correct type

The Custom Elements spec Section 5 [1] says that elements which look like Custom Elements, but which don't have a definition yet, should be wrapped at HTMLElement for HTML elements with custom tags and SVGElement for SVG elements with custom tags.

This rejigs the Custom Element creation code to always create an element for a custom tag, even ones without a definition, and to wrap those elements directly (HTMLElement) instead of as fallback elements (HTMLUnknownElement.)

[1] <https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/custom/index.html#registering-custom-elements>;

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

[dominicc (has gone to gerrit), 2013-05-01 16:00:57]

We will now ford the muddiest swamp on the road to element upgrade.

The existing implementation only handles Custom Elements with definitions.
Element upgrade also needs to process elements that do not have definitions
yet--upgrade candidates.

This patch introduces two changes for that:

First, when processing custom tags, the CustomElementRegistry always creates an
element. Before it only created elements when it had a definition for that
element in hand.

Second, the CustomElementRegistry wraps upgrade candidates. This improves spec
conformance which says these elements should not be HTMLUnknownElement but
should be HTMLElement.

Frankly the approach to generating Custom Element wrappers is too complex and I
believe it can be radically simplified. For example just like splitting custom
tag element creation and type extension element creation simplified element
creation, wrapping can benefit from the same split.

However I would prefer to see upgrade candidates work end-to-end before wading
seriously into the wrapping code.

[dglazkov, 2013-05-01 16:51:22]

LGTM. Would love to have a second look of a "True Bindings Person".

https://codereview.chromium.org/14776002/diff/1/Source/bindings/v8/CustomElem...
File Source/bindings/v8/CustomElementHelpers.h (right):

https://codereview.chromium.org/14776002/diff/1/Source/bindings/v8/CustomElem...
Source/bindings/v8/CustomElementHelpers.h:72: class CreateWrapperFunction {
Effectively, CreateWrapperFunction carries the knowledge of which of the two
specific kinds of wrapper functions to call, allowing the callsite just call it
and expect the right thing to happen.

https://codereview.chromium.org/14776002/diff/1/Source/bindings/v8/CustomElem...
Source/bindings/v8/CustomElementHelpers.h:91: static v8::Handle<v8::Object>
createUpgradeCandidateWrapper(PassRefPtr<Element>, v8::Handle<v8::Object>
creationContext, v8::Isolate*, const CreateWrapperFunction&
createTypeExtensionUpgradeCandidateWrapper);
The new createUpgradeCandidateWrapper serves as the new fallback for custom
elements, instead of the standard HTML fallback that simply spits out
HTMLUnknownElement.

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

https://codereview.chromium.org/14776002/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:833: CustomElementRegistry*
Document::ensureCustomElementRegistry()
Making registry RAII could've been its own patch, but I am okay with rolling
this up.

[haraken, 2013-05-01 18:49:12]

https://codereview.chromium.org/14776002/diff/1/Source/bindings/v8/CustomElem...
File Source/bindings/v8/CustomElementHelpers.h (right):

https://codereview.chromium.org/14776002/diff/1/Source/bindings/v8/CustomElem...
Source/bindings/v8/CustomElementHelpers.h:49: class SVGElement;

Nit: Alphabetical order please.

https://codereview.chromium.org/14776002/diff/1/Source/bindings/v8/custom/V8C...
File Source/bindings/v8/custom/V8CustomElementConstructorCustom.cpp (right):

https://codereview.chromium.org/14776002/diff/1/Source/bindings/v8/custom/V8C...
Source/bindings/v8/custom/V8CustomElementConstructorCustom.cpp:57: return
toV8(element.get(), args.Holder(), args.GetIsolate());

What is this change for? I think the change looks correct in this case, but I'm
concerned about this kind of change because toV8() might be unexpectedly
overridden.

If you're sure that args.Holder() is a correct creation context, you should use
toV8Fast() instead of toV8().

If you're not sure, you can remove the FIXME and use toV8(element.get(),
v8::Handle<v8::Object>(), args.GetIsolate()).

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

https://codereview.chromium.org/14776002/diff/1/Source/core/dom/CustomElement...
Source/core/dom/CustomElementRegistry.cpp:230: return element.release();

This will change behavior. Previously 0 was returned when definition is 0. Is
this change OK?

[dominicc (has gone to gerrit), 2013-05-02 01:29:56]

Thanks for your advice!

haraken--PTAL

https://codereview.chromium.org/14776002/diff/1/Source/bindings/v8/CustomElem...
File Source/bindings/v8/CustomElementHelpers.h (right):

https://codereview.chromium.org/14776002/diff/1/Source/bindings/v8/CustomElem...
Source/bindings/v8/CustomElementHelpers.h:72: class CreateWrapperFunction {
On 2013/05/01 16:51:22, Dimitri Glazkov wrote:
> Effectively, CreateWrapperFunction carries the knowledge of which of the two
> specific kinds of wrapper functions to call, allowing the callsite just call
it
> and expect the right thing to happen.

I added a brief comment. It's C++ man... in any reasonable language this would
be 3 lines of code. But as George Bernard Shaw said, "all progress depends on
unreasonable languages." Or something like that.

Once upgrade is happening I will tear into this; I think I can obviate the need
for this.

https://codereview.chromium.org/14776002/diff/1/Source/bindings/v8/custom/V8C...
File Source/bindings/v8/custom/V8CustomElementConstructorCustom.cpp (right):

https://codereview.chromium.org/14776002/diff/1/Source/bindings/v8/custom/V8C...
Source/bindings/v8/custom/V8CustomElementConstructorCustom.cpp:57: return
toV8(element.get(), args.Holder(), args.GetIsolate());
On 2013/05/01 18:49:12, haraken wrote:
> What is this change for?

I want to simplify this so that it doesn't have intimate knowledge of how custom
element wrapping works. I basically want to be the equivalent of
"createElement".

> I think the change looks correct in this case, but I'm
> concerned about this kind of change because toV8() might be unexpectedly
> overridden.

My hope is that there is nothing special about elements created by calling a
constructor with "new", doing createElement, or setting innerHTML and doing
querySelector. Since the toV8 wrapping machinery has to work in those cases, it
should work here too.

> If you're sure that args.Holder() is a correct creation context, you should
use
> toV8Fast() instead of toV8().
> 
> If you're not sure, you can remove the FIXME and use toV8(element.get(),
> v8::Handle<v8::Object>(), args.GetIsolate()).

I stared at this more and I *think* args.Holder() is right. Is there a design
doc explaining the right way to use creation contexts?

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

https://codereview.chromium.org/14776002/diff/1/Source/core/dom/CustomElement...
Source/core/dom/CustomElementRegistry.cpp:230: return element.release();
On 2013/05/01 18:49:12, haraken wrote:
> 
> This will change behavior. Previously 0 was returned when definition is 0. Is
> this change OK?

Absolutely. That is what we're going for here, hence tryToCreateCustomTagElement
-> createCustomTagElement.

[dominicc (has gone to gerrit), 2013-05-02 06:17:24]

I think this upsets the Inspector somehow. Specifically the wrappers for
unresolved elements. Let me go spelunking.

[haraken, 2013-05-02 06:58:21]

LGTM in terms of binding implementation.

> > I think the change looks correct in this case, but I'm
> > concerned about this kind of change because toV8() might be unexpectedly
> > overridden.
> 
> My hope is that there is nothing special about elements created by calling a
> constructor with "new", doing createElement, or setting innerHTML and doing
> querySelector. Since the toV8 wrapping machinery has to work in those cases,
it
> should work here too.

Thanks for the clarification. toV8() looks correct.

However, as far as I see your latest patch, did you decide to use
CustomElementHelpers::wrap?

> > If you're sure that args.Holder() is a correct creation context, you should
> use
> > toV8Fast() instead of toV8().
> > 
> > If you're not sure, you can remove the FIXME and use toV8(element.get(),
> > v8::Handle<v8::Object>(), args.GetIsolate()).
> 
> I stared at this more and I *think* args.Holder() is right. Is there a design
> doc explaining the right way to use creation contexts?

creationContext=args.Holder() means that wrap() will create a new object in the
same context that created args.Holder().

[commit-bot: I haz the power, 2013-05-02 08:26:37]

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

[commit-bot: I haz the power, 2013-05-02 08:59:35]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_layout_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-02 09:26:08]

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

[dominicc (has gone to gerrit), 2013-05-02 09:29:04]

Thanks for the review!

I have switched back to toV8 in the constructor.

I investigated the Inspector issue noted above. As I mentioned on #blink, I
think this is a latent issue that is masked by CustomElementRegistry eagerly
allocating wrappers for upgraded custom elements in
CustomElementHelpers::invokeReadyCallbackIfNeeded. I will work on making
Inspector not pass a null creation context in another patch, or at least guard
against null contexts as discussed.

[commit-bot: I haz the power, 2013-05-02 09:49:03]

Change committed as 149584