Fix V8 bindings for named constructors to set prototype object correctly

Fix V8 bindings for named constructors to set prototype object correctly

Fix V8 bindings for named constructors to set prototype object correctly by
explicitly setting the prototype of the object after constructing the Function.

Also adds a V8PrivateProperty to store whether the prototype has been set, so
that it is only set once per object.

BUG=310776
Review-Url: https://codereview.chromium.org/2647643002
Cr-Commit-Position: refs/heads/master@{#455659}
Committed: https://chromium.googlesource.com/chromium/src/+/a47c72562d1182fb1ee8c62cbc252f9965103d53

[sashab, 2017-01-19 08:16:55]

sashab@chromium.org changed reviewers:
+ bashi@chromium.org, yukishiino@chromium.org

[sashab, 2017-01-19 08:16:56]

PTAL :-)

[sashab, 2017-01-19 08:18:51]

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

[commit-bot: I haz the power, 2017-01-19 08:19:08]

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

[Yuki, 2017-01-19 09:57:27]

yukishiino@chromium.org changed reviewers:
+ haraken@chromium.org

[Yuki, 2017-01-19 09:57:28]

Overall, looks great.  Mostly nitpicks.

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/named-constructor-prototype-chain.html
(right):

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/named-constructor-prototype-chain.html:9:
assert_not_equals(Image.prototype.__proto__, HTMLImageElement.prototype,
"Image.prototype __proto__ is not HTMLImageElement prototype");
We know that Image.prototype.__proto__ must be HTMLElement.prototype, I'd
recommend to compare with HTMLElement.prototype.

Also, we know that:
  Image.__proto__ should be HTMLElement
  Image.name should be "Image" instead of "HTMLImageElement"

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:576: # FIXME:
replace this with [NamedConstructor] extended attribute
nit: FIXME is an old style in WebKit era, and we're now using the following
style.
  TODO(someone): ...

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:336:
includes.add('core/frame/LocalDOMWindow.h')
You may want to add 'bindings/core/v8/V8PrivateProperty.h' only if there is a
named constructor, instead of adding it unconditionally.

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:430: if
attribute.is_named_constructor else
nit: Indent one more space?
    ('V8%s::...' % (...)
     if attribute... else
     '...')
because you have an open parenthesis.

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:551:
result->SetClassName(v8AtomicString(isolate, "{{cpp_class}}"));
You may want to change the class name to "{{named_constructor.name}}".

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:560:
ExceptionState exceptionState(info.GetIsolate(),
ExceptionState::ConstructionContext, "{{interface_name}}");
I think this should be ExceptionState::GetterContext because this is the getter
callback for named constructor.  Also you should specify
"{{named_constructor.name}}" as a propertyName argument.

And let's move this declaration into the following if-clause because you need to
throw an exception only inside the if-clause.

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:568:
v8::Local<v8::Function> namedConstructorInterface =
perContextData->constructorForType(WrapperTypeInfo::unwrap(data));
Now this function is specific to {{v8_class}} and you know that you want to use
{{v8_class}}Constructor::wrapperTypeInfo defined above, you no longer need to
use |data| here.

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:568:
v8::Local<v8::Function> namedConstructorInterface =
perContextData->constructorForType(WrapperTypeInfo::unwrap(data));
Exactly speaking, this is a "named constructor" and not a DOM interface (while
HTMLImageElement is a DOM interface).  So let's name this "namedConstructor".

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:578:
exceptionState.throwTypeError("Could not get prototype for {{v8_class}}.");
Please return immediately right after throwing an exception.

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:582:
exceptionState.throwTypeError("Could not set prototype for
{{interface_name}}.");
Please return immediately after throwing an exception.
And the message should be "set prototype for {{named_constructor.name}}" because
you're setting the prototype object on the named constructor, I think?

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:589: info,
perContextData->constructorForType(WrapperTypeInfo::unwrap(data)));
|namedConstructorInterface| is what you'd like to return because this is the
getter callback for named constructor.
    v8SetReturnValue(info, namedConstructorInterface);

[bashi, 2017-01-19 09:58:01]

bashi@chromium.org changed reviewers:
- haraken@chromium.org

[bashi, 2017-01-19 09:58:01]

Thank you for fixing this! lgtm on my side, but please wait for yukishiino@'s
review.

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:576: # FIXME:
replace this with [NamedConstructor] extended attribute
Can we remove this comment? I think FIXME comments like line 571 no longer make
sense.

[commit-bot: I haz the power, 2017-01-19 10:04:26]

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

[commit-bot: I haz the power, 2017-01-19 10:04:27]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Yuki, 2017-01-19 10:09:30]

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:576: # FIXME:
replace this with [NamedConstructor] extended attribute
On 2017/01/19 09:58:01, bashi wrote:
> Can we remove this comment? I think FIXME comments like line 571 no longer
make
> sense.

I'm fine to remove the comment.

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:560:
ExceptionState exceptionState(info.GetIsolate(),
ExceptionState::ConstructionContext, "{{interface_name}}");
On 2017/01/19 09:57:28, Yuki wrote:
> I think this should be ExceptionState::GetterContext because this is the
getter
> callback for named constructor.  Also you should specify
> "{{named_constructor.name}}" as a propertyName argument.

And the interface name should be "Window" instead of "{{interface_name}}" though
it's unfortunate to have to hard-code here.

Named constructors are exposed as
    window.Image
where "Window" is an interface name of |window| and "Image" is a property name.

Hard-coding is not great, but we know that only Window exposes named
constructors, so the interface name must be "Window".


