Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2054433002:
Implement "create an element" when sync for Custom Element V1 (Closed)
|
Created:
4 years, 6 months ago by kojii Modified:
4 years, 6 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, 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@async-ce Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement "create an element" when sync for Custom Element V1
This patch implements "create an element" concept[1] step 6.2, when
synchronous custom elements flag is set.
[1] https://dom.spec.whatwg.org/#concept-create-element
BUG=594918
Committed: https://crrev.com/e21fcf2e901efb0655bc6beb1355173ea401de83
Cr-Commit-Position: refs/heads/master@{#399789}
Patch Set 1 #Patch Set 2 : Fix building tests #
Total comments: 14
Patch Set 3 : Add tests for createElementNS #Patch Set 4 : dominicc review #Patch Set 5 : Prefer ExceptionState over throw #
Total comments: 23
Patch Set 6 : dominicc review #
Total comments: 23
Patch Set 7 : dominicc/haraken review #
Total comments: 7
Patch Set 8 : haraken review and rebase #Messages
Total messages: 37 (12 generated)
Description was changed from ========== WIP: sync-ce BUG= ========== to ========== WIP: Implement "create an element" when sync for Custom Element V1 This patch implements "create an element" concept[1] step 6.2, when synchronous custom elements flag is set. [1] https://dom.spec.whatwg.org/#concept-create-element ==========
Description was changed from ========== WIP: Implement "create an element" when sync for Custom Element V1 This patch implements "create an element" concept[1] step 6.2, when synchronous custom elements flag is set. [1] https://dom.spec.whatwg.org/#concept-create-element ========== to ========== Implement "create an element" when sync for Custom Element V1 This patch implements "create an element" concept[1] step 6.2, when synchronous custom elements flag is set. [1] https://dom.spec.whatwg.org/#concept-create-element ==========
kojii@chromium.org changed reviewers: + dominicc@chromium.org
PTAL. I need more tests and will update this CL, but code is stable enough for review.
Tests updated, ready for review. PTAL.
I like the direction of this. Lots of comments inline. High level: 1. I think we should pass ExceptionState& more places and try to use that. So the pattern is something like: - If it can generate exceptions, take ExceptionState& - If it has to rethrow V8 exceptions, it must be in ScriptCED and use v8::TryCatch to capture the exception, then transmit it to ExceptionState with ExceptionState::setException. - If it swallows (logs) exceptions, do not have an ExceptionState parameter. Method must be virtual and implemented in ScriptCED and set up a verbose v8::TryCatch. Construct and ExceptionState and pass it down. Use ExceptionState.throwIfNeeded. - If it is a DOM binding, it can pass through its ExceptionState. 2. We should put things just related to DOM, like attribute/parent checks, in CustomElementDefinition. In general a "template method pattern" that makes virtual calls for the script parts is better than the ScriptCED calling parts of algorithms in CED because other subtypes may skip those parts. 3. Could we add some C++ unit tests? https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/custom-elements/spec/create-element-defined-synchronous.html (right): https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/create-element-defined-synchronous.html:26: test_create_element_synchronous((w, constructor, options) => { Why do it this way, where you're passing in this helper? Why not just: function define_and_create_element(w, constructor, options) { ... } test_with_window((w) => { let is_constructed = false; define_and_create_element(w, class extends w.HTMLElement { constructor() { super(); is_constructed = true; } }); assert_true(is_constructed, 'the constructor should have run'); }, '...'); https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/create-element-defined-synchronous.html:54: }, '3. If result does not implement the HTMLElement interface, throw a TypeError'); Might be useful to include tests with HTMLImageElement, etc. on the prototype chain. You could imagine a buggy implementation of the instanceof check which only looks at one prototype or something. Might also be useful to include one with a HTMLElement *instance* on the prototype chain. I believe that should pass (but it will be weird.) https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/create-element-defined-synchronous.html:65: assert_throws(expectNotSupportedError, () => { These should have messages so we get descriptive failures. https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:200: // 6.1.3. If result does not implement the HTMLElement interface, throw a TypeError. This might be a helpful reference: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:205: throwTypeError("The result must implement HTMLElement interface"); Might not be worth adding a helper for one line? BTW, if we switch to ExceptionState it has es.throwTypeError. https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:209: // 6.1.4. through 6.1.9. Could we make this a "template method pattern" (not a *C++* template) where we keep the checks on attributes, etc. in CustomElementDefinition and just put the script-specific things in ScriptCustomElementDefinition? It looks like everything to the end of this method is just dealing with DOM things. https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h (right): https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h:49: HTMLElement* createCustomElement(Document&, const QualifiedName&) override; I think this name should be different. First, "custom" comes from the context--this is a CustomElementDefinition. We have a lot of "custom element" and a lot of "create element" so it is not *really* clear what this does. WDYT about createElementByRunningConstructor Maybe these signatures should be: HTMLElement* createElementByRunningConstructor(Document&, const QualifiedName&, ExceptionState&) override; HTMLElement* createElementByRunningConstructor(Document&, const QualifiedName&); That way the ExceptionState& one can throw exceptions, and the exception reporting one can just set up and pass and ExceptionState. It looks like runConstructor wants to use ExceptionState too. CustomElementDefinition::upgrade could use TrackExceptionState. Then runConstructor does not need to return bool; callers can use ExceptionState::hadException. https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp (right): https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:58: // TODO(kojii): We can't call HTMLElementFactory or This is good!
Patchset #5 (id:80001) has been deleted
PTAL. https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/custom-elements/spec/create-element-defined-synchronous.html (right): https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/create-element-defined-synchronous.html:54: }, '3. If result does not implement the HTMLElement interface, throw a TypeError'); On 2016/06/09 at 06:39:22, dominicc wrote: > Might be useful to include tests with HTMLImageElement, etc. on the prototype chain. You could imagine a buggy implementation of the instanceof check which only looks at one prototype or something. > > Might also be useful to include one with a HTMLElement *instance* on the prototype chain. I believe that should pass (but it will be weird.) Let me try this in following patch. Extending HTMLImageElement throws "invalid constructor" error, and I guess I need a little more reading of the spec to understand such cases better. https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/create-element-defined-synchronous.html:65: assert_throws(expectNotSupportedError, () => { On 2016/06/09 at 06:39:22, dominicc wrote: > These should have messages so we get descriptive failures. Done. https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:200: // 6.1.3. If result does not implement the HTMLElement interface, throw a TypeError. On 2016/06/09 at 06:39:22, dominicc wrote: > This might be a helpful reference: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... Ah, thanks for the pointer, this looks good. It looks like Domenic is fine with checking isHTMLElement() for now: https://github.com/whatwg/html/issues/1402 Hope this is good with you too? https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:205: throwTypeError("The result must implement HTMLElement interface"); On 2016/06/09 at 06:39:22, dominicc wrote: > Might not be worth adding a helper for one line? BTW, if we switch to ExceptionState it has es.throwTypeError. Removed helpers as we switched to ExceptionState. https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:209: // 6.1.4. through 6.1.9. On 2016/06/09 at 06:39:23, dominicc wrote: > Could we make this a "template method pattern" (not a *C++* template) where we keep the checks on attributes, etc. in CustomElementDefinition and just put the script-specific things in ScriptCustomElementDefinition? > > It looks like everything to the end of this method is just dealing with DOM things. Done. https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h (right): https://codereview.chromium.org/2054433002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h:49: HTMLElement* createCustomElement(Document&, const QualifiedName&) override; On 2016/06/09 at 06:39:23, dominicc wrote: > I think this name should be different. > > First, "custom" comes from the context--this is a CustomElementDefinition. We have a lot of "custom element" and a lot of "create element" so it is not *really* clear what this does. > > WDYT about > > createElementByRunningConstructor > > Maybe these signatures should be: > > HTMLElement* createElementByRunningConstructor(Document&, const QualifiedName&, ExceptionState&) override; > HTMLElement* createElementByRunningConstructor(Document&, const QualifiedName&); > > That way the ExceptionState& one can throw exceptions, and the exception reporting one can just set up and pass and ExceptionState. > > It looks like runConstructor wants to use ExceptionState too. CustomElementDefinition::upgrade could use TrackExceptionState. Then runConstructor does not need to return bool; callers can use ExceptionState::hadException. Renaming done in PS4 and switched to ExceptionState done in PS5.
More comments inline. I saw the back and forth between Domenic and Boris about instanceof. The check you have here is fine. It sounds like Boris is thinking about a slightly different check too. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/custom-elements/spec/create-element-defined-synchronous.html (right): https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/create-element-defined-synchronous.html:30: }); Nit, maybe indent this two spaces to line up with the ( of (w,? Also line 37. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:177: errorForConstructorResult(element, document, tagName, exceptionState); Could we call this checkConstructorResult or something, like the pattern in CustomElementConstructorBuilder::check*. This really does the check and schedules the exception. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:195: v8::Isolate* isolate = m_scriptState->isolate(); When the parser calls this there will not be a handle scope on the stack. TryCatch, ExceptionState, etc. use handles; they need a handle scope. I think you need a ScriptState::Scope here. I'm not sure if we need to check contextIsValid here. Let's see what haraken says. Remember the document/window this definition comes from could be completely unrelated to the one the parser is from. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:201: if (!exceptionState.throwIfNeeded() && element && !tryCatch.HasCaught()) exceptionState.throwIfNeeded() will set tryCatch.HasCaught(). It might be more readable to just have // (*) exceptionState.throwIfNeeded(); if (element && !tryCatch.HasCaught()) ... In theory, tryCatch.HasCaught() should be false at (*), because runConstructor should catch the exception and use rethrowV8Exception to transmit it, *not* V8 try-catch handlers. Could you add that assertion? https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:206: return element; Maybe reversing the condition and putting HTMLUnknownElement::create in the branch will eliminate the duplication of return element. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:242: if (!m_scriptState->contextIsValid()) I'm not sure this is right; all the calls are from ScriptCustomElementDefinition. The callers should have entered the scope. A pretty sure sign is you're relying on the caller to have a TryCatch to catch your exception from v8Call--it must have a scope to do that. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp (right): https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:58: // TODO(kojii): We can't call HTMLElementFactory or Could we tighten this up a bit? Like TODO(kojii): When HTMLElementFactory has an option to not queue upgrade, call that instead of HTMLElement. Could you put a TODO on CreateElementFlags to remove the synchronous flag? I think we need at most one bit that is: queue upgrade for custom elements, or don't queue upgrade https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:60: // constructors. HTMLElement is enough for now, but type extesion Spelling: Extension https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:312: Element* createElement(const QualifiedName&, CreateElementFlags, ExceptionState&); OK now I'm really confused. Who uses *this*? I think we're adding too many methods called createElement, and we don't have good layering. Instead of having everything calling everything and using flags to control where the recursion bottoms out, we need a each method to have a distinct responsibility. So, what do we absolutely need: Document::createElement(NS) for the DOM callbacks. These can call CustomElement::createElementSync. Document::createElement for cloning, etc. This calls HTMLElementFactory. CustomElement::createElementSync which swallows (and reports) exceptions. This is called by the parser. Calls HTMLElementFactory. HTMLElementFactory::create. This actually creates elements. The only question is where create-and-queue-upgrade happens. That could be HTMLElementFactory, but it needs an option to turn it off (for createElementSync.) Or it could be Document::createElement. is="" means probably HTMLElementFactory should do *less* and *not* schedule upgrade. The reason is HTMLElementFactory::create does NOT get the attribute list, so it CAN NOT KNOW which definition to queue upgrade with. So, again, I really think we need fewer createElement methods and fewer flags to HTMLElementFactory. I am fine to go in and do this refactoring. But it would be get this patch committed. So let's not make the clean up more difficult by adding more createElement methods, more use of flags in this patch. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElement.h (right): https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElement.h:65: static HTMLElement* createCustomElement(Document&, const QualifiedName&, CreateElementFlags, ExceptionState*); I don't like how things have ended up here. You have ExceptionState/no ExceptionState variants because the caller needs us to swallow (and report) exceptions or transmit them back. Then you compress that down to this method with an optional exception state. Then you decompress that to overloads which do/don't take ExceptionState. Why not give these methods which behave really differently different names and the right signatures, and have them each call the exactly right implementation? So: createCustomElementSync(..., ExceptionState&); createCustomElementSync(...); createCustomElementAsync(...); This is better. It stops the caller from doing something weird like "create custom element asynchronously and put exception state here." There's nothing that needs that AFAIK, and we don't implement that, but your signatures support that. Seems complex and error prone. Let's keep it simple. I DON'T think we need the create element flags here (what are they used for except sync/async?) https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h (left): https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:16: class ScriptState; Thanks for cleaning this up. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h (right): https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:46: virtual HTMLElement* createElementByRunningConstructor(Document&, const QualifiedName&) = 0; Maybe call this createElementSync to match the name in CustomElement. That is the only caller, right?
Patchset #6 (id:120001) has been deleted
All done, PTAL. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/custom-elements/spec/create-element-defined-synchronous.html (right): https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/create-element-defined-synchronous.html:30: }); On 2016/06/10 at 06:13:28, dominicc wrote: > Nit, maybe indent this two spaces to line up with the ( of (w,? Also line 37. Done. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:177: errorForConstructorResult(element, document, tagName, exceptionState); On 2016/06/10 at 06:13:28, dominicc wrote: > Could we call this checkConstructorResult or something, like the pattern in CustomElementConstructorBuilder::check*. This really does the check and schedules the exception. Done. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:195: v8::Isolate* isolate = m_scriptState->isolate(); On 2016/06/10 at 06:13:28, dominicc wrote: > When the parser calls this there will not be a handle scope on the stack. TryCatch, ExceptionState, etc. use handles; they need a handle scope. I think you need a ScriptState::Scope here. > > I'm not sure if we need to check contextIsValid here. Let's see what haraken says. Remember the document/window this definition comes from could be completely unrelated to the one the parser is from. Done. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:201: if (!exceptionState.throwIfNeeded() && element && !tryCatch.HasCaught()) On 2016/06/10 at 06:13:28, dominicc wrote: > exceptionState.throwIfNeeded() will set tryCatch.HasCaught(). It might be more readable to just have > > // (*) > exceptionState.throwIfNeeded(); > if (element && !tryCatch.HasCaught()) > ... > > In theory, tryCatch.HasCaught() should be false at (*), because runConstructor should catch the exception and use rethrowV8Exception to transmit it, *not* V8 try-catch handlers. Could you add that assertion? Done. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:206: return element; On 2016/06/10 at 06:13:28, dominicc wrote: > Maybe reversing the condition and putting HTMLUnknownElement::create in the branch will eliminate the duplication of return element. Done. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:242: if (!m_scriptState->contextIsValid()) On 2016/06/10 at 06:13:28, dominicc wrote: > I'm not sure this is right; all the calls are from ScriptCustomElementDefinition. The callers should have entered the scope. A pretty sure sign is you're relying on the caller to have a TryCatch to catch your exception from v8Call--it must have a scope to do that. Removed and the one above revived. Started to understand what the Scope is... https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp (right): https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:58: // TODO(kojii): We can't call HTMLElementFactory or On 2016/06/10 at 06:13:28, dominicc wrote: > Could we tighten this up a bit? Like > > TODO(kojii): When HTMLElementFactory has an option to not queue upgrade, call that instead of HTMLElement. > > Could you put a TODO on CreateElementFlags to remove the synchronous flag? I think we need at most one bit that is: > > queue upgrade for custom elements, or > don't queue upgrade Done. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:60: // constructors. HTMLElement is enough for now, but type extesion On 2016/06/10 at 06:13:28, dominicc wrote: > Spelling: Extension Done. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:312: Element* createElement(const QualifiedName&, CreateElementFlags, ExceptionState&); On 2016/06/10 at 06:13:28, dominicc wrote: > OK now I'm really confused. Who uses *this*? > > I think we're adding too many methods called createElement, and we don't have good layering. Instead of having everything calling everything and using flags to control where the recursion bottoms out, we need a each method to have a distinct responsibility. > > So, what do we absolutely need: > > Document::createElement(NS) for the DOM callbacks. These can call CustomElement::createElementSync. > Document::createElement for cloning, etc. This calls HTMLElementFactory. > CustomElement::createElementSync which swallows (and reports) exceptions. This is called by the parser. Calls HTMLElementFactory. > HTMLElementFactory::create. This actually creates elements. > > The only question is where create-and-queue-upgrade happens. That could be HTMLElementFactory, but it needs an option to turn it off (for createElementSync.) Or it could be Document::createElement. > > is="" means probably HTMLElementFactory should do *less* and *not* schedule upgrade. The reason is HTMLElementFactory::create does NOT get the attribute list, so it CAN NOT KNOW which definition to queue upgrade with. > > So, again, I really think we need fewer createElement methods and fewer flags to HTMLElementFactory. > > I am fine to go in and do this refactoring. But it would be get this patch committed. So let's not make the clean up more difficult by adding more createElement methods, more use of flags in this patch. Done. took some refactoring, but not all yet. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElement.h (right): https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElement.h:65: static HTMLElement* createCustomElement(Document&, const QualifiedName&, CreateElementFlags, ExceptionState*); On 2016/06/10 at 06:13:28, dominicc wrote: > I don't like how things have ended up here. You have ExceptionState/no ExceptionState variants because the caller needs us to swallow (and report) exceptions or transmit them back. > > Then you compress that down to this method with an optional exception state. > > Then you decompress that to overloads which do/don't take ExceptionState. > > Why not give these methods which behave really differently different names and the right signatures, and have them each call the exactly right implementation? So: > > createCustomElementSync(..., ExceptionState&); > createCustomElementSync(...); > createCustomElementAsync(...); > > This is better. It stops the caller from doing something weird like "create custom element asynchronously and put exception state here." There's nothing that needs that AFAIK, and we don't implement that, but your signatures support that. Seems complex and error prone. Let's keep it simple. > > I DON'T think we need the create element flags here (what are they used for except sync/async?) Done. It used to use CreatedByParser too, but that was replaced by optional ExceptionState. https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h (right): https://codereview.chromium.org/2054433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:46: virtual HTMLElement* createElementByRunningConstructor(Document&, const QualifiedName&) = 0; On 2016/06/10 at 06:13:28, dominicc wrote: > Maybe call this createElementSync to match the name in CustomElement. That is the only caller, right? Done.
lgtm This LGTM. I think we should use CustomElementDescriptor more and name/qname less, but I am fine if we work on that in follow-up patches--your choice. Parts of this look quite neat--nice work. https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:221: // TODO(kojii): Remove these flags, add an option to HTMLElementFactory I think we should not let HTMLElementFactory ever queue upgrade because it won't work with customized built-in elements. None of those call sites pass in "is." I think it is better if the caller handles it. If you want to do it this way, can you sketch the API for HTMLElementFactory::createElement? How do all of the callers pass "is"? https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp (right): https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:34: CustomElementDefinition* CustomElement::definitionForName(const Document& document, const AtomicString& name) This is not the right signature for when we implement customized built-in elements. Use a const CustomElementDescriptor&. https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:104: if (CustomElementDefinition* definition = definitionForName(document, tagName.localName())) This is error-prone because it throws away the namespace; maybe we should upgrade the DCHECK to CHECK. In general, it might be better to have people construct CustomElementDescriptors, and use those consistently to find definitions. The only exceptions are: - The get API. - Testing if a *name* is defined. - As an optimization (for example, taking upgrade candidates for one descriptor implies any other upgrade candidates with that *name* will never be processed and can be discarded.) Then we have assertions about what's a valid descriptor in one place. You can't process an svg:my-button as a custom element because you can't construct that descriptor. If we don't use the descriptor now, we will end up changing all of these method signatures later to take an "is" attribute. It will be simpler if we use the descriptor, now, instead. https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElement.h (right): https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElement.h:61: static HTMLElement* createCustomElementSync(Document&, const QualifiedName& localName); Be consistent about whether you are naming the localName parameter or not.
kojii@chromium.org changed reviewers: + haraken@chromium.org
haraken@, PTAL for binding
https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:161: // Create an element Is it guaranteed that you're already in m_scriptState? https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:166: Element* element; element = nullptr https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:197: v8::TryCatch tryCatch(isolate); Why do you need this TryCatch? https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:198: tryCatch.SetVerbose(true); Remove this line unless you really want to make it verbose. https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:202: DCHECK(!tryCatch.HasCaught()); Instead, you can check DCHECK(!exceptionState.hadException()). https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:223: tryCatch.SetVerbose(true); Remove this line. https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:246: v8::Isolate* isolate = m_scriptState->isolate(); Is it guaranteed that you're already in m_scriptState? https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.idl (right): https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.idl:58: [NewObject, DoNotTestNewObject, CustomElementCallbacks, RaisesException] Element createElementNS(DOMString? namespaceURI, DOMString qualifiedName); Would you help me understand why you need [DoNotTestNewObject]?
Replying to haraken@'s review. Still preparing new CL for dominicc@'s. https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:161: // Create an element On 2016/06/13 at 09:05:35, haraken wrote: > Is it guaranteed that you're already in m_scriptState? Yes, that's the assumption atm; we assume DOM createElement/createElementNS are the only caller of this method. If there's a good way to DCHECK it, I'd be happy to add. https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:166: Element* element; On 2016/06/13 at 09:05:35, haraken wrote: > element = nullptr Will fix in the next CL. https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:198: tryCatch.SetVerbose(true); On 2016/06/13 at 09:05:35, haraken wrote: > Remove this line unless you really want to make it verbose. The spec (copied in the comment above) says catch and report the error to console. I understand tryCatch w/SetVerbose() is the only way to do this, no? https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:202: DCHECK(!tryCatch.HasCaught()); On 2016/06/13 at 09:05:35, haraken wrote: > Instead, you can check DCHECK(!exceptionState.hadException()). There are errors here when JS throws. The purpose here is to ensure: 1. All errors are thrown to ExceptionState, 2. and that tryCatch doesn't catch anything. Is this correct way? https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:223: tryCatch.SetVerbose(true); On 2016/06/13 at 09:05:35, haraken wrote: > Remove this line. The spec requires to "report the error" here, as in the comment above this line. https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:246: v8::Isolate* isolate = m_scriptState->isolate(); On 2016/06/13 at 09:05:35, haraken wrote: > Is it guaranteed that you're already in m_scriptState? Yes, this is called from either: 1. createElement above, where you commented, or 2. runConstructor(Element*) above, where we setup Scope. Again, if there's a way to DCHECK(), I'm happy to add. https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.idl (right): https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.idl:58: [NewObject, DoNotTestNewObject, CustomElementCallbacks, RaisesException] Element createElementNS(DOMString? namespaceURI, DOMString qualifiedName); On 2016/06/13 at 09:05:35, haraken wrote: > Would you help me understand why you need [DoNotTestNewObject]? NewObjet tests if JS wrapper hasn't been created for the native object. With custom elements, JS constructor already runs for the element with "this" set to the element, so the test fails. Consulted with yukishiino, advised to add this attribute in such case. He also recommended to add DCHECK for such methods, but in this case I couldn't find good place to add.
Done, PTAL. Would like to defer some of refactoring requests in following patch. https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:221: // TODO(kojii): Remove these flags, add an option to HTMLElementFactory On 2016/06/13 at 07:37:24, dominicc wrote: > I think we should not let HTMLElementFactory ever queue upgrade because it won't work with customized built-in elements. None of those call sites pass in "is." I think it is better if the caller handles it. > > If you want to do it this way, can you sketch the API for HTMLElementFactory::createElement? How do all of the callers pass "is"? I don't have opinions here, just tried to follow your previous advice. "To..." was inaccurate probably, removed. Thinking about the refactoring is on my queue, but haven't really spent time on it yet. https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp (right): https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:34: CustomElementDefinition* CustomElement::definitionForName(const Document& document, const AtomicString& name) On 2016/06/13 at 07:37:24, dominicc wrote: > This is not the right signature for when we implement customized built-in elements. Use a const CustomElementDescriptor&. Made this static to this file for now to avoid expanding its use, and see below for more comments. https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:104: if (CustomElementDefinition* definition = definitionForName(document, tagName.localName())) On 2016/06/13 at 07:37:24, dominicc wrote: > This is error-prone because it throws away the namespace; maybe we should upgrade the DCHECK to CHECK. > > In general, it might be better to have people construct CustomElementDescriptors, and use those consistently to find definitions. The only exceptions are: > > - The get API. > - Testing if a *name* is defined. > - As an optimization (for example, taking upgrade candidates for one descriptor implies any other upgrade candidates with that *name* will never be processed and can be discarded.) > > Then we have assertions about what's a valid descriptor in one place. You can't process an svg:my-button as a custom element because you can't construct that descriptor. > > If we don't use the descriptor now, we will end up changing all of these method signatures later to take an "is" attribute. It will be simpler if we use the descriptor, now, instead. I agree with you in general direction we should move forward to, but I'd prefer to take a refactoring step on this point. Unable to create synchronously limits us creating test cases for the parallel work we're doing. To use CustomElementDescriptors properly and to prepare for type extension, I need to refactor HTMLElementFactory first. That can be done in parallel with other work while I'm waiting for reviews on them. DCHECKs changed to CHECKs. https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElement.h (right): https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElement.h:61: static HTMLElement* createCustomElementSync(Document&, const QualifiedName& localName); On 2016/06/13 at 07:37:24, dominicc wrote: > Be consistent about whether you are naming the localName parameter or not. Done.
Show me how HTMLElementFactory will work on the whiteboard if you have time today :) On Mon, Jun 13, 2016 at 10:44 PM, <kojii@chromium.org> wrote: > Done, PTAL. Would like to defer some of refactoring requests in following > patch. > > > > https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/Document.h (right): > > > https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Document.h:221: // TODO(kojii): > Remove these flags, add an option to HTMLElementFactory > On 2016/06/13 at 07:37:24, dominicc wrote: > > I think we should not let HTMLElementFactory ever queue upgrade > because it won't work with customized built-in elements. None of those > call sites pass in "is." I think it is better if the caller handles it. > > > > If you want to do it this way, can you sketch the API for > HTMLElementFactory::createElement? How do all of the callers pass "is"? > > I don't have opinions here, just tried to follow your previous advice. > "To..." was inaccurate probably, removed. > > Thinking about the refactoring is on my queue, but haven't really spent > time on it yet. > > > https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp > (right): > > > https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:34: > CustomElementDefinition* CustomElement::definitionForName(const > Document& document, const AtomicString& name) > On 2016/06/13 at 07:37:24, dominicc wrote: > > This is not the right signature for when we implement customized > built-in elements. Use a const CustomElementDescriptor&. > > Made this static to this file for now to avoid expanding its use, and > see below for more comments. > > > https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:104: if > (CustomElementDefinition* definition = definitionForName(document, > tagName.localName())) > On 2016/06/13 at 07:37:24, dominicc wrote: > > This is error-prone because it throws away the namespace; maybe we > should upgrade the DCHECK to CHECK. > > > > In general, it might be better to have people construct > CustomElementDescriptors, and use those consistently to find > definitions. The only exceptions are: > > > > - The get API. > > - Testing if a *name* is defined. > > - As an optimization (for example, taking upgrade candidates for one > descriptor implies any other upgrade candidates with that *name* will > never be processed and can be discarded.) > > > > Then we have assertions about what's a valid descriptor in one place. > You can't process an svg:my-button as a custom element because you can't > construct that descriptor. > > > > If we don't use the descriptor now, we will end up changing all of > these method signatures later to take an "is" attribute. It will be > simpler if we use the descriptor, now, instead. > > I agree with you in general direction we should move forward to, but I'd > prefer to take a refactoring step on this point. Unable to create > synchronously limits us creating test cases for the parallel work we're > doing. > > To use CustomElementDescriptors properly and to prepare for type > extension, I need to refactor HTMLElementFactory first. That can be done > in parallel with other work while I'm waiting for reviews on them. > > DCHECKs changed to CHECKs. > > > https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/custom/CustomElement.h (right): > > > https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/custom/CustomElement.h:61: static > HTMLElement* createCustomElementSync(Document&, const QualifiedName& > localName); > On 2016/06/13 at 07:37:24, dominicc wrote: > > Be consistent about whether you are naming the localName parameter or > not. > > Done. > > https://codereview.chromium.org/2054433002/ > -- 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.
Show me how HTMLElementFactory will work on the whiteboard if you have time today :) On Mon, Jun 13, 2016 at 10:44 PM, <kojii@chromium.org> wrote: > Done, PTAL. Would like to defer some of refactoring requests in following > patch. > > > > https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/Document.h (right): > > > https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Document.h:221: // TODO(kojii): > Remove these flags, add an option to HTMLElementFactory > On 2016/06/13 at 07:37:24, dominicc wrote: > > I think we should not let HTMLElementFactory ever queue upgrade > because it won't work with customized built-in elements. None of those > call sites pass in "is." I think it is better if the caller handles it. > > > > If you want to do it this way, can you sketch the API for > HTMLElementFactory::createElement? How do all of the callers pass "is"? > > I don't have opinions here, just tried to follow your previous advice. > "To..." was inaccurate probably, removed. > > Thinking about the refactoring is on my queue, but haven't really spent > time on it yet. > > > https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp > (right): > > > https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:34: > CustomElementDefinition* CustomElement::definitionForName(const > Document& document, const AtomicString& name) > On 2016/06/13 at 07:37:24, dominicc wrote: > > This is not the right signature for when we implement customized > built-in elements. Use a const CustomElementDescriptor&. > > Made this static to this file for now to avoid expanding its use, and > see below for more comments. > > > https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:104: if > (CustomElementDefinition* definition = definitionForName(document, > tagName.localName())) > On 2016/06/13 at 07:37:24, dominicc wrote: > > This is error-prone because it throws away the namespace; maybe we > should upgrade the DCHECK to CHECK. > > > > In general, it might be better to have people construct > CustomElementDescriptors, and use those consistently to find > definitions. The only exceptions are: > > > > - The get API. > > - Testing if a *name* is defined. > > - As an optimization (for example, taking upgrade candidates for one > descriptor implies any other upgrade candidates with that *name* will > never be processed and can be discarded.) > > > > Then we have assertions about what's a valid descriptor in one place. > You can't process an svg:my-button as a custom element because you can't > construct that descriptor. > > > > If we don't use the descriptor now, we will end up changing all of > these method signatures later to take an "is" attribute. It will be > simpler if we use the descriptor, now, instead. > > I agree with you in general direction we should move forward to, but I'd > prefer to take a refactoring step on this point. Unable to create > synchronously limits us creating test cases for the parallel work we're > doing. > > To use CustomElementDescriptors properly and to prepare for type > extension, I need to refactor HTMLElementFactory first. That can be done > in parallel with other work while I'm waiting for reviews on them. > > DCHECKs changed to CHECKs. > > > https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/custom/CustomElement.h (right): > > > https://codereview.chromium.org/2054433002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/custom/CustomElement.h:61: static > HTMLElement* createCustomElementSync(Document&, const QualifiedName& > localName); > On 2016/06/13 at 07:37:24, dominicc wrote: > > Be consistent about whether you are naming the localName parameter or > not. > > Done. > > https://codereview.chromium.org/2054433002/ > -- 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.
https://codereview.chromium.org/2054433002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2054433002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:160: { You can add DCHECK(ScriptState::current() == m_scriptState). https://codereview.chromium.org/2054433002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:199: exceptionState.throwIfNeeded(); I still don't understand these lines. The createElementSync at line 157 should never throw a V8 exception because it is suppressed at the TryCatch block at line 168. Note that exceptionState.rethrowV8Exception just sets the exception on the exceptionState and doesn't throw the exception. https://codereview.chromium.org/2054433002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:245: { You can add DCHECK(ScriptState::current() == m_scriptState).
https://codereview.chromium.org/2054433002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2054433002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:208: } To make the code clearer, let's write this as follows: createElementSync(..., exceptionState); if (exceptionState.hadException()) { ...; } v8::TryCatch tryCatch(isolate); tryCatch.SetVerbose(true); exceptionState.throwIfNeeded();
PTAL. https://codereview.chromium.org/2054433002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2054433002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:160: { On 2016/06/14 at 01:48:11, haraken wrote: > You can add DCHECK(ScriptState::current() == m_scriptState). Done. https://codereview.chromium.org/2054433002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:199: exceptionState.throwIfNeeded(); On 2016/06/14 at 01:48:11, haraken wrote: > I still don't understand these lines. > > The createElementSync at line 157 should never throw a V8 exception because it is suppressed at the TryCatch block at line 168. Note that exceptionState.rethrowV8Exception just sets the exception on the exceptionState and doesn't throw the exception. Reordered to read easier by reflecting our offline discussion. https://codereview.chromium.org/2054433002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:245: { On 2016/06/14 at 01:48:11, haraken wrote: > You can add DCHECK(ScriptState::current() == m_scriptState). Done.
LGTM But I want to consider how to clean up the exception handling (by improving APIs of TryCatch and/or ExceptionState). For example, would it be helpful to have ExceptionState::verboselyThrowIfNeeded()? (FWIW, I'm trying to deprecate throwIfNeeded in https://codereview.chromium.org/2061113002.)
Thank you for the prompt review. On 2016/06/14 at 07:22:10, haraken wrote: > But I want to consider how to clean up the exception handling (by improving APIs of TryCatch and/or ExceptionState). For example, would it be helpful to have ExceptionState::verboselyThrowIfNeeded()? > > (FWIW, I'm trying to deprecate throwIfNeeded in https://codereview.chromium.org/2061113002.) Ah, I see your intention. verboselyThrowIfNeeded() sounds like it throws. What about throwAndCatchVerboselyIfNeeded()? Too verbose? ;) The spec I'm following here is "report an exception" https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception this includes Error event handling etc., which IIUC implemented in ExecutionContext::reportException(). Maybe reportException() is easier to find?
On 2016/06/14 07:53:41, kojii wrote: > Thank you for the prompt review. > > On 2016/06/14 at 07:22:10, haraken wrote: > > But I want to consider how to clean up the exception handling (by improving > APIs of TryCatch and/or ExceptionState). For example, would it be helpful to > have ExceptionState::verboselyThrowIfNeeded()? > > > > (FWIW, I'm trying to deprecate throwIfNeeded in > https://codereview.chromium.org/2061113002.) > > Ah, I see your intention. > > verboselyThrowIfNeeded() sounds like it throws. What about > throwAndCatchVerboselyIfNeeded()? Too verbose? ;) > > The spec I'm following here is "report an exception" > https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception > this includes Error event handling etc., which IIUC implemented in > ExecutionContext::reportException(). Maybe reportException() is easier to find? Yeah, sounds nice. (I'm not sure when I can remove throwIfNeeded(). Feel free to make your change first.)
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominicc@chromium.org Link to the patchset: https://codereview.chromium.org/2054433002/#ps180001 (title: "haraken review and rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2054433002/180001
Description was changed from ========== Implement "create an element" when sync for Custom Element V1 This patch implements "create an element" concept[1] step 6.2, when synchronous custom elements flag is set. [1] https://dom.spec.whatwg.org/#concept-create-element ========== to ========== Implement "create an element" when sync for Custom Element V1 This patch implements "create an element" concept[1] step 6.2, when synchronous custom elements flag is set. [1] https://dom.spec.whatwg.org/#concept-create-element BUG=594918 ==========
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 kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2054433002/180001
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Implement "create an element" when sync for Custom Element V1 This patch implements "create an element" concept[1] step 6.2, when synchronous custom elements flag is set. [1] https://dom.spec.whatwg.org/#concept-create-element BUG=594918 ========== to ========== Implement "create an element" when sync for Custom Element V1 This patch implements "create an element" concept[1] step 6.2, when synchronous custom elements flag is set. [1] https://dom.spec.whatwg.org/#concept-create-element BUG=594918 Committed: https://crrev.com/e21fcf2e901efb0655bc6beb1355173ea401de83 Cr-Commit-Position: refs/heads/master@{#399789} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e21fcf2e901efb0655bc6beb1355173ea401de83 Cr-Commit-Position: refs/heads/master@{#399789} |
