Implement custom element construction and some 'define' checks

Implement custom element construction and some 'define' checks

BUG=594918
TEST=LayoutTests/custom-elements

Committed: https://crrev.com/00c4f05945de64e240b230db5b22536be02d09e7
Cr-Commit-Position: refs/heads/master@{#393469}

[dominicc (has gone to gerrit), 2016-05-05 06:14:14]

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

[dominicc (has gone to gerrit), 2016-05-05 06:17:23]

Not ready for review yet, but FYI and feedback is always appreciated.

domenic, justinfagnani: I'd like your feedback on the layout tests.
kojii: Everything else. After I have polished a bit I will add a V8 bindings
expert too.

TL;DR this makes the following work:

class X extends HTMLElement {
  constructor() {
    super();
    // do stuff
  }
  // other regular JavaScript stuff
}
window.customElements.define('some-thing', X);
new X();

Nothing else works ;)

[justinfagnani, 2016-05-05 17:04:47]

justinfagnani@google.com changed reviewers:
+ justinfagnani@google.com

[justinfagnani, 2016-05-05 17:04:48]

https://codereview.chromium.org/1952893003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/custom-elements/spec/construct.html (right):

https://codereview.chromium.org/1952893003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/custom-elements/spec/construct.html:34:
assert_true(e instanceof w.HTMLElement,
might want to verify the localName

[domenic, 2016-05-05 19:15:28]

This is how I feel about this patch: http://i.imgur.com/dfe4ZGi.gif

https://codereview.chromium.org/1952893003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/custom-elements/spec/construct.html (right):

https://codereview.chromium.org/1952893003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/custom-elements/spec/construct.html:6: <script
src="resources/custom-elements-helpers.js"></script>
In preparation for upstreaming might want to add <meta name="author"> and <link
rel="help"> (see e.g. https://github.com/w3c/web-platform-tests/pull/2940/files)

https://codereview.chromium.org/1952893003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/custom-elements/spec/construct.html:34:
assert_true(e instanceof w.HTMLElement,
Other ideas:

e.namespaceURI === "http://www.w3.org/1999/xhtml"
e.constructor === C
e.prefix === null
e.ownerDocument === w.ownerDocument (NOT window.ownerDocument)

https://codereview.chromium.org/1952893003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html
(right):

https://codereview.chromium.org/1952893003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:11: 
Koji wrote a bunch of tests for this already:
https://github.com/w3c/web-platform-tests/pull/2940

They are generally a bit more complete. I will comment on anything I see here
that is not tested by his tests.

https://codereview.chromium.org/1952893003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/custom-elements/spec/define-element.html:66:
test_with_window((w) => {
His tests did not generally test the order of the steps in the constructor like
this one does. You could get really detailed here, e.g. by defining getters for
{observedAttributes,connectedCallback,disconnectedCallback,attributeChangedCallback}
you could test that the order those are retrieved are all per-spec.

https://codereview.chromium.org/1952893003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/custom-elements/spec/resources/custom-elements-helpers.js
(right):

https://codereview.chromium.org/1952893003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/custom-elements/spec/resources/custom-elements-helpers.js:15:
function async_test_with_window(f, name) {
In general async_test is worse than promise_test; might want to create a
different variant. Seems unused in this CL though.

[dominicc (has gone to gerrit), 2016-05-09 12:40:51]

Description was changed from

==========
[WIP] Implement custom element construction and some 'define' checks.

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

to

==========
Implement custom element construction and some 'define' checks

BUG=594918
TEST=LayoutTests/custom-elements
==========

[dominicc (has gone to gerrit), 2016-05-09 12:40:51]

dominicc@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org
- kojii@chromium.org

[dominicc (has gone to gerrit), 2016-05-09 12:55:15]

- kojii for now because he's away at a CSS meeting.
+ esprehn for everything; would you mind taking a look at this?
+ haraken for bindings.

This basically makes the following work, and nothing more:

class MyElement extends HTMLElement {
  constructor() {
    super();
    console.log('hello, world');
  }
}
window.customElements.define('my-element', MyElement);
new MyElement();

This is a bit of a change of direction from 'classic' custom elements which
tried hard to hide V8 from DOM. That never really paid off; the only
implementation is the V8 one. Native Dart bindings are no longer a thing. Custom
elements have some ECMA-262isms in their definition. So this uses V8 directly.

There are obviously some large gaps in features, tests and documentation with
this. I just wanted to keep this patch a manageable size and enable some
implementation parallelism so this seemed like a good place to checkpoint. There
are actually a number of details working here:

- Making HTMLElement behave the way it always has, throwing TypeError, except
when constructing a custom element.
- GC and not leaking prototypes when frames are torn down (holding onto
constructors will also be necessary later).
- Disallowing custom element definition from isolated worlds.
- Mostly disallowing overlapping document.registerElement and
window.customElements.define, although the problem is a bit complicated by the
registry-sharing of document.registerElement.

PTAL

[dominicc (has gone to gerrit), 2016-05-10 01:50:52]

dominicc@chromium.org changed reviewers:
+ tkent@chromium.org
- esprehn@chromium.org

[dominicc (has gone to gerrit), 2016-05-10 01:50:52]

-esprehn, guess he is busy; +tkent, PTAL

[tkent, 2016-05-10 03:59:05]

The patch contains many v8 usages.  haraken@ should review this.

Non-binding parts look ok to me.
nit: use DCHECK instead of ASSERT.

https://codereview.chromium.org/1952893003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1952893003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:67: //
TODO(dominicc): Flip this back to test; this is not ready yet.
Why do you change it?

[dominicc (has gone to gerrit), 2016-05-10 05:40:28]

On 2016/05/10 at 03:59:05, tkent wrote:
> The patch contains many v8 usages.  haraken@ should review this.
> 
> Non-binding parts look ok to me.
> nit: use DCHECK instead of ASSERT.

Done, thanks!

>
https://codereview.chromium.org/1952893003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):
> 
>
https://codereview.chromium.org/1952893003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:67: //
TODO(dominicc): Flip this back to test; this is not ready yet.
> Why do you change it?

Oops. Rolled it out. Just did it to play with it in Chromium during development.

PTAL

[haraken, 2016-05-10 09:04:22]

Sorry for the delayed review :/

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8ThrowException.h (right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ThrowException.h:57: static
v8::Local<v8::Value> throwIllegalConstructorTypeError(v8::Isolate*);

I'd prefer not adding this helper method since "IllegalConstructorTypeError" is
not a spec-ed error. Currently V8ThrowException is intended to provide methods
to throw spec-ed errors.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
(right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:57:
info.GetReturnValue().Set(wrapper);

Can we use v8SetReturnValue(info, wrapper)?

I'm a bit concerned that this constructorCustom is very hand-written. I hope
you'd copy & paste the generated code and modify only the part you really need
to edit. That way we can minimize a risk of causing bugs due to hand-written
bindings.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:65: if
(!couldSetPrototype.FromJust()) {

V8BindingMacros.h provides macros to avoid developers from using IsNothing,
FromJust etc. Can we replace these with the macro?

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:67: //
test for this.

You can use v8CallOrCrash. (In practice, it will never crash except OOM.)

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
(right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:23:
DCHECK(!object.IsEmpty());

Would you help me understand who guarantees that m_prototype is not empty?
m_prototype is a weak persistent handle, which may be cleared at any point.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h
(right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:21:
v8::Handle<v8::Object> prototype);

Use v8::Local. v8::Handle is deprecated.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:27: if
(!scriptState)

This shouldn't happen.

Also what you really need to check is:

  if (!scriptState->contextIsValid())
    return;

but it shouldn't happen either. Even if it happens it will hit the
ASSERT(scriptState->contextIsValid()) in ScriptState::Scope.

In summary, you can remove this check.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:29:
ScriptState::Scope scope(scriptState);

BTW, CustomElementsRegistry::create is called only by JS via the binding layer,
right? Then it's already guaranteed that you're in a correct ScriptState. So you
don't need to enter the ScriptState::Scope.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:30:
DCHECK(DOMWrapperWorld::current(scriptState->isolate()).isMainWorld());

Use scriptState->world().

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:43:
Document* doc = toDocument(currentExecutionContext(isolate));

Can we pass in a ScriptState to v0CustomElementIsDefined?

currentXXX methods have a risk of looking up a wrong thing unless you're careful
about all the call sites. To mitigate the risk, it's better to explicitly pass
in a ScriptState that should be used.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:53: if
(!DOMWrapperWorld::current(scriptState->isolate()).isMainWorld()) {

Use scriptState->world().

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:102:
v8::TryCatch tryCatch(isolate);

What is this TryCatch for?

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:103:
v8::MaybeLocal<v8::Value> maybePrototype = constructorObject->Get(

Avoid using MaybeLocal handles by using macros in V8BindingMacros.h.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:126:
m_constructorIdMap.Get(isolate)->Set(

Why is it okay to make the map a weak map? As far as I understand correctly,
this weak map is caching the constructor to avoid the overhead of instantiating
the constructor every time -- right?

If the constructor is exposed to JS, a user can set an expando on the
constructor object. If the map is weak, I'm afraid that the expand can be lost.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:137:
v8::Handle<v8::Context> context,

I'd prefer passing in a ScriptState instead of a v8::Context.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:144: return
m_definitions[entry->Uint32Value(context).FromJust()];

Use a v8Call macro in V8BindingMacros.h.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/V0CustomElementRegistry.cpp
(right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/V0CustomElementRegistry.cpp:47:
v8::Isolate* isolate = v8::Isolate::GetCurrent();

v8::Isolate::GetCurrent() is deprecated.

Can we pass a ScriptState from Document::registerElement down to this point?

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/V0CustomElementRegistry.cpp:49:
LocalDOMWindow* window = doc->domWindow();

We should use ScriptState::domWindow(), which does all necessary security
checks.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1328: ScriptState*
scriptState = ScriptState::forMainWorld(frame());

Can you add [CallWith=ScriptState] to Window.customElements and pass in the
ScriptState from the binding layer?

[yosin_UTC9, 2016-05-10 13:10:58]

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

[yosin_UTC9, 2016-05-10 13:10:59]

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
(right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:57:
info.GetReturnValue().Set(wrapper);
I think we don't need to set return value for constructor since constructor
doesn't return value.
Note: my v8 host works fine w/o setting return value.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:62:
wrapper->SetPrototype(context, def->prototype(isolate));
Can we use new.target.constructor.prototype? If so we can extend class implement
custom-element.

class X extends HTMLElement { ... } // from comment#1
class Y extends Y { .... } // extends X

Setting prototype from |def| makes (new Y()).__proto__ == X.__proto__


const auto& prototype = info.NewTarget().As<v8::Function>()->Get("prototype");
wrapper->SetPrototype(context, prototype);

It works fine for my v8 host. ;-)

BTW, I'm asking V8 does above:
https://bugs.chromium.org/p/v8/issues/detail?id=5001

[Yuki, 2016-05-11 01:59:01]

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

[Yuki, 2016-05-11 01:59:02]

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ExceptionState.h (right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ExceptionState.h:155: void
throwSyntaxError(const String& message = String()) override;
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
If this is not so important, I'd prefer not having the default argument.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ExceptionState.h:167: void
throwSyntaxError(const String& message = String()) override;
Ditto.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
(right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:56:
v8::Local<v8::Object> wrapper = element->wrap(isolate, info.Holder());
Please prefer using toV8() rather than wrap() method.
It's not well-documented, but toV8() is designed to be responsible for the
conversion from C++ DOM object to the corresponding V8 wrapper.  wrap() method
is designed to be used in toV8().

[dominicc (has gone to gerrit), 2016-05-11 02:31:48]

dominicc@chromium.org changed reviewers:
- yukishiino@chromium.org

[dominicc (has gone to gerrit), 2016-05-11 02:31:50]

Thanks so much for the awesome detailed feedback. I've implemented most of it
and made some inline replies.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
(right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:57:
info.GetReturnValue().Set(wrapper);
On 2016/05/10 at 13:10:59, Yosi_UTC9 wrote:
> I think we don't need to set return value for constructor since constructor
doesn't return value.
> Note: my v8 host works fine w/o setting return value.

We do in this case because we are returning a different value to 'this'.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:57:
info.GetReturnValue().Set(wrapper);
On 2016/05/10 at 09:04:22, haraken wrote:
> Can we use v8SetReturnValue(info, wrapper)?
> 
> I'm a bit concerned that this constructorCustom is very hand-written. I hope
you'd copy & paste the generated code and modify only the part you really need
to edit. That way we can minimize a risk of causing bugs due to hand-written
bindings.

Hmm, I actually did start with the HTMLElement constructor, and borrowed the
document lookup from Image (these should be consistent!)

I made some changes to use more ScriptState, PTAL.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:62:
wrapper->SetPrototype(context, def->prototype(isolate));
On 2016/05/10 at 13:10:59, Yosi_UTC9 wrote:
> Can we use new.target.constructor.prototype? If so we can extend class
implement custom-element.
> 
> class X extends HTMLElement { ... } // from comment#1
> class Y extends Y { .... } // extends X
> 
> Setting prototype from |def| makes (new Y()).__proto__ == X.__proto__
> 
> 
> const auto& prototype = info.NewTarget().As<v8::Function>()->Get("prototype");
> wrapper->SetPrototype(context, prototype);
> 
> It works fine for my v8 host. ;-)

I was wondering about this too; I filed
https://github.com/whatwg/html/issues/1229

> BTW, I'm asking V8 does above:
https://bugs.chromium.org/p/v8/issues/detail?id=5001

Looks like verwaest got back to you on the bug. I was surprised it didn't. We
have to set the prototype explicitly here per the spec so we don't really
benefit from this.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
(right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:23:
DCHECK(!object.IsEmpty());
On 2016/05/10 at 09:04:22, haraken wrote:
> Would you help me understand who guarantees that m_prototype is not empty?
m_prototype is a weak persistent handle, which may be cleared at any point.

CustomElementsRegistry::visitDOMWrapper ends up calling
CustomElementDefinition::setReference and keeping the prototype alive. That
should be enough?

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:27: if
(!scriptState)
On 2016/05/10 at 09:04:22, haraken wrote:
> This shouldn't happen.
> 
> Also what you really need to check is:
> 
>   if (!scriptState->contextIsValid())
>     return;
> 
> but it shouldn't happen either. Even if it happens it will hit the
ASSERT(scriptState->contextIsValid()) in ScriptState::Scope.
> 
> In summary, you can remove this check.

Hmm, what happens if a script removes its own window; is the context still
valid?

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:29:
ScriptState::Scope scope(scriptState);
On 2016/05/10 at 09:04:22, haraken wrote:
> BTW, CustomElementsRegistry::create is called only by JS via the binding
layer, right? Then it's already guaranteed that you're in a correct ScriptState.
So you don't need to enter the ScriptState::Scope.

This object is special: it sets up a map (NativeWeakMap) of constructors to
element names. That needs to live as long as an the main world can run script.
So I wanted to create that object in the main world. If I do this in the calling
ScriptState, if an extension called window.customElements and then the extension
isolated world was killed, what would happen when the main world manipulated the
map?

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:102:
v8::TryCatch tryCatch(isolate);
On 2016/05/10 at 09:04:22, haraken wrote:
> What is this TryCatch for?

I need all of the early returns to be terminating or throwing an exception. With

if (x) {
  exceptionState.throwX()
  return;
}

above it is easy to be confident that happens; but below if retrieving the
prototype fails I wanted to check that it is due to an exception.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:126:
m_constructorIdMap.Get(isolate)->Set(
On 2016/05/10 at 09:04:22, haraken wrote:
> Why is it okay to make the map a weak map? As far as I understand correctly,
this weak map is caching the constructor to avoid the overhead of instantiating
the constructor every time -- right?

Custom elements "v1" is a bit different: the constructor is supplied by the
author, not generated by Blink.

This map is used in the HTMLElement constructor to, given a constructor (via
new.target), find the definition to use for it. So we can look up things like
the tag name.

A WeakMap is a good object to use for this, instead of a private symbol, because
the constructor could have been frozen.

> If the constructor is exposed to JS, a user can set an expando on the
constructor object. If the map is weak, I'm afraid that the expand can be lost.

This does not happen because CustomElementsRegistry::visitDOMWrapper keeps the
constructor alive. It adds references to prototypes, which have a strong
reference to the constructor via the 'constructor' property.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1328: ScriptState*
scriptState = ScriptState::forMainWorld(frame());
On 2016/05/10 at 09:04:22, haraken wrote:
> Can you add [CallWith=ScriptState] to Window.customElements and pass in the
ScriptState from the binding layer?

We actually really need to construct that object in the main world; I put some
details in a reply to CustomElementsRegistry. It's about the map that object
maintains needs to be in the main world isolate.

[Yuki, 2016-05-11 03:21:03]

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

[Yuki, 2016-05-11 03:21:05]

https://codereview.chromium.org/1952893003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
(right):

https://codereview.chromium.org/1952893003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:16:
m_prototype.setWeak(this, &CustomElementDefinition::didCollectPrototype);
Can we use the new type of weak phantom handle here?
https://codereview.chromium.org/1944363002/

[dominicc (has gone to gerrit), 2016-05-11 03:41:40]

On 2016/05/11 at 03:21:05, yukishiino wrote:
>
https://codereview.chromium.org/1952893003/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
(right):
> 
>
https://codereview.chromium.org/1952893003/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:16:
m_prototype.setWeak(this, &CustomElementDefinition::didCollectPrototype);
> Can we use the new type of weak phantom handle here?
> https://codereview.chromium.org/1944363002/

Good point; this is neater. I've always liked The Phantom and the Phantom of the
Opera is just misunderstood really. Let's try it.

[yosin_UTC9, 2016-05-11 03:49:55]

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
(right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:57:
info.GetReturnValue().Set(wrapper);
On 2016/05/11 at 02:31:49, dominicc wrote:
> On 2016/05/10 at 13:10:59, Yosi_UTC9 wrote:
> > I think we don't need to set return value for constructor since constructor
doesn't return value.
> > Note: my v8 host works fine w/o setting return value.
> 
> We do in this case because we are returning a different value to 'this'.

AFIK, return value of constructor isn't used. I tried to return |undefined|
value, e.g. info.GetReturnValue().SetUndefined().
It still works.

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:62:
wrapper->SetPrototype(context, def->prototype(isolate));
On 2016/05/11 at 02:31:49, dominicc wrote:
> On 2016/05/10 at 13:10:59, Yosi_UTC9 wrote:
> > Can we use new.target.constructor.prototype? If so we can extend class
implement custom-element.
> > 
> > class X extends HTMLElement { ... } // from comment#1
> > class Y extends Y { .... } // extends X
> > 
> > Setting prototype from |def| makes (new Y()).__proto__ == X.__proto__
> > 
> > 
> > const auto& prototype =
info.NewTarget().As<v8::Function>()->Get("prototype");
> > wrapper->SetPrototype(context, prototype);
> > 
> > It works fine for my v8 host. ;-)
> 
> I was wondering about this too; I filed
https://github.com/whatwg/html/issues/1229
> 
> > BTW, I'm asking V8 does above:
https://bugs.chromium.org/p/v8/issues/detail?id=5001
> 
> Looks like verwaest got back to you on the bug. I was surprised it didn't. We
have to set the prototype explicitly here per the spec so we don't really
benefit from this.

Can we change spec to pass tagName to super? If so, we don't need to have
customer constructor at all.

class X extends HTMLElement { constructor() { super('some-thing'); } }

[dominicc (has gone to gerrit), 2016-05-11 04:40:59]

On 2016/05/11 at 03:49:55, yosin wrote:
>
https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
(right):
> 
>
https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:57:
info.GetReturnValue().Set(wrapper);
> On 2016/05/11 at 02:31:49, dominicc wrote:
> > On 2016/05/10 at 13:10:59, Yosi_UTC9 wrote:
> > > I think we don't need to set return value for constructor since
constructor doesn't return value.
> > > Note: my v8 host works fine w/o setting return value.
> > 
> > We do in this case because we are returning a different value to 'this'.
> 
> AFIK, return value of constructor isn't used. I tried to return |undefined|
value, e.g. info.GetReturnValue().SetUndefined().
> It still works.

Read the ECMA-262. Returning undefined leaves this unchanged (the no-return case
works implicitly the same way); returning a different object substitutes that.

>
https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:62:
wrapper->SetPrototype(context, def->prototype(isolate));
> On 2016/05/11 at 02:31:49, dominicc wrote:
> > On 2016/05/10 at 13:10:59, Yosi_UTC9 wrote:
> > > Can we use new.target.constructor.prototype? If so we can extend class
implement custom-element.
> > > 
> > > class X extends HTMLElement { ... } // from comment#1
> > > class Y extends Y { .... } // extends X
> > > 
> > > Setting prototype from |def| makes (new Y()).__proto__ == X.__proto__
> > > 
> > > 
> > > const auto& prototype =
info.NewTarget().As<v8::Function>()->Get("prototype");
> > > wrapper->SetPrototype(context, prototype);
> > > 
> > > It works fine for my v8 host. ;-)
> > 
> > I was wondering about this too; I filed
https://github.com/whatwg/html/issues/1229
> > 
> > > BTW, I'm asking V8 does above:
https://bugs.chromium.org/p/v8/issues/detail?id=5001
> > 
> > Looks like verwaest got back to you on the bug. I was surprised it didn't.
We have to set the prototype explicitly here per the spec so we don't really
benefit from this.
> 
> Can we change spec to pass tagName to super? If so, we don't need to have
customer constructor at all.
> 
> class X extends HTMLElement { constructor() { super('some-thing'); } }

We need the tag names ahead of time for parsing. See Domenic's feedback on the
spec issue I filed.

[haraken, 2016-05-11 06:25:28]

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:126:
m_constructorIdMap.Get(isolate)->Set(
On 2016/05/11 02:31:49, dominicc wrote:
> On 2016/05/10 at 09:04:22, haraken wrote:
> > Why is it okay to make the map a weak map? As far as I understand correctly,
> this weak map is caching the constructor to avoid the overhead of
instantiating
> the constructor every time -- right?
> 
> Custom elements "v1" is a bit different: the constructor is supplied by the
> author, not generated by Blink.
> 
> This map is used in the HTMLElement constructor to, given a constructor (via
> new.target), find the definition to use for it. So we can look up things like
> the tag name.
> 
> A WeakMap is a good object to use for this, instead of a private symbol,
because
> the constructor could have been frozen.
> 
> > If the constructor is exposed to JS, a user can set an expando on the
> constructor object. If the map is weak, I'm afraid that the expand can be
lost.
> 
> This does not happen because CustomElementsRegistry::visitDOMWrapper keeps the
> constructor alive. It adds references to prototypes, which have a strong
> reference to the constructor via the 'constructor' property.

If you're expecting that the constructor is kept alive in some other way, how
about just using a strong reference here as well? I don't see a benefit of using
a weak map.

(Note that this is the only place in v8 bindings that uses v8::NativeWeakMap.)

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1328: ScriptState*
scriptState = ScriptState::forMainWorld(frame());
On 2016/05/11 02:31:49, dominicc wrote:
> On 2016/05/10 at 09:04:22, haraken wrote:
> > Can you add [CallWith=ScriptState] to Window.customElements and pass in the
> ScriptState from the binding layer?
> 
> We actually really need to construct that object in the main world; I put some
> details in a reply to CustomElementsRegistry. It's about the map that object
> maintains needs to be in the main world isolate.

It's nasty to manipulate V8 objects in a different world... (It has a risk of
leaking wrappers between worlds.)

Would it be an option to simply disallow the isolated world to access
customElements until you implement per-world CustomElementsRegistry correctly?

[dominicc (has gone to gerrit), 2016-05-11 09:10:20]

Thanks for so many quick rounds of review--super helpful. I see the
simplifications you're hinting at.

On 2016/05/11 at 06:25:28, haraken wrote:
>
https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):
> 
>
https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:126:
m_constructorIdMap.Get(isolate)->Set(
> On 2016/05/11 02:31:49, dominicc wrote:
> > On 2016/05/10 at 09:04:22, haraken wrote:
> > > Why is it okay to make the map a weak map? As far as I understand
correctly,
> > this weak map is caching the constructor to avoid the overhead of
instantiating
> > the constructor every time -- right?
> > 
> > Custom elements "v1" is a bit different: the constructor is supplied by the
> > author, not generated by Blink.
> > 
> > This map is used in the HTMLElement constructor to, given a constructor (via
> > new.target), find the definition to use for it. So we can look up things
like
> > the tag name.
> > 
> > A WeakMap is a good object to use for this, instead of a private symbol,
because
> > the constructor could have been frozen.
> > 
> > > If the constructor is exposed to JS, a user can set an expando on the
> > constructor object. If the map is weak, I'm afraid that the expand can be
lost.
> > 
> > This does not happen because CustomElementsRegistry::visitDOMWrapper keeps
the
> > constructor alive. It adds references to prototypes, which have a strong
> > reference to the constructor via the 'constructor' property.
> 
> If you're expecting that the constructor is kept alive in some other way, how
about just using a strong reference here as well? I don't see a benefit of using
a weak map.
> 
> (Note that this is the only place in v8 bindings that uses v8::NativeWeakMap.)

I need to look up this entry based on the identity of the constructor, so I need
to use some kind of map here to do it in O(1). So there are three choices: Map,
WeakMap, or NativeWeakMap.

I've swapped to Map; it turns out this is neater because I can keep it alone
alive with visitDOMWrapper and the prototypes stay alive via the map keys (which
are constructors whose prototype property points to the prototype.) I've added
some comments and documentation about memory management.

>
https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):
> 
>
https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1328: ScriptState*
scriptState = ScriptState::forMainWorld(frame());
> On 2016/05/11 02:31:49, dominicc wrote:
> > On 2016/05/10 at 09:04:22, haraken wrote:
> > > Can you add [CallWith=ScriptState] to Window.customElements and pass in
the
> > ScriptState from the binding layer?
> > 
> > We actually really need to construct that object in the main world; I put
some
> > details in a reply to CustomElementsRegistry. It's about the map that object
> > maintains needs to be in the main world isolate.
> 
> It's nasty to manipulate V8 objects in a different world... (It has a risk of
leaking wrappers between worlds.)

Hmm, I don't think we are manipulating V8 objects in a different world. The C++
object has a persistent to a map that is in the main world. That map is never
leaked to script. (In fact, NativeWeakMaps can't be passed to script; they are
Data, not Values. It's a map now, but still not passed to script.)

The keys in that map are constructors. Those constructors always come from the
main world. (Because they are put in the map in CustomElementsRegistry::define,
and you can see it checks that it is called from the main world.)

So you see ensuring the map is created in the main world is how I ensure there
are *not* references between worlds--the map and the constructor keys are all in
the main world. (The map values are ints, also in the main world, not very
interesting.)

The only thing that might be cross-world is when
V8CustomElementsRegistry::visitDOMWrapper is called with an isolated world
wrapper, we will set a temporary reference to the main world map. But the
ScriptPromise stuff does that, so apparently that is OK/correct? 

> Would it be an option to simply disallow the isolated world to access
customElements until you implement per-world CustomElementsRegistry correctly?

It is possible we will never enable custom elements for isolated worlds at all.
(We never did for "V0".) It's complicated, not only because of wrappers, but
also for semantics. For example, does the CSS :defined property apply if a
custom element is defined in an isolated world? Then what would
window.customElements.get(element.localName) return in that case? It's hard to
provide a consistent system for extensions, and there has not been a lot of
demand for it.

So from that point of view I guess it's better to not return the
CustomElementsRegistry at all in isolated worlds. I've done that, and it is a
good deal simpler.

PTAL

[Yuki, 2016-05-11 09:29:53]

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ExceptionState.h (right):

https://codereview.chromium.org/1952893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ExceptionState.h:155: void
throwSyntaxError(const String& message = String()) override;
On 2016/05/11 01:59:02, Yuki wrote:
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
> If this is not so important, I'd prefer not having the default argument.

ping?

[haraken, 2016-05-11 11:30:09]

https://codereview.chromium.org/1952893003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h
(right):

https://codereview.chromium.org/1952893003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:24:
v8::Local<v8::Object> prototype(v8::Isolate*) const;

Does the performance of prototype() matter?

If not, I'd prefer removing the weak ScopedPersistent. Alternately, you can add
a Member<CustomElementsRegistry>. Then prototype() can look up the prototype
object via the CustomElementsRegistry.

https://codereview.chromium.org/1952893003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):

https://codereview.chromium.org/1952893003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:122:
v8CallOrCrash(m_constructorIdMap.newLocal(isolate)->Set(

It would be nicer to use a hidden value (i.e., V8HiddenValue) on the constructor
to remember the id. Then you won't need to m_constructorIdMap. You can remove
the visitDOMWrapper.

https://codereview.chromium.org/1952893003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:143: return
m_definitions[id];

I'd add m_id to CustomElementDefinition and add DCHECK(m_definitions[id]->id()
== id).

[yosin_UTC9, 2016-05-12 01:30:34]

https://codereview.chromium.org/1952893003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
(right):

https://codereview.chromium.org/1952893003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:59:
v8::Local<v8::Object> wrapper =
Just curiosity, is v8SetReturnValue(info, <C++ pointer>);
info.GetReturnValue().Get() a common pattern for converting <C++ pointer> to v8
wrapper in Blink?

https://codereview.chromium.org/1952893003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:65:
wrapper->SetPrototype(scriptState->context(), def->prototype(isolate))))
Is info.GetNewTarget().GetProperty("prototype") != def->prototype(isolate)?

[bashi, 2016-05-12 02:03:24]

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

[bashi, 2016-05-12 02:03:25]

https://codereview.chromium.org/1952893003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
(right):

https://codereview.chromium.org/1952893003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:59:
v8::Local<v8::Object> wrapper =
On 2016/05/12 01:30:34, Yosi_UTC9 wrote:
> Just curiosity, is v8SetReturnValue(info, <C++ pointer>);
> info.GetReturnValue().Get() a common pattern for converting <C++ pointer> to
v8
> wrapper in Blink?

No. Try toV8() if you need to do. Basically the binding layer should care of C++
<-> JS conversion though.

[haraken, 2016-05-12 02:13:58]

https://codereview.chromium.org/1952893003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
(right):

https://codereview.chromium.org/1952893003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:59:
v8::Local<v8::Object> wrapper =
On 2016/05/12 02:03:25, bashi1 wrote:
> On 2016/05/12 01:30:34, Yosi_UTC9 wrote:
> > Just curiosity, is v8SetReturnValue(info, <C++ pointer>);
> > info.GetReturnValue().Get() a common pattern for converting <C++ pointer> to
> v8
> > wrapper in Blink?
> 
> No. Try toV8() if you need to do. Basically the binding layer should care of
C++
> <-> JS conversion though.

Please do the same thing as the generated code is doing.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

toV8() won't work here because what you want to do here is not to create a new
wrapper but to decorate a given V8 object with wrapper functionalities (i.e.,
call associateWithWrapper).

[bashi, 2016-05-12 02:15:18]

https://codereview.chromium.org/1952893003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
(right):

https://codereview.chromium.org/1952893003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:59:
v8::Local<v8::Object> wrapper =
On 2016/05/12 02:13:58, haraken wrote:
> On 2016/05/12 02:03:25, bashi1 wrote:
> > On 2016/05/12 01:30:34, Yosi_UTC9 wrote:
> > > Just curiosity, is v8SetReturnValue(info, <C++ pointer>);
> > > info.GetReturnValue().Get() a common pattern for converting <C++ pointer>
to
> > v8
> > > wrapper in Blink?
> > 
> > No. Try toV8() if you need to do. Basically the binding layer should care of
> C++
> > <-> JS conversion though.
> 
> Please do the same thing as the generated code is doing.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> toV8() won't work here because what you want to do here is not to create a new
> wrapper but to decorate a given V8 object with wrapper functionalities (i.e.,
> call associateWithWrapper).

I thought he asked a common pattten, not this specific case :)

[dominicc (has gone to gerrit), 2016-05-12 04:58:17]

On 2016/05/12 at 01:30:34, yosin wrote:
>
https://codereview.chromium.org/1952893003/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
(right):
> 
>
https://codereview.chromium.org/1952893003/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:59:
v8::Local<v8::Object> wrapper =
> Just curiosity, is v8SetReturnValue(info, <C++ pointer>);
info.GetReturnValue().Get() a common pattern for converting <C++ pointer> to v8
wrapper in Blink?

No; I'm cleaning this up.

>
https://codereview.chromium.org/1952893003/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:65:
wrapper->SetPrototype(scriptState->context(), def->prototype(isolate))))
> Is info.GetNewTarget().GetProperty("prototype") != def->prototype(isolate)?

That may or may not be true. It should be true for class constructors; it may be
true for other functions but there is no guarantee.

[dominicc (has gone to gerrit), 2016-05-12 07:08:33]

Thank you all for the advice and attention to this review; I think this is ready
for another look. PTAL.

[Yuki, 2016-05-12 07:33:27]

lgtm on my side.

[yosin_UTC9, 2016-05-12 07:54:23]

https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
(right):

https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:70:
return;
Do you forget |V8ThrowException| here?

[dominicc (has gone to gerrit), 2016-05-12 08:00:40]

https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
(right):

https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:70:
return;
On 2016/05/12 at 07:54:22, Yosi_UTC9 wrote:
> Do you forget |V8ThrowException| here?

I'm not sure when that would fail, actually, because we're using a new wrapper
here. I just put the return in there because there will be steps which come
after this.

[bashi, 2016-05-12 08:19:13]

https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp
(right):

https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:70:
return;
On 2016/05/12 08:00:40, dominicc wrote:
> On 2016/05/12 at 07:54:22, Yosi_UTC9 wrote:
> > Do you forget |V8ThrowException| here?
> 
> I'm not sure when that would fail, actually, because we're using a new wrapper
> here. I just put the return in there because there will be steps which come
> after this.

I'm not sure if this would fail but there will be an exception when
v8CallBoolean() returns false because that's the convention of v8 APIs which
returns Maybe<>. I don't think that you need to throw a new exception here.

[haraken, 2016-05-12 08:19:41]

https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):

https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:38:
.As<v8::Object>();

Add DCHECK(!wrapper.IsEmpty()).

https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:75:
exceptionState.throwDOMException(

Do you want to throw a DOM exception, not JS's syntax error?

https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:129:
v8CallOrCrash(map->Set(context, idValue, prototype));

Hmm, would you help me understand why you cannot simply add a hidden reference
from constructor to idValue? I want to understand why you need a v8::Map.

https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:167: return
V8HiddenValue::getHiddenValueFromMainWorldWrapper(

getHiddenValueFromMainWorldWrapper => getHiddenValue

(getHiddenValueFromMainWorldWrapper is a method only for some hacky cases.)

[dominicc (has gone to gerrit), 2016-05-12 08:52:24]

On 2016/05/12 at 08:19:41, haraken wrote:
>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):
> 
>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:38:
.As<v8::Object>();
> 
> Add DCHECK(!wrapper.IsEmpty()).

Should I do that? Because V8 will crash dereferencing the empty handle a few
lines from here.

>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:75:
exceptionState.throwDOMException(
> 
> Do you want to throw a DOM exception, not JS's syntax error?

Yes; turns out I was wrong before and the spec has clarified "SyntaxErr"
DOMException:

https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregis...

>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:129:
v8CallOrCrash(map->Set(context, idValue, prototype));
> 
> Hmm, would you help me understand why you cannot simply add a hidden reference
from constructor to idValue? I want to understand why you need a v8::Map.

I need to do four things:

1. Given a constructor, find out which element definition it is associated with
(if any.)
2. Given a definition, find the prototype retrieved for it during 'define'.
3. Keep constructors alive as long as window can run script.
4. Keep prototypes alive that long, too.

Having a hidden value on the constructor that gives the ID of the definition
solves #1, but then I would need to do something else to solve 2, 3, 4; probably
by using a Set (2, 3) and something (ScopedPersistent?) from the definition to
the prototype (4).

That all sounded a bit complicated, and I got the impression you don't want me
to use weak ScopedPersistent. If I was going to have to use a Set anyway, I
thought it might be simpler to keep everything in a Map.

>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:167:
return V8HiddenValue::getHiddenValueFromMainWorldWrapper(
> 
> getHiddenValueFromMainWorldWrapper => getHiddenValue
> 
> (getHiddenValueFromMainWorldWrapper is a method only for some hacky cases.)

OK

[dominicc (has gone to gerrit), 2016-05-12 08:54:52]

On 2016/05/12 at 08:52:24, dominicc wrote:
> On 2016/05/12 at 08:19:41, haraken wrote:
> >
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):
> > 
> >
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:38:
.As<v8::Object>();
> > 
> > Add DCHECK(!wrapper.IsEmpty()).
> 
> Should I do that? Because V8 will crash dereferencing the empty handle a few
lines from here.
> 
> >
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:75:
exceptionState.throwDOMException(
> > 
> > Do you want to throw a DOM exception, not JS's syntax error?
> 
> Yes; turns out I was wrong before and the spec has clarified "SyntaxErr"
DOMException:
> 
>
https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregis...
> 
> >
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:129:
v8CallOrCrash(map->Set(context, idValue, prototype));
> > 
> > Hmm, would you help me understand why you cannot simply add a hidden
reference from constructor to idValue? I want to understand why you need a
v8::Map.
> 
> I need to do four things:
> 
> 1. Given a constructor, find out which element definition it is associated
with (if any.)
> 2. Given a definition, find the prototype retrieved for it during 'define'.
> 3. Keep constructors alive as long as window can run script.
> 4. Keep prototypes alive that long, too.
> 
> Having a hidden value on the constructor that gives the ID of the definition
solves #1, but then I would need to do something else to solve 2, 3, 4; probably
by using a Set (2, 3) and something (ScopedPersistent?) from the definition to
the prototype (4).

Oops, I meant using a Set (#3, #4) for lifetime and something from the
definition to the prototype (#2).

> That all sounded a bit complicated, and I got the impression you don't want me
to use weak ScopedPersistent. If I was going to have to use a Set anyway, I
thought it might be simpler to keep everything in a Map.
> 
> >
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:167:
return V8HiddenValue::getHiddenValueFromMainWorldWrapper(
> > 
> > getHiddenValueFromMainWorldWrapper => getHiddenValue
> > 
> > (getHiddenValueFromMainWorldWrapper is a method only for some hacky cases.)
> 
> OK

[haraken, 2016-05-12 13:40:10]

On 2016/05/12 08:54:52, dominicc wrote:
> On 2016/05/12 at 08:52:24, dominicc wrote:
> > On 2016/05/12 at 08:19:41, haraken wrote:
> > >
>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
> (right):
> > > 
> > >
>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:38:
> .As<v8::Object>();
> > > 
> > > Add DCHECK(!wrapper.IsEmpty()).
> > 
> > Should I do that? Because V8 will crash dereferencing the empty handle a few
> lines from here.
> > 
> > >
>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:75:
> exceptionState.throwDOMException(
> > > 
> > > Do you want to throw a DOM exception, not JS's syntax error?
> > 
> > Yes; turns out I was wrong before and the spec has clarified "SyntaxErr"
> DOMException:
> > 
> >
>
https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregis...
> > 
> > >
>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:129:
> v8CallOrCrash(map->Set(context, idValue, prototype));
> > > 
> > > Hmm, would you help me understand why you cannot simply add a hidden
> reference from constructor to idValue? I want to understand why you need a
> v8::Map.
> > 
> > I need to do four things:
> > 
> > 1. Given a constructor, find out which element definition it is associated
> with (if any.)
> > 2. Given a definition, find the prototype retrieved for it during 'define'.
> > 3. Keep constructors alive as long as window can run script.
> > 4. Keep prototypes alive that long, too.
> > 
> > Having a hidden value on the constructor that gives the ID of the definition
> solves #1, but then I would need to do something else to solve 2, 3, 4;
probably
> by using a Set (2, 3) and something (ScopedPersistent?) from the definition to
> the prototype (4).
> 
> Oops, I meant using a Set (#3, #4) for lifetime and something from the
> definition to the prototype (#2).

Thanks, I'm now on the same page.

Final question: Why do we need to add a reference from |idValue| to |prototype|?
This is essentially a reference from |constructor| to |prototype|, but it is
already stored as constructor.prototype. Also if someone rewrites
constructor.prototype to a new prototype object, you don't want to keep the old
prototype object alive, right? (Then you shouldn't add a reference from
|constructor| to |prototype| in the Map.)

[dominicc (has gone to gerrit), 2016-05-12 14:33:42]

This is by design; the spec for 'define' is to cache the prototype.
Presumably to avoid reentrancy and throwing from HTMLElement constructor if
prototype was a getter.

Re: ID, prototype; all of this will be changing when we have to look up by
element name and type extension. I was just hoping to get something basic
working to parallelize work with kojii when he's back next week.

If it's better to do it in one shot I can grow this a bit more and try to
cover it with unit tests.
On May 12, 2016 10:40 PM, <haraken@chromium.org> wrote:

> On 2016/05/12 08:54:52, dominicc wrote:
> > On 2016/05/12 at 08:52:24, dominicc wrote:
> > > On 2016/05/12 at 08:19:41, haraken wrote:
> > > >
> >
>
>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > > > File
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
> > (right):
> > > >
> > > >
> >
>
>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > > >
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:38:
> > .As<v8::Object>();
> > > >
> > > > Add DCHECK(!wrapper.IsEmpty()).
> > >
> > > Should I do that? Because V8 will crash dereferencing the empty handle
> a few
> > lines from here.
> > >
> > > >
> >
>
>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > > >
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:75:
> > exceptionState.throwDOMException(
> > > >
> > > > Do you want to throw a DOM exception, not JS's syntax error?
> > >
> > > Yes; turns out I was wrong before and the spec has clarified
> "SyntaxErr"
> > DOMException:
> > >
> > >
> >
>
>
https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregis...
> > >
> > > >
> >
>
>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > > >
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:129:
> > v8CallOrCrash(map->Set(context, idValue, prototype));
> > > >
> > > > Hmm, would you help me understand why you cannot simply add a hidden
> > reference from constructor to idValue? I want to understand why you need
> a
> > v8::Map.
> > >
> > > I need to do four things:
> > >
> > > 1. Given a constructor, find out which element definition it is
> associated
> > with (if any.)
> > > 2. Given a definition, find the prototype retrieved for it during
> 'define'.
> > > 3. Keep constructors alive as long as window can run script.
> > > 4. Keep prototypes alive that long, too.
> > >
> > > Having a hidden value on the constructor that gives the ID of the
> definition
> > solves #1, but then I would need to do something else to solve 2, 3, 4;
> probably
> > by using a Set (2, 3) and something (ScopedPersistent?) from the
> definition to
> > the prototype (4).
> >
> > Oops, I meant using a Set (#3, #4) for lifetime and something from the
> > definition to the prototype (#2).
>
> Thanks, I'm now on the same page.
>
> Final question: Why do we need to add a reference from |idValue| to
> |prototype|?
> This is essentially a reference from |constructor| to |prototype|, but it
> is
> already stored as constructor.prototype. Also if someone rewrites
> constructor.prototype to a new prototype object, you don't want to keep
> the old
> prototype object alive, right? (Then you shouldn't add a reference from
> |constructor| to |prototype| in the Map.)
>
>
>
> https://codereview.chromium.org/1952893003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dominicc (has gone to gerrit), 2016-05-12 14:33:42]

This is by design; the spec for 'define' is to cache the prototype.
Presumably to avoid reentrancy and throwing from HTMLElement constructor if
prototype was a getter.

Re: ID, prototype; all of this will be changing when we have to look up by
element name and type extension. I was just hoping to get something basic
working to parallelize work with kojii when he's back next week.

If it's better to do it in one shot I can grow this a bit more and try to
cover it with unit tests.
On May 12, 2016 10:40 PM, <haraken@chromium.org> wrote:

> On 2016/05/12 08:54:52, dominicc wrote:
> > On 2016/05/12 at 08:52:24, dominicc wrote:
> > > On 2016/05/12 at 08:19:41, haraken wrote:
> > > >
> >
>
>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > > > File
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
> > (right):
> > > >
> > > >
> >
>
>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > > >
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:38:
> > .As<v8::Object>();
> > > >
> > > > Add DCHECK(!wrapper.IsEmpty()).
> > >
> > > Should I do that? Because V8 will crash dereferencing the empty handle
> a few
> > lines from here.
> > >
> > > >
> >
>
>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > > >
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:75:
> > exceptionState.throwDOMException(
> > > >
> > > > Do you want to throw a DOM exception, not JS's syntax error?
> > >
> > > Yes; turns out I was wrong before and the spec has clarified
> "SyntaxErr"
> > DOMException:
> > >
> > >
> >
>
>
https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregis...
> > >
> > > >
> >
>
>
https://codereview.chromium.org/1952893003/diff/240001/third_party/WebKit/Sou...
> > > >
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:129:
> > v8CallOrCrash(map->Set(context, idValue, prototype));
> > > >
> > > > Hmm, would you help me understand why you cannot simply add a hidden
> > reference from constructor to idValue? I want to understand why you need
> a
> > v8::Map.
> > >
> > > I need to do four things:
> > >
> > > 1. Given a constructor, find out which element definition it is
> associated
> > with (if any.)
> > > 2. Given a definition, find the prototype retrieved for it during
> 'define'.
> > > 3. Keep constructors alive as long as window can run script.
> > > 4. Keep prototypes alive that long, too.
> > >
> > > Having a hidden value on the constructor that gives the ID of the
> definition
> > solves #1, but then I would need to do something else to solve 2, 3, 4;
> probably
> > by using a Set (2, 3) and something (ScopedPersistent?) from the
> definition to
> > the prototype (4).
> >
> > Oops, I meant using a Set (#3, #4) for lifetime and something from the
> > definition to the prototype (#2).
>
> Thanks, I'm now on the same page.
>
> Final question: Why do we need to add a reference from |idValue| to
> |prototype|?
> This is essentially a reference from |constructor| to |prototype|, but it
> is
> already stored as constructor.prototype. Also if someone rewrites
> constructor.prototype to a new prototype object, you don't want to keep
> the old
> prototype object alive, right? (Then you shouldn't add a reference from
> |constructor| to |prototype| in the Map.)
>
>
>
> https://codereview.chromium.org/1952893003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haraken, 2016-05-12 14:54:23]

Thanks, makes sense. LGTM.

https://codereview.chromium.org/1952893003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):

https://codereview.chromium.org/1952893003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:37:
toV8(registry, scriptState->context()->Global(), isolate)

toV8(registry, scriptState);

https://codereview.chromium.org/1952893003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:42:
DCHECK(V8HiddenValue::setHiddenValue(scriptState, wrapper, name, map));

Let's avoid writing code that has a side effect in DCHECK.

https://codereview.chromium.org/1952893003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:129:
v8CallOrCrash(map->Set(context, idValue, prototype));

Add a comment about the lifetime relationship.

https://codereview.chromium.org/1952893003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:167:
toV8(this, scriptState->context()->Global(), scriptState->isolate())

toV8(this, scriptState);

[tkent, 2016-05-12 23:03:00]

lgtm

[dominicc (has gone to gerrit), 2016-05-13 07:40:08]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-05-13 07:40:08]

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

[commit-bot: I haz the power, 2016-05-13 07:40:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1952893003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1952893003/280001

[commit-bot: I haz the power, 2016-05-13 08:11:12]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-05-13 08:13:25]

Description was changed from

==========
Implement custom element construction and some 'define' checks

BUG=594918
TEST=LayoutTests/custom-elements
==========

to

==========
Implement custom element construction and some 'define' checks

BUG=594918
TEST=LayoutTests/custom-elements

Committed: https://crrev.com/00c4f05945de64e240b230db5b22536be02d09e7
Cr-Commit-Position: refs/heads/master@{#393469}
==========

[commit-bot: I haz the power, 2016-05-13 08:13:27]

Patchset 15 (id:??) landed as
https://crrev.com/00c4f05945de64e240b230db5b22536be02d09e7
Cr-Commit-Position: refs/heads/master@{#393469}