> And let's move this declaration into the following if-clause because you need
to
> throw an exception only inside the if-clause.

[sashab, 2017-01-20 04:37:06]

Thanks for all your review feedback - yuki i just had one question, about a
failing test

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/named-constructor-prototype-chain.html
(right):

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/named-constructor-prototype-chain.html:9:
assert_not_equals(Image.prototype.__proto__, HTMLImageElement.prototype,
"Image.prototype __proto__ is not HTMLImageElement prototype");
On 2017/01/19 at 09:57:28, Yuki wrote:
> We know that Image.prototype.__proto__ must be HTMLElement.prototype, I'd
recommend to compare with HTMLElement.prototype.
> 
> Also, we know that:
>   Image.__proto__ should be HTMLElement
>   Image.name should be "Image" instead of "HTMLImageElement"

Done.

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_attributes.py:576: # FIXME:
replace this with [NamedConstructor] extended attribute
On 2017/01/19 at 10:09:30, Yuki wrote:
> On 2017/01/19 09:58:01, bashi wrote:
> > Can we remove this comment? I think FIXME comments like line 571 no longer
make
> > sense.
> 
> I'm fine to remove the comment.

Removed all 3 comments on lines 571, 566 and new added comment.

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:336:
includes.add('core/frame/LocalDOMWindow.h')
On 2017/01/19 at 09:57:28, Yuki wrote:
> You may want to add 'bindings/core/v8/V8PrivateProperty.h' only if there is a
named constructor, instead of adding it unconditionally.

Done :)

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:430: if
attribute.is_named_constructor else
On 2017/01/19 at 09:57:28, Yuki wrote:
> nit: Indent one more space?
>     ('V8%s::...' % (...)
>      if attribute... else
>      '...')
> because you have an open parenthesis.

Good idea -- done.

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:560:
ExceptionState exceptionState(info.GetIsolate(),
ExceptionState::ConstructionContext, "{{interface_name}}");
On 2017/01/19 at 10:09:30, Yuki wrote:
> On 2017/01/19 09:57:28, Yuki wrote:
> > I think this should be ExceptionState::GetterContext because this is the
getter
> > callback for named constructor.  Also you should specify
> > "{{named_constructor.name}}" as a propertyName argument.
> 
> And the interface name should be "Window" instead of "{{interface_name}}"
though it's unfortunate to have to hard-code here.
> 
> Named constructors are exposed as
>     window.Image
> where "Window" is an interface name of |window| and "Image" is a property
name.
> 
> Hard-coding is not great, but we know that only Window exposes named
constructors, so the interface name must be "Window".
> 
> 
> > And let's move this declaration into the following if-clause because you
need to
> > throw an exception only inside the if-clause.

Hard-coded window, added propertyName argument and moved declaration into
if-statement. Also added comment:
// Only Window exposes named constructors, so that will always be the interface
name.

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:568:
v8::Local<v8::Function> namedConstructorInterface =
perContextData->constructorForType(WrapperTypeInfo::unwrap(data));
On 2017/01/19 at 09:57:28, Yuki wrote:
> Now this function is specific to {{v8_class}} and you know that you want to
use {{v8_class}}Constructor::wrapperTypeInfo defined above, you no longer need
to use |data| here.

Done the name change, and removed use of data :)

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:578:
exceptionState.throwTypeError("Could not get prototype for {{v8_class}}.");
On 2017/01/19 at 09:57:28, Yuki wrote:
> Please return immediately right after throwing an exception.

Done

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:582:
exceptionState.throwTypeError("Could not set prototype for
{{interface_name}}.");
On 2017/01/19 at 09:57:28, Yuki wrote:
> Please return immediately after throwing an exception.
> And the message should be "set prototype for {{named_constructor.name}}"
because you're setting the prototype object on the named constructor, I think?

Yes, done.

https://codereview.chromium.org/2647643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:589: info,
perContextData->constructorForType(WrapperTypeInfo::unwrap(data)));
On 2017/01/19 at 09:57:28, Yuki wrote:
> |namedConstructorInterface| is what you'd like to return because this is the
getter callback for named constructor.
>     v8SetReturnValue(info, namedConstructorInterface);

Ahh of course, thank you. Done.

https://codereview.chromium.org/2647643002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/named-constructor-prototype-chain.html
(right):

https://codereview.chromium.org/2647643002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/named-constructor-prototype-chain.html:7:
assert_equals(Image.name, "Image", "Image name should be Image (not
HTMLImageElement)");
This was failing - fixed by updating setClassName()

https://codereview.chromium.org/2647643002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/named-constructor-prototype-chain.html:8:
assert_equals(Image.__proto__, HTMLElement, "Image __proto__ is HTMLElement");
This test fails with the error:
FAIL NamedConstructor creates the correct object structure. assert_equals: Image
__proto__ is HTMLElement expected function "function HTMLElement() { [native
code] }" but got function "function () { [native code] }"

Any ideas why? :/

[sashab, 2017-01-20 05:26:39]

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

[sashab, 2017-01-20 05:26:49]

Tyvm! Fixed failing test. :)

[commit-bot: I haz the power, 2017-01-20 05:26:56]

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

[Yuki, 2017-01-20 05:54:42]

Mostly l-g-t-m.

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:335:
includes.add('bindings/core/v8/V8PrivateProperty.h')
nit: You can do this only if named_constructor:
Then, the diff of run-bindings-tests will be very small.

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:449: if
attribute.constructor_type else '0' %}
nit: Since you no longer need |data| in NamedConstructorAttributeGetter, you
don't need this wrapper type info, too.  You can get rid of this if it's named
constructor.

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:562:
v8::Local<v8::Value> data = info.Data();
You no longer use |data|.

