Simplify Custom Element constructors to be functions, not wrappers

Simplify Custom Element constructors to be functions, not wrappers

Custom Element constructors are returned to script and not used by the bindings again. It is not necessary for them to be full-blown wrapped objects.

BUG=234509
R=haraken@chromium.org, morrita@chromium.org

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

[dominicc (has gone to gerrit), 2013-06-09 23:33:54]

This is a WIP but if you have preliminary feedback I welcome it. I think it is
close.

Bindings in general, and string handling in particular, changed a lot since I
was here last. I'm probably doing something stupid. Is document wrapped the
right way in createConstructor? Is global the right context for that?

Can we remove this WrapAsFunction stuff now? CustomElementConstructor.idl was
the only thing I saw using it.

I understand now why ScriptValue is hated so--RefPtr SharedPersistent! I would
like to refactor this so that this is handled in the bindings layer and we can
avoid ScriptValue. But one step at a time.

I also experimented with doing this with v8::Script returning a function that
created a closure instead of a FunctionTemplate. I wish we could implement more
of Custom Elements in JavaScript. The fundamental problem is that there's no way
to tell if a call is [[Call]] or [[Construct]] from within JavaScript. Maybe
when ES6 classes are available I can try again.

[haraken, 2013-06-10 01:18:19]

Here are just rough comments:

> I understand now why ScriptValue is hated so--RefPtr SharedPersistent! I would
> like to refactor this so that this is handled in the bindings layer and we can
> avoid ScriptValue. But one step at a time.

In general a ScriptValue is dangerous since it can easily produce a circular
reference due to the SharedPersistent. However, in your specific implementation,
using a ScriptValue looks OK. You are using a ScriptValue just for returning a
value to V8 and do not hold the ScriptValue anywhere in Blink.

BTW, why do you need to make the return type of webkitRegister() 'any'? 'any'
means that you can return any types, whereas webkitRegister() can just return a
Function. Maybe you have to use 'any' since there is no IDL type corresponding
to a Function? Can't you use a callback function type?
http://dev.w3.org/2006/webapi/WebIDL/#idl-callback-function

> I also experimented with doing this with v8::Script returning a function that
> created a closure instead of a FunctionTemplate. I wish we could implement
more
> of Custom Elements in JavaScript. The fundamental problem is that there's no
way
> to tell if a call is [[Call]] or [[Construct]] from within JavaScript. Maybe
> when ES6 classes are available I can try again.

Yeah. The more JavaScript, the better. I'd guess you can move a bunch of
GetHiddenValue/SetHiddenValue/HasOwnProperty/Set etc in CustomElementHelpers.cpp
to JavaScript.


> Bindings in general, and string handling in particular, changed a lot since I
> was here last. I'm probably doing something stupid. Is document wrapped the
> right way in createConstructor? Is global the right context for that?

I guess it's right but morrita-san should take a look.

[dominicc (has gone to gerrit), 2013-06-10 03:26:47]

PTAL

[dominicc (has gone to gerrit), 2013-06-10 03:32:02]

PTAL

[dominicc (has gone to gerrit), 2013-06-10 03:37:22]

On 2013/06/10 01:18:19, haraken wrote:
> BTW, why do you need to make the return type of webkitRegister() 'any'? 'any'
> means that you can return any types, whereas webkitRegister() can just return
a
> Function. Maybe you have to use 'any' since there is no IDL type corresponding
> to a Function? Can't you use a callback function type?
> http://dev.w3.org/2006/webapi/WebIDL/#idl-callback-function

Just laziness; ScriptValue does not need much handling.

I have removed WrapAsFunction and have updated callback function parsing
(although only the name is used.) Now I'm using callback functions. Like enums
the definition needs to be in the same file as their use, which is OK for now,
since this is only used in one place.

