Closed Bug 1416999 Opened 5 years ago Closed 5 years ago

Deprecate document.registerElement

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(2 files)

Bug 1416999 - Remove document.registerElement;
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1416999 - Remove the custom elements prototype and document order stored in CustomElementDefinition;
59 bytes, text/x-review-board-request
smaug
: review+
Details 
We are close to finishing the implementation for new spec, and we already deprecate some old callback, it's time to deprecate document.registerElement and revise the test mochitest cases to use customElement.define instead.
Assignee: nobody → echen
Attachment #8928915 - Flags: feedback?(jdai)
Comment on attachment 8928915 [details]
Bug 1416999 - Remove document.registerElement;

https://reviewboard.mozilla.org/r/200252/#review208340

::: dom/tests/mochitest/webcomponents/test_custom_element_lifecycle.html:192
(Diff revision 4)
>    };
>  
> -  document.registerElement("x-resolved", { prototype: p });
> +  customElements.define("x-resolved", Resolved);
>  
>    var createdElement = document.createElement("x-resolved");
> -  is(createdElement.__proto__, p, "Prototype of custom element should be the registered prototype.");
> +  is(createdElement.__proto__, Resolved.prototype, "Prototype of custom element should be the registered prototype.");

Nit: s/registered/defined/

::: dom/tests/mochitest/webcomponents/test_custom_element_define_parser.html:20
(Diff revision 4)
> -  is(buttonCallbackCalled, false, "created callback for x-button should only be called once.");
> +};
> +
> +class XButton extends HTMLButtonElement {
> +  connectedCallback() {
> -  is(this.tagName, "BUTTON", "Only the <button> element should be upgraded.");
> +    is(this.tagName, "BUTTON", "Only the <button> element should be upgraded.");
> -  buttonCallbackCalled = true;
> +    ok(uncaughtError instanceof TypeError,

Why a type error for upgrading <x-div> element put in XButton's connectedCallback? It seems that when XButton's connectedCallback is called, but <x-div> not execute yet, hence, there is no error to report.

::: dom/tests/mochitest/webcomponents/test_custom_element_define_parser.html:21
(Diff revision 4)
> +
> +class XButton extends HTMLButtonElement {
> +  connectedCallback() {
> -  is(this.tagName, "BUTTON", "Only the <button> element should be upgraded.");
> +    is(this.tagName, "BUTTON", "Only the <button> element should be upgraded.");
> -  buttonCallbackCalled = true;
> +    ok(uncaughtError instanceof TypeError,
> +       "TpyeError should be filed for upgrading <x-div> element.");

Nit: s/TpyeError/TypeError/
Attachment #8928915 - Flags: feedback?(jdai)
Comment on attachment 8928915 [details]
Bug 1416999 - Remove document.registerElement;

https://reviewboard.mozilla.org/r/200252/#review208404

::: dom/tests/mochitest/webcomponents/test_custom_element_define_parser.html:20
(Diff revision 4)
> -  is(buttonCallbackCalled, false, "created callback for x-button should only be called once.");
> +};
> +
> +class XButton extends HTMLButtonElement {
> +  connectedCallback() {
> -  is(this.tagName, "BUTTON", "Only the <button> element should be upgraded.");
> +    is(this.tagName, "BUTTON", "Only the <button> element should be upgraded.");
> -  buttonCallbackCalled = true;
> +    ok(uncaughtError instanceof TypeError,

For parser case, the upgrading and callbacks are all scheduled to BackupQueue which is invoked "synchronously" in microtask check point.

In microtask check point, the BackupQueue is like,
1. upgrading <button is="x-button"></button> (this will enqueue a connectedCallback reaction to the end of BackupQueue)
2. upgrading <x-button></x-button>
3. upgrading <span is="x-button"></span>
4. upgrading <div is="x-div"></div>
5. upgrading <x-div></x-div> (this will throw the error)

And the connectedCallback reaction for 1) is scheduled to the end of the BackupQueue (after "upgrading <x-div></x-div>"), so the upgrading for <x-div></x-div> is executed and the error is reported at the point that connectedCallback called.
(In reply to Edgar Chen [:edgar] from comment #8)
> For parser case, the upgrading and callbacks are all scheduled to BackupQueue.

Correct: In parser case, the connectedCallback is scheduled to BackupQueue, but upgrading is NOT (I always forget that creating an element from parser is calling HTMLConstructor synchronously ... :()

And yes, this test becomes failed after bug 1419305 (nice catch!!!). Because that the microtask checkpoint now happens at https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/parser/html/nsHtml5TreeOperation.cpp#428 just right before creating <x-div></x-div>...

Will fix the test.
Attachment #8928915 - Flags: feedback?(jdai)
Attachment #8928915 - Flags: feedback?(jdai) → feedback+
Attachment #8928915 - Flags: review?(bugs)
Comment on attachment 8928915 [details]
Bug 1416999 - Remove document.registerElement;

https://reviewboard.mozilla.org/r/200252/#review209090

::: commit-message-7e320:1
(Diff revision 6)
> +Bug 1416999 - Deprecate document.registerElement;

I would say:
Remove document.registerElement

::: dom/base/CustomElementRegistry.h:171
(Diff revision 6)
>  
>    // The list of attributes that this custom element observes.
>    nsTArray<RefPtr<nsAtom>> mObservedAttributes;
>  
>    // The prototype to use for new custom elements of this type.
>    JS::Heap<JSObject *> mPrototype;

Do we actually use mPrototype for something after the code removals in this patch.
Attachment #8928915 - Flags: review?(bugs) → review+
Comment on attachment 8928915 [details]
Bug 1416999 - Remove document.registerElement;

https://reviewboard.mozilla.org/r/200252/#review209090

> I would say:
> Remove document.registerElement

Ｗill do.

> Do we actually use mPrototype for something after the code removals in this patch.

No, no one use the mPrototype, will remove it in subsequence patch. Thanks for catching this.
(In reply to Olli Pettay [:smaug] from comment #13)
> ::: dom/base/CustomElementRegistry.h:171
> (Diff revision 6)
> >  
> >    // The list of attributes that this custom element observes.
> >    nsTArray<RefPtr<nsAtom>> mObservedAttributes;
> >  
> >    // The prototype to use for new custom elements of this type.
> >    JS::Heap<JSObject *> mPrototype;
> 
> Do we actually use mPrototype for something after the code removals in this
> patch.

mDocOrder[1] in CustomElementDefinition can be removed as well.

[1] https://www.w3.org/TR/2016/WD-custom-elements-20160226/#dfn-document-custom-element-order
Attachment #8932703 - Flags: review?(bugs)
Comment on attachment 8932703 [details]
Bug 1416999 - Remove the custom elements prototype and document order stored in CustomElementDefinition;

https://reviewboard.mozilla.org/r/203758/#review209292
Attachment #8932703 - Flags: review?(bugs) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8fb5eee0c3c5 -d d7a40b75ddf0: rebasing 436574:8fb5eee0c3c5 "Bug 1416999 - Remove document.registerElement; r=smaug"
merging dom/base/CustomElementRegistry.cpp
merging dom/base/CustomElementRegistry.h
merging dom/base/Element.cpp
merging dom/base/nsContentUtils.cpp
merging dom/base/nsContentUtils.h
merging dom/base/nsDocument.cpp
merging dom/base/test/chrome/chrome.ini
merging dom/tests/mochitest/webcomponents/mochitest.ini
warning: conflicts while merging dom/base/nsContentUtils.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/base/nsDocument.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/16d0cd656b05
Remove document.registerElement; r=smaug
https://hg.mozilla.org/integration/autoland/rev/1d970cdb9797
Remove the custom elements prototype and document order stored in CustomElementDefinition; r=smaug
https://hg.mozilla.org/mozilla-central/rev/16d0cd656b05
https://hg.mozilla.org/mozilla-central/rev/1d970cdb9797
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.