[commit-bot: I haz the power, 2017-01-20 07:08:41]

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

[commit-bot: I haz the power, 2017-01-20 07:08:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[haraken, 2017-01-20 08:07:04]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-01-20 08:07:04]

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:570:
v8::Local<v8::Function> interface =
perContextData->constructorForType(&{{v8_class}}::wrapperTypeInfo);

Move this code into the if clause at line 576.

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:573:
v8::Local<v8::Context> context = info.GetIsolate()->GetCurrentContext();

You're using info.Holder()->CreationContext() at line 565 whereas you're using
GetCurrentContext() at line 573. Which one is correct?

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:579:
v8::MaybeLocal<v8::Value> interfacePrototype = interface->Get(context,
v8AtomicString(info.GetIsolate(), "prototype"));

Can you avoid using a MaybeLocal handle?

  Local<v8::Value> interfacePrototype;
  if (!interface->Get(...)->ToLocal(interfacePrototype)) {
    return;
  }
  ...;

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:585:
v8::Maybe<bool> setResult = namedConstructor->Set(context,
v8AtomicString(info.GetIsolate(), "prototype"),
interfacePrototype.ToLocalChecked());

Ditto. Avoid using Maybe. You can use To.

[Yuki, 2017-01-20 15:20:30]

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:573:
v8::Local<v8::Context> context = info.GetIsolate()->GetCurrentContext();
On 2017/01/20 08:07:04, haraken wrote:
> 
> You're using info.Holder()->CreationContext() at line 565 whereas you're using
> GetCurrentContext() at line 573. Which one is correct?

Both are correct.

The interface obejct and prototype object should be created in the relevant
realm (= relevant context) of the receiver object (= holder object).  So it's
correct.

APIs for private property and get/set operations require a v8::Context, but it
doesn't create anything new, so using the current context is appropriate.

Having said that, it's a good idea to rename this variable to |currentContext|
so that we don't get confused.

[sashab, 2017-01-25 02:34:50]

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

[sashab, 2017-01-25 02:34:51]

Thanks everyone, responded to all your review feedback :)

yuki -- if you could PTAL -- some of the comments I'm not sure if I responded to
correctly, lmk if I did.

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:335:
includes.add('bindings/core/v8/V8PrivateProperty.h')
On 2017/01/20 at 05:54:42, Yuki wrote:
> nit: You can do this only if named_constructor:
> Then, the diff of run-bindings-tests will be very small.

Done ty! :)

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:449: if
attribute.constructor_type else '0' %}
On 2017/01/20 at 05:54:42, Yuki wrote:
> nit: Since you no longer need |data| in NamedConstructorAttributeGetter, you
don't need this wrapper type info, too.  You can get rid of this if it's named
constructor.

Done, I think. Is this what you meant?

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:562:
v8::Local<v8::Value> data = info.Data();
On 2017/01/20 at 05:54:42, Yuki wrote:
> You no longer use |data|.

Done

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:570:
v8::Local<v8::Function> interface =
perContextData->constructorForType(&{{v8_class}}::wrapperTypeInfo);
On 2017/01/20 at 08:07:04, haraken wrote:
> Move this code into the if clause at line 576.

Oh yup nice, thx done.

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:573:
v8::Local<v8::Context> context = info.GetIsolate()->GetCurrentContext();
On 2017/01/20 at 15:20:30, Yuki wrote:
> On 2017/01/20 08:07:04, haraken wrote:
> > 
> > You're using info.Holder()->CreationContext() at line 565 whereas you're
using
> > GetCurrentContext() at line 573. Which one is correct?
> 
> Both are correct.
> 
> The interface obejct and prototype object should be created in the relevant
realm (= relevant context) of the receiver object (= holder object).  So it's
correct.
> 
> APIs for private property and get/set operations require a v8::Context, but it
doesn't create anything new, so using the current context is appropriate.
> 
> Having said that, it's a good idea to rename this variable to |currentContext|
so that we don't get confused.

Renamed to |currentContext| and only used info.Holder()->CreationContext().

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:579:
v8::MaybeLocal<v8::Value> interfacePrototype = interface->Get(context,
v8AtomicString(info.GetIsolate(), "prototype"));
On 2017/01/20 at 08:07:04, haraken wrote:
> Can you avoid using a MaybeLocal handle?
> 
>   Local<v8::Value> interfacePrototype;
>   if (!interface->Get(...)->ToLocal(interfacePrototype)) {
>     return;
>   }
>   ...;

Nice! Done.

https://codereview.chromium.org/2647643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:585:
v8::Maybe<bool> setResult = namedConstructor->Set(context,
v8AtomicString(info.GetIsolate(), "prototype"),
interfacePrototype.ToLocalChecked());
On 2017/01/20 at 08:07:04, haraken wrote:
> Ditto. Avoid using Maybe. You can use To.

Done. Changed to:

if (namedConstructor->Set(currentContext, v8AtomicString(info.GetIsolate(),
"prototype"), interfacePrototype).To() != true) {

[haraken, 2017-01-25 03:37:16]

LGTM

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:428: if
attribute.needs_constructor_getter_callback else

Can you clean up the if-else clause? If-else-if-else looks too confusing.

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:449: if
attribute.constructor_type and not attribute.is_named_constructor else '0' %}

Is this change needed?

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:562:
v8::Local<v8::Context> currentContext = info.Holder()->CreationContext();

currentContext => creationContext

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:564: if
(!perContextData)