> > I also experimented with doing this with v8::Script returning a function
that
> > created a closure instead of a FunctionTemplate. I wish we could implement
> more
> > of Custom Elements in JavaScript. The fundamental problem is that there's no
> way
> > to tell if a call is [[Call]] or [[Construct]] from within JavaScript. Maybe
> > when ES6 classes are available I can try again.
> 
> Yeah. The more JavaScript, the better. I'd guess you can move a bunch of
> GetHiddenValue/SetHiddenValue/HasOwnProperty/Set etc in
CustomElementHelpers.cpp
> to JavaScript.

Yes, I just used a closure:

(function (create, document, namespaceURI, name, type) {
  return function () {
    try {
      create(document, namespaceURI, name, type);
    } catch (e) {
      throw e;
    }
  };
})

Where "create" was a native function that called createElementNS. If there was a
mechanism to get unadulterated Document.prototype.createElementNS and
Function.prototoype.call, I would not even need that. This needs to throw
TypeError when called as an ordinary function though, which I think needs ES6.

> > Bindings in general, and string handling in particular, changed a lot since
I
> > was here last. I'm probably doing something stupid. Is document wrapped the
> > right way in createConstructor? Is global the right context for that?
> 
> I guess it's right but morrita-san should take a look.

I have given him a courtesy CC.

[Hajime Morrita, 2013-06-10 04:25:19]

We can close https://code.google.com/p/chromium/issues/detail?id=234501 once
this CL is in.

[Hajime Morrita, 2013-06-10 04:32:21]

https://codereview.chromium.org/16708002/diff/7001/Source/bindings/v8/CustomE...
File Source/bindings/v8/CustomElementHelpers.cpp (right):

https://codereview.chromium.org/16708002/diff/7001/Source/bindings/v8/CustomE...
Source/bindings/v8/CustomElementHelpers.cpp:123: void
constructCustomElement(const v8::FunctionCallbackInfo<v8::Value>& args)
Nit: can be static.

https://codereview.chromium.org/16708002/diff/7001/Source/core/dom/CustomElem...
File Source/core/dom/CustomElementRegistry.cpp (right):

https://codereview.chromium.org/16708002/diff/7001/Source/core/dom/CustomElem...
Source/core/dom/CustomElementRegistry.cpp:155: ScriptValue constructor =
CustomElementHelpers::createConstructor(state, prototypeValue, document(),
definition->namespaceURI(), definition->name(), definition->isTypeExtension() ?
definition->type() : nullAtom);
Can this fail? We might need some guard if so.

[Hajime Morrita, 2013-06-10 04:44:21]

lgtm other than these nits.

[dominicc (has gone to gerrit), 2013-06-10 04:45:29]

Thanks for the review. createConstructor fails in
document-register-reentrant-throwing-constructor. I have made this an early
return instead of limping along. PTAL

[haraken, 2013-06-10 09:11:02]

