Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2002903002:
Hook createElement for Custom Elements V1 (Closed)
|
Created:
4 years, 7 months ago by kojii Modified:
4 years, 7 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. |
DescriptionHook createElement for Custom Elements V1
This patch hooks Custom Element V1 creation points and set the Custom
Element State to Undefined when it meets the criteria.
When and how to set the Custom Element State has open issues and is
subject to change. We may need to change when the spec changes.
Most of the logic to create an element[1] is not implemented yet.
[1] https://dom.spec.whatwg.org/#concept-create-element
BUG=594918
Committed: https://crrev.com/1d85c7a9c9f2c5d668adef2ae8c35f17679a5853
Cr-Commit-Position: refs/heads/master@{#396078}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add test #Patch Set 3 : Add createElementNS tests #Patch Set 4 : RuntimeEnabledFeatures #
Total comments: 16
Patch Set 5 : dominicc review #
Dependent Patchsets: Messages
Total messages: 20 (5 generated)
kojii@chromium.org changed reviewers: + dominicc@chromium.org, yosin@chromium.org
Not ready for review yet, but I think these are all the places we need to hook for creating custom elements. Still: * When/how to set state is still under discussion, but we'll need hooks to create elements anyway, and this can move :defined forward. * Type extension isn't supported yet. Non-parser code can add an argument, but parser needs another hook. * Callers need to pass synchronous flag. Haven't really thought well on it yet. https://codereview.chromium.org/2002903002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp (right): https://codereview.chromium.org/2002903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:57: return document.frame() Is this correct condition when the spec says "has browsing context"?
Description was changed from ========== WIP: Set CustomElementState to Undefined BUG= ========== to ========== Set CustomElementState to Undefined This patch hooks Custom Element V1 creation points and set the Custom Element State to Undefined when it meets the criteria. When and how to set the Custom Element State has open issues and is subject to change. We may need to change when the spec changes. Most of the logic to create an element[1] is not implemented yet. [1] https://dom.spec.whatwg.org/#concept-create-element BUG=594918 ==========
PTAL, tests added.
Description was changed from ========== Set CustomElementState to Undefined This patch hooks Custom Element V1 creation points and set the Custom Element State to Undefined when it meets the criteria. When and how to set the Custom Element State has open issues and is subject to change. We may need to change when the spec changes. Most of the logic to create an element[1] is not implemented yet. [1] https://dom.spec.whatwg.org/#concept-create-element BUG=594918 ========== to ========== Hook createElement for Custom Elements V1 This patch hooks Custom Element V1 creation points and set the Custom Element State to Undefined when it meets the criteria. When and how to set the Custom Element State has open issues and is subject to change. We may need to change when the spec changes. Most of the logic to create an element[1] is not implemented yet. [1] https://dom.spec.whatwg.org/#concept-create-element BUG=594918 ==========
https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp (right): https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:80: Element* e = document.registrationContext()->createCustomTagElement(document, tagName); nit: Could you avoid to use one letter variable name? https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:87: element->setCustomElementState(CustomElementState::Undefined); Can we set customer element state in HTML constructor to make sure HTMLElement starts with "undefined" state?
Thank you for review. https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp (right): https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:80: Element* e = document.registrationContext()->createCustomTagElement(document, tagName); On 2016/05/25 at 01:16:20, Yosi_UTC9 wrote: > nit: Could you avoid to use one letter variable name? Will do in the next patch. https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:87: element->setCustomElementState(CustomElementState::Undefined); On 2016/05/25 at 01:16:20, Yosi_UTC9 wrote: > Can we set customer element state in HTML constructor to make sure HTMLElement starts with "undefined" state? Shouldn't the default be "uncustomized"? That's the most common case, no? I'm thinking: 1. Elements starts with uncustomized, including HTMLElement 2. v8 custom constructor sets to customized 3. Here sets to undefined 4. There might be more places to set to customized, esp. for your extensions Am I missing something?
https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp (right): https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:87: element->setCustomElementState(CustomElementState::Undefined); On 2016/05/25 at 02:44:53, kojii wrote: > On 2016/05/25 at 01:16:20, Yosi_UTC9 wrote: > > Can we set customer element state in HTML constructor to make sure HTMLElement starts with "undefined" state? > > Shouldn't the default be "uncustomized"? That's the most common case, no? > > I'm thinking: > 1. Elements starts with uncustomized, including HTMLElement > 2. v8 custom constructor sets to customized > 3. Here sets to undefined > 4. There might be more places to set to customized, esp. for your extensions > > Am I missing something? I summarize initial state of "customer element state": https://docs.google.com/a/chromium.org/presentation/d/1OYAif_l4tn19_1FceJAQcZ... It seems HTMLElement ctor can set "undefined" or "uncustomized" based on whether local name is valid for custom element.
On 2016/05/25 at 04:18:15, yosin wrote: > https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp (right): > > https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:87: element->setCustomElementState(CustomElementState::Undefined); > On 2016/05/25 at 02:44:53, kojii wrote: > > On 2016/05/25 at 01:16:20, Yosi_UTC9 wrote: > > > Can we set customer element state in HTML constructor to make sure HTMLElement starts with "undefined" state? > > > > Shouldn't the default be "uncustomized"? That's the most common case, no? > > > > I'm thinking: > > 1. Elements starts with uncustomized, including HTMLElement > > 2. v8 custom constructor sets to customized > > 3. Here sets to undefined > > 4. There might be more places to set to customized, esp. for your extensions > > > > Am I missing something? > > I summarize initial state of "customer element state": > https://docs.google.com/a/chromium.org/presentation/d/1OYAif_l4tn19_1FceJAQcZ... > > It seems HTMLElement ctor can set "undefined" or "uncustomized" based on whether local name is valid for custom element. Yes, it can. But since we'll need to check it anyway here, it'll double the check. Why do you want to check the same logic in two separate places?
On 2016/05/25 at 04:18:15, yosin wrote: > I summarize initial state of "customer element state": > https://docs.google.com/a/chromium.org/presentation/d/1OYAif_l4tn19_1FceJAQcZ... BTW, changing "is" does not change the state, so it never goes back.
Some comments inline. https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ElementFactory.cpp.tmpl (right): https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ElementFactory.cpp.tmpl:93: if (CustomElement::shouldCreateCustomElement(document, localName)) :/ this might do things with v0 where they did not happen before. How does this do extra stuff and yet work? :P https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp (right): https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:76: // TODO(kojii): This does not lookup already defined custom elements yet. Could you make this TODO(who) what/when so it is easier to know when a TODO can be fixed and what to do about it? Maybe something like // TODO(kojii): Look up already defined custom elements when custom element queues and upgrade are implemented. or something like that. https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:87: element->setCustomElementState(CustomElementState::Undefined); On 2016/05/25 at 02:44:53, kojii wrote: > On 2016/05/25 at 01:16:20, Yosi_UTC9 wrote: > > Can we set customer element state in HTML constructor to make sure HTMLElement starts with "undefined" state? > > Shouldn't the default be "uncustomized"? That's the most common case, no? > > I'm thinking: > 1. Elements starts with uncustomized, including HTMLElement > 2. v8 custom constructor sets to customized > 3. Here sets to undefined > 4. There might be more places to set to customized, esp. for your extensions > > Am I missing something? I guess "is" happens in parserSetAttributes? https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp (right): https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:137: document.view()->updateAllLifecyclePhases(); Do we need this? The script could set innerHTML and expect to see the results straight away; let's test something like that? https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:164: { "font-face", CustomElementState::Uncustomized, Element::V0WaitingForUpgrade }, I think V0WaitingForUpgrade is a bug; I think pdr regressed this in https://codereview.chromium.org/656913006 . Maybe but TODO(pdr) on it and link to that code review.
> I guess "is" happens in parserSetAttributes? Answer my own question: It's in Element::attributeChangedByParserOrByCloning. We need to support "is" later, too; it is not clear to me what's the best hook for that.
Thank you for the review, CL updated and comments inline: https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ElementFactory.cpp.tmpl (right): https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ElementFactory.cpp.tmpl:93: if (CustomElement::shouldCreateCustomElement(document, localName)) On 2016/05/25 at 05:25:52, dominicc wrote: > :/ this might do things with v0 where they did not happen before. How does this do extra stuff and yet work? :P Not sure if I understand correctly, but if you're talking about the case where the element can be either v0 or v1, the v1 CustomElement::shouldCreateCustomElement() handles both. You'll see the same logic as the one below copied to CustomElement::shouldCreateCustomElement(). ...or did I misunderstand your comment? https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/custom/CustomElement.cpp (right): https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:76: // TODO(kojii): This does not lookup already defined custom elements yet. On 2016/05/25 at 05:25:52, dominicc wrote: > Could you make this TODO(who) what/when so it is easier to know when a TODO can be fixed and what to do about it? Maybe something like > > // TODO(kojii): Look up already defined custom elements when custom element queues and upgrade are implemented. > > or something like that. Done. https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:80: Element* e = document.registrationContext()->createCustomTagElement(document, tagName); On 2016/05/25 at 02:44:53, kojii wrote: > On 2016/05/25 at 01:16:20, Yosi_UTC9 wrote: > > nit: Could you avoid to use one letter variable name? > > Will do in the next patch. Done. https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElement.cpp:87: element->setCustomElementState(CustomElementState::Undefined); On 2016/05/25 at 05:25:52, dominicc wrote: > On 2016/05/25 at 02:44:53, kojii wrote: > > On 2016/05/25 at 01:16:20, Yosi_UTC9 wrote: > > > Can we set customer element state in HTML constructor to make sure HTMLElement starts with "undefined" state? > > > > Shouldn't the default be "uncustomized"? That's the most common case, no? > > > > I'm thinking: > > 1. Elements starts with uncustomized, including HTMLElement > > 2. v8 custom constructor sets to customized > > 3. Here sets to undefined > > 4. There might be more places to set to customized, esp. for your extensions > > > > Am I missing something? > > I guess "is" happens in parserSetAttributes? Yeah, for parser, I think we'll need to split creation in two phases and delay the creation until we parse all attributes. https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp (right): https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:137: document.view()->updateAllLifecyclePhases(); On 2016/05/25 at 05:25:52, dominicc wrote: > Do we need this? The script could set innerHTML and expect to see the results straight away; let's test something like that? Removed, wasn't needed, thanks. https://codereview.chromium.org/2002903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/custom/CustomElementTest.cpp:164: { "font-face", CustomElementState::Uncustomized, Element::V0WaitingForUpgrade }, On 2016/05/25 at 05:25:52, dominicc wrote: > I think V0WaitingForUpgrade is a bug; I think pdr regressed this in https://codereview.chromium.org/656913006 . Maybe but TODO(pdr) on it and link to that code review. Done.
lgtm I was fearful about leaving ElementFactory.cpp.tmpl lines 97-102, given there's another version of them in CustomElement. I guess this is a bit tricky because we want v1 to take precedence, but with the given interface it's unclear whether v0 is creating upgraded elements or not; and for selectors to work v1 has to set its defined-ness bits anyway. So it is fine as-is. You could do some surgery on v0 and have CustomElement::shouldCreateCustomElement/createCustomElement handle both, since it has to have the v0 code sitting in there anyway, but it is up to you. It's the sort of thing we could do in a follow-up patch. BTW, does your later patch have some querySelector tests in disconnected documents?
On 2016/05/25 at 22:01:39, dominicc wrote: > lgtm > > I was fearful about leaving ElementFactory.cpp.tmpl lines 97-102, given there's another version of them in CustomElement. > > I guess this is a bit tricky because we want v1 to take precedence, but with the given interface it's unclear whether v0 is creating upgraded elements or not; and for selectors to work v1 has to set its defined-ness bits anyway. > > So it is fine as-is. You could do some surgery on v0 and have CustomElement::shouldCreateCustomElement/createCustomElement handle both, since it has to have the v0 code sitting in there anyway, but it is up to you. It's the sort of thing we could do in a follow-up patch. Ah, I see your point now, makes sense. I'll give thoughts on the surgery in a follow-up patch. > BTW, does your later patch have some querySelector tests in disconnected documents? Good point, I don't think it has. Will check.
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/2002903002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2002903002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Hook createElement for Custom Elements V1 This patch hooks Custom Element V1 creation points and set the Custom Element State to Undefined when it meets the criteria. When and how to set the Custom Element State has open issues and is subject to change. We may need to change when the spec changes. Most of the logic to create an element[1] is not implemented yet. [1] https://dom.spec.whatwg.org/#concept-create-element BUG=594918 ========== to ========== Hook createElement for Custom Elements V1 This patch hooks Custom Element V1 creation points and set the Custom Element State to Undefined when it meets the criteria. When and how to set the Custom Element State has open issues and is subject to change. We may need to change when the spec changes. Most of the logic to create an element[1] is not implemented yet. [1] https://dom.spec.whatwg.org/#concept-create-element BUG=594918 Committed: https://crrev.com/1d85c7a9c9f2c5d668adef2ae8c35f17679a5853 Cr-Commit-Position: refs/heads/master@{#396078} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1d85c7a9c9f2c5d668adef2ae8c35f17679a5853 Cr-Commit-Position: refs/heads/master@{#396078} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