// TODO(yukishiino): Return a valid named constructor even after the context is
detached

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:576:
v8::MaybeLocal<v8::Value> interfacePrototype = interface->Get(currentContext,
v8AtomicString(info.GetIsolate(), "prototype"));

We want to avoid using MaybeLocal. Also this Get should not fail.

  v8::Local<v8::Value> interfacePrototype =
interface->Get(...).ToLocalChecked();

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:582:
v8::Maybe<bool> setResult = namedConstructor->Set(currentContext,
v8AtomicString(info.GetIsolate(), "prototype"),
interfacePrototype.ToLocalChecked());

Ditto. Avoid using MaybeLocal. Also this Set should not fail.

bool result = namedConstructor->Set(...).ToChecked();

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.h.tmpl (right):

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.h.tmpl:24: static
v8::Local<v8::FunctionTemplate> NamedConstructorAttributeGetter(v8::Isolate*,
const DOMWrapperWorld&);

Is this needed?

[sashab, 2017-01-25 04:54:05]

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

[sashab, 2017-01-25 04:54:15]

Thanks haraken; I had a few questions so ptal :-)

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:428: if
attribute.needs_constructor_getter_callback else
On 2017/01/25 at 03:37:16, haraken wrote:
> Can you clean up the if-else clause? If-else-if-else looks too confusing.

Changed to actual {% if %} and {% else %} statements, is this what you meant? :)

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:449: if
attribute.constructor_type and not attribute.is_named_constructor else '0' %}
On 2017/01/25 at 03:37:16, haraken wrote:
> Is this change needed?

Nit suggested by yuki: "Since you no longer need |data| in
NamedConstructorAttributeGetter, you don't need this wrapper type info, too. 
You can get rid of this if it's named constructor."

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:562:
v8::Local<v8::Context> currentContext = info.Holder()->CreationContext();
On 2017/01/25 at 03:37:16, haraken wrote:
> currentContext => creationContext

Done.

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:564: if
(!perContextData)
On 2017/01/25 at 03:37:16, haraken wrote:
> // TODO(yukishiino): Return a valid named constructor even after the context
is detached

Done :)

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:576:
v8::MaybeLocal<v8::Value> interfacePrototype = interface->Get(currentContext,
v8AtomicString(info.GetIsolate(), "prototype"));
On 2017/01/25 at 03:37:16, haraken wrote:
> We want to avoid using MaybeLocal. Also this Get should not fail.
> 
>   v8::Local<v8::Value> interfacePrototype =
interface->Get(...).ToLocalChecked();

Sorry, did this locally but didn't upload it :/ Done.

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:582:
v8::Maybe<bool> setResult = namedConstructor->Set(currentContext,
v8AtomicString(info.GetIsolate(), "prototype"),
interfacePrototype.ToLocalChecked());
On 2017/01/25 at 03:37:16, haraken wrote:
> Ditto. Avoid using MaybeLocal. Also this Set should not fail.
> 
> bool result = namedConstructor->Set(...).ToChecked();

Done. Also removed exceptionContext and just returned if it fails. Is this
better?

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.h.tmpl (right):

https://codereview.chromium.org/2647643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.h.tmpl:24: static
v8::Local<v8::FunctionTemplate> NamedConstructorAttributeGetter(v8::Isolate*,
const DOMWrapperWorld&);
On 2017/01/25 at 03:37:16, haraken wrote:
> Is this needed?

Nope :) Removed.

[commit-bot: I haz the power, 2017-01-25 04:54:17]

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

[haraken, 2017-01-25 05:54:28]

LGTM

[commit-bot: I haz the power, 2017-01-25 06:06:04]

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

[commit-bot: I haz the power, 2017-01-25 06:06:05]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Yuki, 2017-01-26 07:08:53]

I'm sorry that I was not clear on this point, but there are "current context"
and "creation context (or relevant context)", and we need to use the correct
one.

It's tough to explain how to select the right one, though, the basic rule is:
- If you creates a something new, you should use the "creation context".
- If you don't create anything new and just do "get" or "set", then you should
use the "current context".

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:563:
V8PerContextData* perContextData = V8PerContextData::from(creationContext);
This is okay that you're correctly using the *creation context*.

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:573:
v8::Local<v8::Value> privateValue = privateProperty.get(creationContext,
namedConstructor);
It's quite confusing, but *current context* can be different from *creation
context*.  So, would you use *current context* here and below?

v8::Local<v8::Context> currentContext = info.GetIsolate()->GetCurrentContext();

and you should use the |currentContext| here and below.

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:576:
v8::Local<v8::Value> interfacePrototype = interface->Get(creationContext,
v8AtomicString(info.GetIsolate(), "prototype")).ToLocalChecked();
These interface-Get and namedConstructor-Set should be done on *currentContext*
instead of creationContext.

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:580:
privateProperty.set(creationContext, namedConstructor,
v8::True(info.GetIsolate()));
This also should be currentContext instead of creationContext.

[Yuki, 2017-01-26 07:42:26]

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:537: const
WrapperTypeInfo {{v8_class}}Constructor::wrapperTypeInfo = {
gin::kEmbedderBlink, {{v8_class}}Constructor::domTemplate, {{v8_class}}::trace,
{{v8_class}}::traceWrappers, 0, {{prepare_prototype_and_interface_object_func}},
"{{interface_name}}", {{parent_wrapper_type_info}},
WrapperTypeInfo::WrapperTypeObjectPrototype,
WrapperTypeInfo::{{wrapper_class_id}},
WrapperTypeInfo::{{active_scriptwrappable_inheritance}},
WrapperTypeInfo::{{event_target_inheritance}}, WrapperTypeInfo::{{lifetime}} };
I'm very sorry that I was wrong about this.

