Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1994093002:
Introduce CustomElementRegistry#get() method (Closed)
|
Created:
4 years, 7 months ago by yosin_UTC9 Modified:
4 years, 6 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, dominicc+watchlist_chromium.org, eae+blinkwatch, rwlbuis, sof, webcomponents-bugzilla_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce CustomElementRegistry#get() method
This patch introduces |CustomElementRegistry#get()| as specified in [1].
To return associated constructor by |get()|, this patch introduces
|getConsturcotr(ScriptState*)| member function to |CustomElementDefinition| as
pure virtual function and implements it in |ScriptCustomElementDefinition|.
[1] https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregistry-get
BUG=594918
TEST=LayoutTests/custom-elements/spec/custom-elements-registry/get.html
TEST=LayoutTests/webexposed/global-interface-listing.html
Committed: https://crrev.com/217bd0280d1a9cfd18ade5f2f3c744e8cfb0a0f4
Cr-Commit-Position: refs/heads/master@{#397351}
Patch Set 1 : 2016-05-19T17:33:00 #
Total comments: 4
Patch Set 2 : 2016-05-20T19:26:06 #Patch Set 3 : 2016-05-23T13:18:56 #Patch Set 4 : 2016-05-23T13:42:56 #
Total comments: 6
Patch Set 5 : 2016-05-30T15:13:58 #Patch Set 6 : 2016-05-31T14:40:56 #
Total comments: 8
Patch Set 7 : 2016-05-31T18:12:12 #
Total comments: 13
Patch Set 8 : 2016-06-01T11:06:28 #
Total comments: 6
Patch Set 9 : 2016-06-01T12:54:14 #Patch Set 10 : 2016-06-01T15:09:57 #Messages
Total messages: 50 (20 generated)
The CQ bit was checked by yosin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1994093002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994093002/1
Description was changed from ========== 2016-05-19T17:33:00 BUG= ========== to ========== Introduce CustomElementRegistry#get() method This patch introduces |CustomElementRegistry#get()| as specified in [1]. [1] http://w3c.github.io/webcomponents/spec/custom/#dom-customelementsregistry-get BUG=594918 TEST=TBD ==========
yosin@chromium.org changed reviewers: + dominicc@chromium.org, kojii@chromium.org
PTAL This patch isn't ready for commit since it has not test. Do we have tests for CustomElementRegistery#get()? Or, should I write one?
> Or, should I write one? The interface was changed since WebKit experimented, so they don't have tests. There's a WIP test on github: https://github.com/w3c/web-platform-tests/pull/2969 you could take it over, or take it to our repo first and upload later. https://codereview.chromium.org/1994093002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp (right): https://codereview.chromium.org/1994093002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:157: // http://w3c.github.io/webcomponents/spec/custom/#dom-customelementsregistry-get It's already old, should be: https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregis...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Oh btw, dominicc wrote a doc here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Please add a LayoutTest in custom-elements/spec. https://codereview.chromium.org/1994093002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp (right): https://codereview.chromium.org/1994093002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:164: v8::Local<v8::Context> context = scriptState->context(); You should CHECK that scriptState->world().isMainWorld(); maybe you could upgrade the DCHECK in idMap to work in release. The reason is, if it was not the main world, you would be returning a constructor in the main world to an isolated world which would be bad--those are different contexts. However IIRC the CustomElementsRegistry object can't be retrieved from isolated worlds, so a check is enough. But make function in release; it really should never happen. https://codereview.chromium.org/1994093002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:167: return ScriptValue(scriptState, v8CallOrCrash(map->Get(context, idValue))); I believe this is the prototype; not the constructor.
Description was changed from ========== Introduce CustomElementRegistry#get() method This patch introduces |CustomElementRegistry#get()| as specified in [1]. [1] http://w3c.github.io/webcomponents/spec/custom/#dom-customelementsregistry-get BUG=594918 TEST=TBD ========== to ========== Introduce CustomElementRegistry#get() method This patch introduces |CustomElementRegistry#get()| as specified in [1]. [1] http://w3c.github.io/webcomponents/spec/custom/#dom-customelementsregistry-get BUG=594918 TEST=TBD Need to update: webexposed/global-interface-listing.html ==========
Description was changed from ========== Introduce CustomElementRegistry#get() method This patch introduces |CustomElementRegistry#get()| as specified in [1]. [1] http://w3c.github.io/webcomponents/spec/custom/#dom-customelementsregistry-get BUG=594918 TEST=TBD Need to update: webexposed/global-interface-listing.html ========== to ========== Introduce CustomElementRegistry#get() method This patch introduces |CustomElementRegistry#get()| as specified in [1]. To return associated constructor by |get()|, this patch change the hidden map in JavaScript to map customer element id to an array holding constructor and prototype. [1] http://w3c.github.io/webcomponents/spec/custom/#dom-customelementsregistry-get BUG=594918 TEST=LayoutTests/custom-elements/spec/custom-elements-registry/get.html ==========
Description was changed from ========== Introduce CustomElementRegistry#get() method This patch introduces |CustomElementRegistry#get()| as specified in [1]. To return associated constructor by |get()|, this patch change the hidden map in JavaScript to map customer element id to an array holding constructor and prototype. [1] http://w3c.github.io/webcomponents/spec/custom/#dom-customelementsregistry-get BUG=594918 TEST=LayoutTests/custom-elements/spec/custom-elements-registry/get.html ========== to ========== Introduce CustomElementRegistry#get() method This patch introduces |CustomElementRegistry#get()| as specified in [1]. To return associated constructor by |get()|, this patch change the hidden map in JavaScript to map customer element id to an array holding constructor and prototype. [1] https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregis... BUG=594918 TEST=LayoutTests/custom-elements/spec/custom-elements-registry/get.html ==========
PTAL https://codereview.chromium.org/1994093002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp (right): https://codereview.chromium.org/1994093002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:157: // http://w3c.github.io/webcomponents/spec/custom/#dom-customelementsregistry-get On 2016/05/19 at 10:04:32, kojii wrote: > It's already old, should be: > https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregis... Done
Sorry for the slow reply. This is looking pretty good; some comments inline. This will probably get churned a bit by splitting script and non-script stuff. I hope to have a WIP patch up soon. https://codereview.chromium.org/1994093002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/custom-elements/spec/custom-elements-registry/get.html (right): https://codereview.chromium.org/1994093002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/custom-elements-registry/get.html:11: <iframe id="iframe"></iframe> I think it is better to dynamically create these; it is easier to add more tests to the file and have an iframe for each of them. I think I checked in some tests which do that. https://codereview.chromium.org/1994093002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/custom-elements-registry/get.html:17: const testWindow = iframe.contentDocument.defaultView; iframe.contentWindow https://codereview.chromium.org/1994093002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/custom-elements-registry/get.html:19: let testable = false; I wonder what it would look like if testable was a promise, and then promise tests chained off that?
Who knows how that big split-up patch will go. However I think it is fine to add virtual ScriptValue getThing(ScriptState*) = 0 to CustomElementDefinition. If there are Web Module custom elements, they can decide what object they want to return to script. This can thunk to constructor() in ScriptCustomElementDefinition.
Description was changed from ========== Introduce CustomElementRegistry#get() method This patch introduces |CustomElementRegistry#get()| as specified in [1]. To return associated constructor by |get()|, this patch change the hidden map in JavaScript to map customer element id to an array holding constructor and prototype. [1] https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregis... BUG=594918 TEST=LayoutTests/custom-elements/spec/custom-elements-registry/get.html ========== to ========== Introduce CustomElementRegistry#get() method This patch introduces |CustomElementRegistry#get()| as specified in [1]. To return associated constructor by |get()|, this patch introduces |getConsturcotr(ScriptState*)| member function to |CustomElementDefinition| as pure virtual function and implements it in |ScriptCustomElementDefinition|. [1] https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregis... BUG=594918 TEST=LayoutTests/custom-elements/spec/custom-elements-registry/get.html ==========
Description was changed from ========== Introduce CustomElementRegistry#get() method This patch introduces |CustomElementRegistry#get()| as specified in [1]. To return associated constructor by |get()|, this patch introduces |getConsturcotr(ScriptState*)| member function to |CustomElementDefinition| as pure virtual function and implements it in |ScriptCustomElementDefinition|. [1] https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregis... BUG=594918 TEST=LayoutTests/custom-elements/spec/custom-elements-registry/get.html ========== to ========== Introduce CustomElementRegistry#get() method This patch introduces |CustomElementRegistry#get()| as specified in [1]. To return associated constructor by |get()|, this patch introduces |getConsturcotr(ScriptState*)| member function to |CustomElementDefinition| as pure virtual function and implements it in |ScriptCustomElementDefinition|. [1] https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregis... BUG=594918 TEST=LayoutTests/custom-elements/spec/custom-elements-registry/get.html TEST=LayoutTests/webexposed/global-interface-listing.html ==========
PTAL This patch introduces CustomElementDefinition::getConstructor(ScriptState*). Following patch will introduce getWhenDefinedPromise(ScriptState*). https://codereview.chromium.org/1994093002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/custom-elements/spec/custom-elements-registry/get.html (right): https://codereview.chromium.org/1994093002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/custom-elements-registry/get.html:11: <iframe id="iframe"></iframe> On 2016/05/25 at 01:31:23, dominicc wrote: > I think it is better to dynamically create these; it is easier to add more tests to the file and have an iframe for each of them. I think I checked in some tests which do that. Done https://codereview.chromium.org/1994093002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/custom-elements-registry/get.html:17: const testWindow = iframe.contentDocument.defaultView; On 2016/05/25 at 01:31:23, dominicc wrote: > iframe.contentWindow Done https://codereview.chromium.org/1994093002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/custom-elements-registry/get.html:19: let testable = false; On 2016/05/25 at 01:31:23, dominicc wrote: > I wonder what it would look like if testable was a promise, and then promise tests chained off that? Done
Could we get this rebased?
On 2016/05/31 at 05:00:38, dominicc wrote: > Could we get this rebased? PTAL rebased.
https://codereview.chromium.org/1994093002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h (right): https://codereview.chromium.org/1994093002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h:49: ScriptValue getConstructor(ScriptState*) final; Because ScriptCustomElementDefinition will need to vend callbacks, I think it should have RefPtr<ScriptState> and simply use that. https://codereview.chromium.org/1994093002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h (right): https://codereview.chromium.org/1994093002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:16: class ScriptValue; Is it possible to return by value without a complete definition? https://codereview.chromium.org/1994093002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:27: virtual ScriptValue getConstructor(ScriptState*) = 0; Write a C++ unit test that exercises this. https://codereview.chromium.org/1994093002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.idl (right): https://codereview.chromium.org/1994093002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.idl:9: [CallWith=ScriptState] I don't think we need CallWith=ScriptState; ScriptCustomElementDefinition can cache the ScriptState and use that.
yosin@chromium.org changed reviewers: + yukishiino@chromium.org
PTAL I'm not sure I need to pass around |ScriptState| from CustomElementsRegistery.get() to ScriptCustomElementsRegistry via CustomElementsRegistery. If we need to check caller's ScriptState and object's ScriptState, we should have m_scriptState in CustomElementsRegistry too. https://codereview.chromium.org/1994093002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h (right): https://codereview.chromium.org/1994093002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h:49: ScriptValue getConstructor(ScriptState*) final; On 2016/05/31 at 06:01:50, dominicc wrote: > Because ScriptCustomElementDefinition will need to vend callbacks, I think it should have RefPtr<ScriptState> and simply use that. We should have |ScriptState*| here to verify we use |SCE| with valid context. https://codereview.chromium.org/1994093002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h (right): https://codereview.chromium.org/1994093002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:16: class ScriptValue; On 2016/05/31 at 06:01:50, dominicc wrote: > Is it possible to return by value without a complete definition? It works, but users of CED needs to include "ScriptValue.h". I changed to add #include "ScriptValue.h" here. https://codereview.chromium.org/1994093002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:27: virtual ScriptValue getConstructor(ScriptState*) = 0; On 2016/05/31 at 06:01:51, dominicc wrote: > Write a C++ unit test that exercises this. Since, getConstructor() is used for implementing CER.get() and it is tested by layout test. Writing C++ test for getConstructor() is heavy, since it requires lots of thing, e.g. DummyPageHolder to have ScriptCER, evaluate script to get constructor. So, I would like to postpone having C++ test until we have script-free version of getConstructor(). https://codereview.chromium.org/1994093002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.idl (right): https://codereview.chromium.org/1994093002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.idl:9: [CallWith=ScriptState] On 2016/05/31 at 06:01:51, dominicc wrote: > I don't think we need CallWith=ScriptState; ScriptCustomElementDefinition can cache the ScriptState and use that. I need to return |undefined| value, when there is no definition for name as |ScriptValue(scriptState, v8Undefined())|. Cached ScriptValue in ScriptCustomElementDefinition will be used for context checking. https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:134: DCHECK_EQ(m_scriptState, scriptState); Do we really need to have this check? https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:143: if (m_scriptState != scriptState) Do we really need to have this check? https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:144: return ScriptValue(scriptState, v8Undefined()); Should be |ScriptValue()|
The CQ bit was checked by dominicc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1994093002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994093002/120001
https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:144: return ScriptValue(scriptState, v8Undefined()); On 2016/05/31 at 09:16:57, Yosi_UTC9 wrote: > Should be |ScriptValue()| Why not just do that? https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h (right): https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:27: // TODO(yosin): We should ask binding layer to convert C++ constructor This TODO covers "what"--ask the binding layer to convert C++ constructors. What's missing is a "when" and/or "why". Try to include some detail that's the trigger condition, so a knowledgable reader can say "ah, we can fix this TODO now" or "no, this is still blocked because X." https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:28: // value to ScriptValue, e.g. introduce |CustomElementConsturcotr| spelling: constructor https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:29: // class with |ScripttCustomElementConsturcotr| spelling: script, constructor https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:30: virtual ScriptValue getConstructor(ScriptState*) = 0; What do you think about calling this getConstructorForScript or getScriptConstructor? This method is all about interfacing with JavaScript, we may as well put it right in the name. https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp (right): https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:122: // Binding layer converts |ScriptValue()| to script specific value, Love this comment. Short and specific.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yosin@chromium.org changed reviewers: + haraken@chromium.org
PTAL +haraken@ for another eye for ScriptState usage. https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:144: return ScriptValue(scriptState, v8Undefined()); On 2016/05/31 at 22:56:32, dominicc wrote: > On 2016/05/31 at 09:16:57, Yosi_UTC9 wrote: > > Should be |ScriptValue()| > > Why not just do that? I expect to have this patch isn't ready for commit or first round of review. I decided to fix in next round. Done. https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h (right): https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:28: // value to ScriptValue, e.g. introduce |CustomElementConsturcotr| On 2016/05/31 at 22:56:32, dominicc wrote: > spelling: constructor Done. https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:29: // class with |ScripttCustomElementConsturcotr| On 2016/05/31 at 22:56:32, dominicc wrote: > spelling: script, constructor Done. https://codereview.chromium.org/1994093002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:30: virtual ScriptValue getConstructor(ScriptState*) = 0; On 2016/05/31 at 22:56:32, dominicc wrote: > What do you think about calling this getConstructorForScript or getScriptConstructor? This method is all about interfacing with JavaScript, we may as well put it right in the name. Done. Use getConstructorForScript()
https://codereview.chromium.org/1994093002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/1994093002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:124: ScriptState* scriptState) const ScriptState is not needed. You can just pass in an Isolate. https://codereview.chromium.org/1994093002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:145: return ScriptValue(scriptState, constructor(scriptState)); I guess this should be: return ScriptValue(m_scriptState, constructor(m_scriptState)); and you can remove the |scriptState| parameter from getConstructorForScript. As far as I understand, CustomElementsRegistry should return a constructor object of the context from which the CustomElementsRegistry is created. If that is the case, you should just use m_scriptState. Also I cannot imagine a scenario where |scriptState| is not equal to |m_scriptState| (because get() is always called by JavaScript; it's never asynchronously called back by DOM). Can it happen? https://codereview.chromium.org/1994093002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.idl (right): https://codereview.chromium.org/1994093002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.idl:10: any get(DOMString name); Nit: Don't break the line.
The CQ bit was checked by yosin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1994093002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994093002/140001
PTAL https://codereview.chromium.org/1994093002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/1994093002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:124: ScriptState* scriptState) const On 2016/06/01 at 02:28:12, haraken wrote: > ScriptState is not needed. You can just pass in an Isolate. I'll handle this change in another CL. https://codereview.chromium.org/1994093002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:145: return ScriptValue(scriptState, constructor(scriptState)); On 2016/06/01 at 02:28:12, haraken wrote: > I guess this should be: > > return ScriptValue(m_scriptState, constructor(m_scriptState)); > > and you can remove the |scriptState| parameter from getConstructorForScript. > > As far as I understand, CustomElementsRegistry should return a constructor object of the context from which the CustomElementsRegistry is created. If that is the case, you should just use m_scriptState. > > Also I cannot imagine a scenario where |scriptState| is not equal to |m_scriptState| (because get() is always called by JavaScript; it's never asynchronously called back by DOM). Can it happen? AFAIK, No. I should not be happened. https://codereview.chromium.org/1994093002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.idl (right): https://codereview.chromium.org/1994093002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.idl:10: any get(DOMString name); On 2016/06/01 at 02:28:12, haraken wrote: > Nit: Don't break the line. Done.
LGTM
The CQ bit was checked by dominicc@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1994093002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994093002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yosin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1994093002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994093002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yosin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1994093002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994093002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Introduce CustomElementRegistry#get() method This patch introduces |CustomElementRegistry#get()| as specified in [1]. To return associated constructor by |get()|, this patch introduces |getConsturcotr(ScriptState*)| member function to |CustomElementDefinition| as pure virtual function and implements it in |ScriptCustomElementDefinition|. [1] https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregis... BUG=594918 TEST=LayoutTests/custom-elements/spec/custom-elements-registry/get.html TEST=LayoutTests/webexposed/global-interface-listing.html ========== to ========== Introduce CustomElementRegistry#get() method This patch introduces |CustomElementRegistry#get()| as specified in [1]. To return associated constructor by |get()|, this patch introduces |getConsturcotr(ScriptState*)| member function to |CustomElementDefinition| as pure virtual function and implements it in |ScriptCustomElementDefinition|. [1] https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregis... BUG=594918 TEST=LayoutTests/custom-elements/spec/custom-elements-registry/get.html TEST=LayoutTests/webexposed/global-interface-listing.html Committed: https://crrev.com/217bd0280d1a9cfd18ade5f2f3c744e8cfb0a0f4 Cr-Commit-Position: refs/heads/master@{#397351} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/217bd0280d1a9cfd18ade5f2f3c744e8cfb0a0f4 Cr-Commit-Position: refs/heads/master@{#397351} |