This is great improvement! Now we have 'callback' in the Web IDL spec!

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/scripts/C...
File Source/bindings/scripts/CodeGeneratorV8.pm (left):

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:666: if
($interface->extendedAttributes->{"WrapAsFunction"}) {

Now you can remove [WrapAsFunction] from IDLAttribute.txt as well.

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/scripts/I...
File Source/bindings/scripts/IDLParser.pm (right):

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:44: struct( CallbackFunction => {

Nit: CallbackFunction => callbackFunction

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:46: returnType => '$',

Nit: Use 'type' for consistency with other data structure.

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:47: arguments => '@',

Nit: Use 'parameters' for consistency with other data structure.

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/tests/idl...
File Source/bindings/tests/idls/TestObject.idl (right):

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/tests/idl...
Source/bindings/tests/idls/TestObject.idl:183: any anyReturnValue();

Do you need to add this test? In conclusion, it looks like this CL doesn't need
to implement a method that returns 'any'.

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/tests/idl...
Source/bindings/tests/idls/TestObject.idl:187: void
callbackFunctionArgument(TestCallbackFunction f);

Nit: f => function

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
File Source/bindings/v8/CustomElementHelpers.cpp (right):

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:131: 

I think you need to add the following check here:

  if (ConstructorMode::current() == ConstructorMode::WrapExistingObject) {
    v8SetReturnValue(args, args.Holder());
    return;
  }

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:132: if (args.Length() > 0) {

Nit: Do you need this check?

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:153: ScriptValue
CustomElementHelpers::createConstructor(ScriptState* state, const ScriptValue&
prototypeValue, Document* document, const AtomicString& namespaceURI, const
AtomicString& name, const AtomicString& type)

Not related to this CL: won't prototypeValue cause a circular reference? If
prototypeValue holds a reference to a window object (I'm not sure if it's
possible or not in terms of the semantics of custom elements), a circle is
produced: window => V8PerContextData => prototypeValue => window.

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:171: constructor->SetName(v8Name);

You need to somehow handle a case where constructor is Empty.

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:173: v8::Handle<v8::String>
v8TypeString = v8String(type, isolate);

By using v8StringOrNull() here, I think you can kill the if branch.

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:178:
constructor->SetHiddenValue(v8String("document", isolate), toV8(document,
state->context()->Global(), isolate));

Can't you use toV8Fast(), given that we already know a creation context here?

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
File Source/bindings/v8/CustomElementHelpers.h (right):

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.h:35: #include
"bindings/v8/ScriptState.h"

Nit: forward declaration will be enough.

https://codereview.chromium.org/16708002/diff/16001/Source/core/dom/CustomEle...
File Source/core/dom/CustomElementRegistry.cpp (right):

https://codereview.chromium.org/16708002/diff/16001/Source/core/dom/CustomEle...
Source/core/dom/CustomElementRegistry.cpp:155: ScriptValue constructor =
CustomElementHelpers::createConstructor(state, prototypeValue, document(),
definition->namespaceURI(), definition->name(), definition->isTypeExtension() ?
definition->type() : nullAtom);

We might want to implement LocalScriptValue which is just intended to pass V8
Values outside bindings/. (Or you can simply use v8::Handle<v8::Function> here,
although there still is a discussion about whether we should allow v8:: outside
bindings/.)

At least, we might want to avoid ScriptValue, which is dangerous and going to be
removed.

https://codereview.chromium.org/16708002/diff/16001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/16708002/diff/16001/Source/core/dom/Document....
Source/core/dom/Document.h:1054: ScriptValue
registerElement(WebCore::ScriptState*, const AtomicString& name, const
Dictionary& options, ExceptionCode&);

Ditto. ScriptValue => LocalScriptValue or v8::Handle<v8::Function>.

[abarth-chromium, 2013-06-10 17:42:18]

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
File Source/bindings/v8/CustomElementHelpers.cpp (right):

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:138:
V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<>, namespaceURI,
args.Callee()->GetHiddenValue(v8String("namespaceURI", isolate)));
Please use V8HiddenPropertyNames rather than allocating new strings every time.

[dominicc (has gone to gerrit), 2013-06-11 01:28:20]

On 2013/06/10 17:42:18, abarth wrote:
>
https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
> File Source/bindings/v8/CustomElementHelpers.cpp (right):
> 
>
https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
> Source/bindings/v8/CustomElementHelpers.cpp:138:
> V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<>, namespaceURI,
> args.Callee()->GetHiddenValue(v8String("namespaceURI", isolate)));
> Please use V8HiddenPropertyNames rather than allocating new strings every
time.

Done, but I'm still creating strings for "prototype" and "constructor". Should I
add something like V8HiddenPropertyNames to cache ordinary strings too?

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

Thanks for the detailed feedback. Some comments inline. Not otherwise noted =>
done.

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
File Source/bindings/v8/CustomElementHelpers.cpp (right):

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:131: 
On 2013/06/10 09:11:03, haraken wrote:
> 
> I think you need to add the following check here:
> 
>   if (ConstructorMode::current() == ConstructorMode::WrapExistingObject) {
>     v8SetReturnValue(args, args.Holder());
>     return;
>   }

This is different to a wrapper constructor; this function is *only* called if
author script is doing

new FooElement()

where FooElement is a custom element. Basically we're using the facility of
JavaScript where a function, even when invoked as a constructor, can return a
different object.

We'll invoke the HTMLElement wrapper constructor as usual as the element is
being wrapped to be returned. But that won't call this function.

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:132: if (args.Length() > 0) {
On 2013/06/10 09:11:03, haraken wrote:
> 
> Nit: Do you need this check?

This is per <http://www.w3.org/TR/WebIDL/#es-interface-call> step 7.

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:153: ScriptValue
CustomElementHelpers::createConstructor(ScriptState* state, const ScriptValue&
prototypeValue, Document* document, const AtomicString& namespaceURI, const
AtomicString& name, const AtomicString& type)
On 2013/06/10 09:11:03, haraken wrote:
> 
> Not related to this CL: won't prototypeValue cause a circular reference? If
> prototypeValue holds a reference to a window object (I'm not sure if it's
> possible or not in terms of the semantics of custom elements), a circle is
> produced: window => V8PerContextData => prototypeValue => window.

We discussed this offline: 

- We think that the FunctionTemplate->Function etc. references are OK.

- I am separately going to try to remove the reference from C++ Document->C++
Custom Element Registry->C++ Custom Element Definition->prototypeValue by
weakening it/moving it to V8PerContextData.

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:171: constructor->SetName(v8Name);
On 2013/06/10 09:11:03, haraken wrote:
> 
> You need to somehow handle a case where constructor is Empty.

When would that happen? I think we early return on line 161. I added an
assertion.

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:178:
constructor->SetHiddenValue(v8String("document", isolate), toV8(document,
state->context()->Global(), isolate));
On 2013/06/10 09:11:03, haraken wrote:
> 
> Can't you use toV8Fast(), given that we already know a creation context here?

Not sure... ScriptState is not a "HolderContainer", but I could make a wrapper.
What would .Holder be?

[haraken, 2013-06-11 01:39:20]

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
File Source/bindings/v8/CustomElementHelpers.cpp (right):

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:171: constructor->SetName(v8Name);
On 2013/06/11 01:29:02, dominicc wrote:
> On 2013/06/10 09:11:03, haraken wrote:
> > 
> > You need to somehow handle a case where constructor is Empty.
> 
> When would that happen? I think we early return on line 161. I added an
> assertion.

I just wanted to make sure here that the constructor is not empty. The ASSERT is
fine.

In general, given:

  v8::TryCatch tryCatch;
  v8::Handle<v8::Value> ret = v8::SomeAPI();

tryCatch.HasCaught() and ret.IsEmpty() are not equivalent for some V8 APIs. It's
a good idea to check both of them.

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:178:
constructor->SetHiddenValue(v8String("document", isolate), toV8(document,
state->context()->Global(), isolate));

You're right. Please ignore my comment. You can use toV8Fast() only when you
have 'info' or 'args' passed by V8.

[dominicc (has gone to gerrit), 2013-06-11 02:41:33]

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
File Source/bindings/v8/CustomElementHelpers.cpp (right):

https://codereview.chromium.org/16708002/diff/16001/Source/bindings/v8/Custom...
Source/bindings/v8/CustomElementHelpers.cpp:171: constructor->SetName(v8Name);
On 2013/06/11 01:39:21, haraken wrote:
> On 2013/06/11 01:29:02, dominicc wrote:
> > On 2013/06/10 09:11:03, haraken wrote:
> > > 
> > > You need to somehow handle a case where constructor is Empty.
> > 
> > When would that happen? I think we early return on line 161. I added an
> > assertion.
> 
> I just wanted to make sure here that the constructor is not empty. The ASSERT
is
> fine.
> 
> In general, given:
> 
>   v8::TryCatch tryCatch;
>   v8::Handle<v8::Value> ret = v8::SomeAPI();
> 
> tryCatch.HasCaught() and ret.IsEmpty() are not equivalent for some V8 APIs.
It's
> a good idea to check both of them.

Done.

[haraken, 2013-06-14 08:08:40]

LGTM

[dominicc (has gone to gerrit), 2013-06-14 08:17:23]

Committed patchset #7 manually as r152430 (presubmit successful).