We should specify |nullptr| (i.e. 0) here as the parent wrapper type info.  I
was misunderstanding that Image.__proto__ should be HTMLElement, but it
shouldn't because Image is not considered as an interface object.

I'm very sorry, but could you revert this point (and the above definition of
parent_wrapper_type_info, and the corresponding layout test)?

[haraken, 2017-01-26 09:43:55]

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:573:
v8::Local<v8::Value> privateValue = privateProperty.get(creationContext,
namedConstructor);
On 2017/01/26 07:08:53, Yuki wrote:
> It's quite confusing, but *current context* can be different from *creation
> context*.  So, would you use *current context* here and below?
> 
> v8::Local<v8::Context> currentContext =
info.GetIsolate()->GetCurrentContext();
> 
> and you should use the |currentContext| here and below.

How can |creationContext| be different from |currentContext| here?

If they're different, we'll end up with caching a named constructor of context X
on a private property of a context Y. Which seems wrong to me.

[Yuki, 2017-01-26 10:07:11]

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:573:
v8::Local<v8::Value> privateValue = privateProperty.get(creationContext,
namedConstructor);
On 2017/01/26 09:43:55, haraken wrote:
> On 2017/01/26 07:08:53, Yuki wrote:
> > It's quite confusing, but *current context* can be different from *creation
> > context*.  So, would you use *current context* here and below?
> > 
> > v8::Local<v8::Context> currentContext =
> info.GetIsolate()->GetCurrentContext();
> > 
> > and you should use the |currentContext| here and below.
> 
> How can |creationContext| be different from |currentContext| here?

Access to |anotherWindow.Image|, then you're in the current context (by
definition), and accessing to |aotherWindow|, which is created in a different
context (this is called |creationContext| in this code because |Image| should be
created in the same context of |anotherWindow|).


> If they're different, we'll end up with caching a named constructor of context
X
> on a private property of a context Y. Which seems wrong to me.

The passed context here is NOT used to create a private key, value to be stored
in a private key, or anything else.

Players here are:
- object (or holder) of the private property
- the key (symbol) of the private property
- the value of the private property
i.e. we're performing
    obj[privateSymbol] = value
and there are three things.

|obj| is the named constructor, which is already created in |creationContext|.

|privateSymbol| does NOT depend on any v8::Context because it's common among
contexts, it only depends on v8::Isolate.

|value| is the prototype object of the interface (|interfacePrototype|), which
will be retrieved below (line 575 to 576).  Note that it's extracted from
|perContextData|, it's created in |creationContext|.

Nothing is created in |currentContext|.  |currentContext| is just used to make
V8 do "Get" and "Set" properties.  These operations should be done in
currentContext, and creationContext shouldn't be abused.

I think that haraken@ remembers a discussion that V8 may not need to take a
context in these APIs (except for throwing an exception, but an exception must
always be thrown in the current context).  Anyway, V8 decided to take a context
(even if it's not needed), then we should pass the current context.

[haraken, 2017-01-26 10:11:11]

On 2017/01/26 10:07:11, Yuki wrote:
>
https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):
> 
>
https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:573:
> v8::Local<v8::Value> privateValue = privateProperty.get(creationContext,
> namedConstructor);
> On 2017/01/26 09:43:55, haraken wrote:
> > On 2017/01/26 07:08:53, Yuki wrote:
> > > It's quite confusing, but *current context* can be different from
*creation
> > > context*.  So, would you use *current context* here and below?
> > > 
> > > v8::Local<v8::Context> currentContext =
> > info.GetIsolate()->GetCurrentContext();
> > > 
> > > and you should use the |currentContext| here and below.
> > 
> > How can |creationContext| be different from |currentContext| here?
> 
> Access to |anotherWindow.Image|, then you're in the current context (by
> definition), and accessing to |aotherWindow|, which is created in a different
> context (this is called |creationContext| in this code because |Image| should
be
> created in the same context of |anotherWindow|).
> 
> 
> > If they're different, we'll end up with caching a named constructor of
context
> X
> > on a private property of a context Y. Which seems wrong to me.
> 
> The passed context here is NOT used to create a private key, value to be
stored
> in a private key, or anything else.
> 
> Players here are:
> - object (or holder) of the private property
> - the key (symbol) of the private property
> - the value of the private property
> i.e. we're performing
>     obj[privateSymbol] = value
> and there are three things.
> 
> |obj| is the named constructor, which is already created in |creationContext|.
> 
> |privateSymbol| does NOT depend on any v8::Context because it's common among
> contexts, it only depends on v8::Isolate.
> 
> |value| is the prototype object of the interface (|interfacePrototype|), which
> will be retrieved below (line 575 to 576).  Note that it's extracted from
> |perContextData|, it's created in |creationContext|.
> 
> Nothing is created in |currentContext|.  |currentContext| is just used to make
> V8 do "Get" and "Set" properties.  These operations should be done in
> currentContext, and creationContext shouldn't be abused.
> 
> I think that haraken@ remembers a discussion that V8 may not need to take a
> context in these APIs (except for throwing an exception, but an exception must
> always be thrown in the current context).  Anyway, V8 decided to take a
context
> (even if it's not needed), then we should pass the current context.

Ah, thanks for the clarification. Makes sense.

Yeah, if the V8 API doesn't require the current context, the confusion will be
gone...

[sashab, 2017-02-21 06:37:29]

Sorry for the silence - ptal haraken and yukishiino :)

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:537: const
WrapperTypeInfo {{v8_class}}Constructor::wrapperTypeInfo = {
gin::kEmbedderBlink, {{v8_class}}Constructor::domTemplate, {{v8_class}}::trace,
{{v8_class}}::traceWrappers, 0, {{prepare_prototype_and_interface_object_func}},
"{{interface_name}}", {{parent_wrapper_type_info}},
WrapperTypeInfo::WrapperTypeObjectPrototype,
WrapperTypeInfo::{{wrapper_class_id}},
WrapperTypeInfo::{{active_scriptwrappable_inheritance}},
WrapperTypeInfo::{{event_target_inheritance}}, WrapperTypeInfo::{{lifetime}} };
On 2017/01/26 at 07:42:26, Yuki wrote:
> I'm very sorry that I was wrong about this.
> 
> We should specify |nullptr| (i.e. 0) here as the parent wrapper type info.  I
was misunderstanding that Image.__proto__ should be HTMLElement, but it
shouldn't because Image is not considered as an interface object.
> 
> I'm very sorry, but could you revert this point (and the above definition of
parent_wrapper_type_info, and the corresponding layout test)?

No prob :) Done.

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:573:
v8::Local<v8::Value> privateValue = privateProperty.get(creationContext,
namedConstructor);
On 2017/01/26 at 10:07:11, Yuki wrote:
> On 2017/01/26 09:43:55, haraken wrote:
> > On 2017/01/26 07:08:53, Yuki wrote:
> > > It's quite confusing, but *current context* can be different from
*creation
> > > context*.  So, would you use *current context* here and below?
> > > 
> > > v8::Local<v8::Context> currentContext =
> > info.GetIsolate()->GetCurrentContext();
> > > 
> > > and you should use the |currentContext| here and below.
> > 
> > How can |creationContext| be different from |currentContext| here?
> 
> Access to |anotherWindow.Image|, then you're in the current context (by
definition), and accessing to |aotherWindow|, which is created in a different
context (this is called |creationContext| in this code because |Image| should be
created in the same context of |anotherWindow|).
> 
> 
> > If they're different, we'll end up with caching a named constructor of
context X
> > on a private property of a context Y. Which seems wrong to me.
> 
> The passed context here is NOT used to create a private key, value to be
stored in a private key, or anything else.
> 
> Players here are:
> - object (or holder) of the private property
> - the key (symbol) of the private property
> - the value of the private property
> i.e. we're performing
>     obj[privateSymbol] = value
> and there are three things.
> 
> |obj| is the named constructor, which is already created in |creationContext|.
> 
> |privateSymbol| does NOT depend on any v8::Context because it's common among
contexts, it only depends on v8::Isolate.
> 
> |value| is the prototype object of the interface (|interfacePrototype|), which
will be retrieved below (line 575 to 576).  Note that it's extracted from
|perContextData|, it's created in |creationContext|.
> 
> Nothing is created in |currentContext|.  |currentContext| is just used to make
V8 do "Get" and "Set" properties.  These operations should be done in
currentContext, and creationContext shouldn't be abused.
> 
> I think that haraken@ remembers a discussion that V8 may not need to take a
context in these APIs (except for throwing an exception, but an exception must
always be thrown in the current context).  Anyway, V8 decided to take a context
(even if it's not needed), then we should pass the current context.

I don't understand the conversation, but changed to use currentContext. :)

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:576:
v8::Local<v8::Value> interfacePrototype = interface->Get(creationContext,
v8AtomicString(info.GetIsolate(), "prototype")).ToLocalChecked();
On 2017/01/26 at 07:08:53, Yuki wrote:
> These interface-Get and namedConstructor-Set should be done on
*currentContext* instead of creationContext.

Done.

https://codereview.chromium.org/2647643002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:580:
privateProperty.set(creationContext, namedConstructor,
v8::True(info.GetIsolate()));
On 2017/01/26 at 07:08:53, Yuki wrote:
> This also should be currentContext instead of creationContext.

Done.

https://codereview.chromium.org/2647643002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/named-constructor-prototype-chain.html
(right):

https://codereview.chromium.org/2647643002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/named-constructor-prototype-chain.html:9:
//assert_equals(Image.__proto__, HTMLElement, "Image __proto__ is HTMLElement");
Just to confirm -- we don't want these, correct?

[Yuki, 2017-02-21 06:57:36]

LGTM.

https://codereview.chromium.org/2647643002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/named-constructor-prototype-chain.html
(right):

https://codereview.chromium.org/2647643002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/named-constructor-prototype-chain.html:8:
//assert_equals(Image.name, "Image", "Image name should be Image (not
HTMLImageElement)");
We want this test.

https://heycam.github.io/webidl/#named-constructors
step 3. Perform ! SetFunctionName(F, id).

This means that Image.name should be "Image".

https://codereview.chromium.org/2647643002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/named-constructor-prototype-chain.html:9:
//assert_equals(Image.__proto__, HTMLElement, "Image __proto__ is HTMLElement");
On 2017/02/21 06:37:29, sashab wrote:
> Just to confirm -- we don't want these, correct?

https://heycam.github.io/webidl/#named-constructors
step 2. Let F be ! CreateBuiltinFunction(realm, steps, %FunctionPrototype% of
realm).

This means that Image.__proto__ should be Function.prototype (not HTMLElement).

[haraken, 2017-02-21 09:53:54]

LGTM

[Yuki, 2017-02-23 06:26:10]

https://codereview.chromium.org/2647643002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:554:
result->SetClassName(v8AtomicString(isolate, "{{cpp_class}}"));
You need to set the class name to "Image" instead of "HTMLImageElement", here.
"{{named_constructor.name}}" should be "Image", I think.

https://codereview.chromium.org/2647643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:555:
result->Inherit({{v8_class}}::domTemplate(isolate, world));
IIUC, now we don't need this line, because you set "prototype" property in
NamedConstructorAttributeGetter.  Could you remove this line if the removal
doesn't cause any problem?

[sashab, 2017-02-24 03:06:14]

Thanks yukishiino; ptal.

https://codereview.chromium.org/2647643002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2647643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:554:
result->SetClassName(v8AtomicString(isolate, "{{cpp_class}}"));
On 2017/02/23 at 06:26:10, Yuki(ooo_til_feb27) wrote:
> You need to set the class name to "Image" instead of "HTMLImageElement", here.
> "{{named_constructor.name}}" should be "Image", I think.

Done! :) This fixes the problem.

https://codereview.chromium.org/2647643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:555:
result->Inherit({{v8_class}}::domTemplate(isolate, world));
On 2017/02/23 at 06:26:10, Yuki(ooo_til_feb27) wrote:
> IIUC, now we don't need this line, because you set "prototype" property in
NamedConstructorAttributeGetter.  Could you remove this line if the removal
doesn't cause any problem?

When I remove this I get the runtime error:

[7903:7945:0224/140232.765122:2754782287949:FATAL:V8GCController.cpp(110)] Check
failed: V8Node::hasInstance(wrapper, m_isolate). 
#0 0x7f0e956e4517 base::debug::StackTrace::StackTrace()
#1 0x7f0e9570887b logging::LogMessage::~LogMessage()
#2 0x7f0e90dd3a2f
blink::MinorGCUnmodifiedWrapperVisitor::VisitPersistentHandle()
#3 0x7f0e92f16db2 v8::VisitorAdapter::VisitEmbedderReference()
#4 0x7f0e9333a5c3
v8::internal::GlobalHandles::IterateWeakRootsInNewSpaceWithClassIds()
#5 0x7f0e92f0d17c v8::Isolate::VisitWeakHandles()
#6 0x7f0e90dd0ac7 blink::V8GCController::gcPrologue()
#7 0x7f0e93349f38 v8::internal::Heap::CallGCPrologueCallbacks()
...

So I think it's still needed! :)

[sashab, 2017-02-28 01:16:26]

Ping yukishiino :)

[bashi, 2017-02-28 01:22:31]

yukishiino@ was ooo yesterday and he may be ooo today too. I think you can
submit this :)

[sashab, 2017-02-28 01:24:04]

Thanks bashi! :)

[sashab, 2017-02-28 01:24:09]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-02-28 01:24:10]

The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org,
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2647643002/#ps120001
(title: "Fixed named constructor name")

[sashab, 2017-02-28 01:24:14]

The CQ bit was unchecked by sashab@chromium.org

[sashab, 2017-02-28 01:24:32]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-02-28 01:24:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, bashi@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2647643002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-28 01:25:18]

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, 2017-02-28 03:20:53]

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

[commit-bot: I haz the power, 2017-02-28 03:20:54]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sashab, 2017-02-28 03:51:44]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2017-02-28 03:52:21]

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, 2017-02-28 05:30:24]

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

[commit-bot: I haz the power, 2017-02-28 05:30:25]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sashab, 2017-02-28 05:40:13]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2017-02-28 05:40:31]

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, 2017-02-28 06:58:12]

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

[commit-bot: I haz the power, 2017-02-28 06:58:13]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bashi, 2017-02-28 07:04:22]

On 2017/02/28 06:58:13, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

The failures seems real. sashab@, could you run layout tests locally?

[Yuki, 2017-03-01 06:58:50]

On 2017/02/28 07:04:22, bashi wrote:
> On 2017/02/28 06:58:13, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> The failures seems real. sashab@, could you run layout tests locally?

https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...

These failures are expected results.
sashab@, you just need to update the test expectations.
The failures are just because you fixed it!!

(BTW, I may be ooo tomorrow and/or the day after next...)

[sashab, 2017-03-01 23:57:02]

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

[commit-bot: I haz the power, 2017-03-01 23:57:52]

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

[sashab, 2017-03-01 23:58:59]

Thanks yukishiino; I rebaselined the test expectations. But now I have concerns
about the results...

bashi/haraken do you understand this code enough to understand the changed test
expectations? Otherwise I'll wait for yukishiino to get back :)

https://codereview.chromium.org/2647643002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-img-element/Image-constructor-expected.txt
(left):

https://codereview.chromium.org/2647643002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-img-element/Image-constructor-expected.txt:1:
This is a testharness.js-based test.
Since this all passes now, can delete this test.

https://codereview.chromium.org/2647643002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/custom/document-register-reentrant-returning-fake-expected.txt
(right):

https://codereview.chromium.org/2647643002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/custom/document-register-reentrant-returning-fake-expected.txt:6:
Constructor object isn't created.
Is this correct? This seems like a bug.

https://codereview.chromium.org/2647643002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/2647643002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt:3016:
attribute @@toStringTag
What is all this extra stuff? Is this expected? :)

[bashi, 2017-03-02 00:21:52]

https://codereview.chromium.org/2647643002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/custom/document-register-reentrant-returning-fake-expected.txt
(right):

https://codereview.chromium.org/2647643002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/custom/document-register-reentrant-returning-fake-expected.txt:6:
Constructor object isn't created.
On 2017/03/01 23:58:59, sashab wrote:
> Is this correct? This seems like a bug.

I think this is ok as the line 1 says "PASS unless crash" :)

https://codereview.chromium.org/2647643002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/2647643002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt:3016:
attribute @@toStringTag
On 2017/03/01 23:58:59, sashab wrote:
> What is all this extra stuff? Is this expected? :)

I think this is expected but yukishiino@ should confirm.

[commit-bot: I haz the power, 2017-03-02 02:36:01]

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

[commit-bot: I haz the power, 2017-03-02 02:36:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yuki, 2017-03-02 05:30:16]

LGTM.

https://codereview.chromium.org/2647643002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/2647643002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt:3016:
attribute @@toStringTag
On 2017/03/02 00:21:52, bashi wrote:
> On 2017/03/01 23:58:59, sashab wrote:
> > What is all this extra stuff? Is this expected? :)
> 
> I think this is expected but yukishiino@ should confirm.

Yes, this is expected.  IIRC, we're printing out all the properties of
<Interface>.prototype (Image.prototype in this  case) in order to follow the
changes on exposed APIs.

This CL makes Image.prototype identical to HTMLImageElement.prototype, so we
should see the completely same list as HTMLImageElement (line 2024).

[sashab, 2017-03-02 05:54:25]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-03-02 05:54:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, bashi@chromium.org
Link to the patchset: https://codereview.chromium.org/2647643002/#ps160001
(title: "Rebaselined tests")

[commit-bot: I haz the power, 2017-03-02 05:55:33]

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, 2017-03-02 06:04:02]

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

[commit-bot: I haz the power, 2017-03-02 06:04:04]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sashab, 2017-03-02 06:16:20]

Hmm:

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
   
third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt

Missing LGTM from an OWNER for these files:
   
third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt

Who is an OWNER for these files? :/

[sof, 2017-03-02 06:19:41]

On 2017/03/02 06:16:20, sashab wrote:
> Hmm:
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>    
>
third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt
> 
> Missing LGTM from an OWNER for these files:
>    
>
third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt
> 
> Who is an OWNER for these files? :/

See LayoutTests/virtual/stable/webexposed/OWNERS

[sashab, 2017-03-02 06:21:36]

sashab@chromium.org changed reviewers:
+ tkent@chromium.org

[sashab, 2017-03-02 06:21:37]

tkent RS please on virtual/stable/* :)

[tkent, 2017-03-02 06:32:30]

This CL needs to update
LayoutTests/platform/mac/virtual/stable/webexposed/global-interface-listing-expected.txt
too.

https://codereview.chromium.org/2647643002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/named-constructor-prototype-chain.html
(right):

https://codereview.chromium.org/2647643002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/named-constructor-prototype-chain.html:1:

Please do not add new test to fast/dom/.

Is
LayoutTests/external/wpt/html/semantics/embedded-content/the-img-element/Image-constructor.html
not enough? If not, please add this test to somewhere in LayoutTests/html/ or
LayoutTests/external/wpt/html/.

[sashab, 2017-03-07 05:07:47]

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

[commit-bot: I haz the power, 2017-03-07 05:08:47]

Dry run: 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, 2017-03-07 06:30:04]

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

[commit-bot: I haz the power, 2017-03-07 06:30:05]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sashab, 2017-03-08 03:26:42]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-03-08 03:26:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, bashi@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2647643002/#ps200001
(title: "Test rebase fix")

[sashab, 2017-03-08 03:26:56]

Thanks tkent; did both those things. :)

[sashab, 2017-03-08 03:26:59]

The CQ bit was unchecked by sashab@chromium.org

[sashab, 2017-03-08 03:27:08]

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

[commit-bot: I haz the power, 2017-03-08 03:27:37]

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

[tkent, 2017-03-08 03:38:17]

lgtm

[sashab, 2017-03-08 03:38:44]

The CQ bit was unchecked by sashab@chromium.org

[sashab, 2017-03-08 03:38:47]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2017-03-08 03:39:16]

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, 2017-03-08 04:11:15]

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

[commit-bot: I haz the power, 2017-03-08 04:11:17]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sashab, 2017-03-09 01:05:23]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-03-09 01:05:24]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
haraken@chromium.org, bashi@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2647643002/#ps220001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-09 01:06:07]

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, 2017-03-09 03:59:56]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1489021523768900,
"parent_rev": "b93e0aca8b783c2fce824dce4b340c5ed7d2108a", "commit_rev":
"a47c72562d1182fb1ee8c62cbc252f9965103d53"}

[commit-bot: I haz the power, 2017-03-09 04:00:39]

Description was changed from

==========
Fix V8 bindings for named constructors to set prototype object correctly

Fix V8 bindings for named constructors to set prototype object correctly by
explicitly setting the prototype of the object after constructing the Function.

Also adds a V8PrivateProperty to store whether the prototype has been set, so
that it is only set once per object.

BUG=310776
==========

to

==========
Fix V8 bindings for named constructors to set prototype object correctly

Fix V8 bindings for named constructors to set prototype object correctly by
explicitly setting the prototype of the object after constructing the Function.

Also adds a V8PrivateProperty to store whether the prototype has been set, so
that it is only set once per object.

BUG=310776

Review-Url: https://codereview.chromium.org/2647643002
Cr-Commit-Position: refs/heads/master@{#455659}
Committed:
https://chromium.googlesource.com/chromium/src/+/a47c72562d1182fb1ee8c62cbc25...
==========

[commit-bot: I haz the power, 2017-03-09 04:00:43]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/a47c72562d1182fb1ee8c62cbc25...