1275833 - Add CustomElementsRegistry interface for custom element

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[John Dai[:jdai]]

According to session 4.13.4[1], we need to add custom elements registry interface.

interface CustomElementsRegistry {
  [CEReactions] void define(DOMString name, Function constructor, optional ElementDefinitionOptions options);
  any get(DOMString name);
  Promise<void> whenDefined(DOMString name);
};

dictionary ElementDefinitionOptions {
  DOMString extends;
};


[1] https://html.spec.whatwg.org/multipage/scripting.html#customelementsregistry

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

John will be working on this.

Comment 2

[John Dai[:jdai]]

Attachment: Bug 1275833 - Remove redundant trailing spaces.

Comment 3

[John Dai[:jdai]]

Attachment: Bug 1275833 - Add CustomElementsRegistry interface for custom element v1.

Comment 4

[John Dai[:jdai]]

Attachment: Bug 1275833 - Add CustomElementsRegistry interface for custom element v1.

Comment 5

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Comment on attachment 8768312 [details] [diff] [review]
Bug 1275833 - Remove redundant trailing spaces.

Review of attachment 8768312 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsPIDOMWindow.h
@@ -589,5 @@
>      mParentTarget = nullptr;
>    }
>  
>    virtual void UpdateParentTarget() = 0;
> -

Keep a blank line here.

Comment 6

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Comment on attachment 8768314 [details] [diff] [review]
Bug 1275833 - Add CustomElementsRegistry interface for custom element v1.

Review of attachment 8768314 [details] [diff] [review]:
-----------------------------------------------------------------

Please split this patch into two,
1. Add window.customElements
2. Move CustomElement* from nsDocument to CustomElementRegistry

::: dom/base/CustomElementsRegistry.cpp
@@ +60,5 @@
> +                                    Function& aFunctionConstructor,
> +                                    const ElementDefinitionOptions& aOptions,
> +                                    JS::MutableHandle<JSObject*> aRetVal,
> +                                    ErrorResult& aRv)
> +{

Since you plan to implement this method in a separated bug, so
1. Add comment with bug id.
2. Throw a NS_ERROR_NOT_IMPLEMENTED error via "aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);"

@@ +67,5 @@
> +void
> +CustomElementsRegistry::Get(JSContext* aCx, const nsAString& aName,
> +                            JS::MutableHandle<JS::Value> aRetVal,
> +                            ErrorResult& aRv)
> +{

Ditto.

@@ +72,5 @@
> +}
> +
> +already_AddRefed<Promise>
> +CustomElementsRegistry::WhenDefined(const nsAString& name, ErrorResult& aRv)
> +{

Ditto and return nullptr.

::: dom/base/CustomElementsRegistry.h
@@ +130,5 @@
> +    DefinitionMap;
> +  // Hashtable for custom element definitions in web components.
> +  // Custom prototypes are stored in the compartment where
> +  // registerElement was called.
> +  DefinitionMap mCustomDefinitions;

We don't really use mCustomDefinitions here, please move the declaration to the bug that really use it.

::: dom/base/nsGlobalWindow.h
@@ +1839,5 @@
>    uint32_t                      mTimeoutPublicIdCounter;
>    uint32_t                      mTimeoutFiringDepth;
>    RefPtr<nsLocation>          mLocation;
>    RefPtr<nsHistory>           mHistory;
> +  RefPtr<mozilla::dom::CustomElementsRegistry> mCustomElements;

1. Since you hold a strong reference, please also add TRAVERSE and UNLINK marco for cycle collection, like https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#1879 and https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#1952

2. Reset mCustomElements to nullptr in nsGlobalWindow::CleanUp() and nsGlobalWindow::FreeInnerObjects().

::: dom/webidl/CustomElementsRegistry.webidl
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +interface CustomElementsRegistry {
> +  [Throws]
> +  object define(DOMString name, Function functionConstructor,

According to https://html.spec.whatwg.org/multipage/scripting.html#customelementsregistry, the return value of define() should be void.

::: dom/webidl/Window.webidl
@@ +35,5 @@
>    [Throws] attribute DOMString name;
>    [PutForwards=href, Unforgeable, Throws,
>     CrossOriginReadable, CrossOriginWritable] readonly attribute Location? location;
>    [Throws] readonly attribute History history;
> +  readonly attribute CustomElementsRegistry customElements;

1). This feature is ready yet, so please having a `Pref` or `Fun` to protect it not being exposed to the web by default, like https://dxr.mozilla.org/mozilla-central/source/dom/webidl/Document.webidl#259.

2). How about putting it into a separated `partial interface` and specify the spec link in the comment? like https://dxr.mozilla.org/mozilla-central/source/dom/webidl/Document.webidl#258

Comment 7

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar][:echen] from comment #6)
> Comment on attachment 8768314 [details] [diff] [review]
> Bug 1275833 - Add CustomElementsRegistry interface for custom element v1.
> 
> Review of attachment 8768314 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please split this patch into two,
> 1. Add window.customElements
> 2. Move CustomElement* from nsDocument to CustomElementRegistry

Will do.
> 
> ::: dom/base/CustomElementsRegistry.cpp
> @@ +60,5 @@
> > +                                    Function& aFunctionConstructor,
> > +                                    const ElementDefinitionOptions& aOptions,
> > +                                    JS::MutableHandle<JSObject*> aRetVal,
> > +                                    ErrorResult& aRv)
> > +{
> 
> Since you plan to implement this method in a separated bug, so
> 1. Add comment with bug id.
> 2. Throw a NS_ERROR_NOT_IMPLEMENTED error via
> "aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);"
Will do.
> 
> @@ +67,5 @@
> > +void
> > +CustomElementsRegistry::Get(JSContext* aCx, const nsAString& aName,
> > +                            JS::MutableHandle<JS::Value> aRetVal,
> > +                            ErrorResult& aRv)
> > +{
> 
> Ditto.
Will do.
> 
> @@ +72,5 @@
> > +}
> > +
> > +already_AddRefed<Promise>
> > +CustomElementsRegistry::WhenDefined(const nsAString& name, ErrorResult& aRv)
> > +{
> 
> Ditto and return nullptr.
Will do.
> 
> ::: dom/base/CustomElementsRegistry.h
> @@ +130,5 @@
> > +    DefinitionMap;
> > +  // Hashtable for custom element definitions in web components.
> > +  // Custom prototypes are stored in the compartment where
> > +  // registerElement was called.
> > +  DefinitionMap mCustomDefinitions;
> 
> We don't really use mCustomDefinitions here, please move the declaration to
> the bug that really use it.
Per offline discussion, mCustomDefinitions isn't related to this bug, it should be removed.
> 
> ::: dom/base/nsGlobalWindow.h
> @@ +1839,5 @@
> >    uint32_t                      mTimeoutPublicIdCounter;
> >    uint32_t                      mTimeoutFiringDepth;
> >    RefPtr<nsLocation>          mLocation;
> >    RefPtr<nsHistory>           mHistory;
> > +  RefPtr<mozilla::dom::CustomElementsRegistry> mCustomElements;
> 
> 1. Since you hold a strong reference, please also add TRAVERSE and UNLINK
> marco for cycle collection, like
> https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.
> cpp#1879 and
> https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.
> cpp#1952
Will do.
> 
> 2. Reset mCustomElements to nullptr in nsGlobalWindow::CleanUp() and
> nsGlobalWindow::FreeInnerObjects().
> 
> ::: dom/webidl/CustomElementsRegistry.webidl
> @@ +3,5 @@
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +interface CustomElementsRegistry {
> > +  [Throws]
> > +  object define(DOMString name, Function functionConstructor,
> 
> According to
> https://html.spec.whatwg.org/multipage/scripting.html#customelementsregistry,
> the return value of define() should be void.
> 
Will do.
> ::: dom/webidl/Window.webidl
> @@ +35,5 @@
> >    [Throws] attribute DOMString name;
> >    [PutForwards=href, Unforgeable, Throws,
> >     CrossOriginReadable, CrossOriginWritable] readonly attribute Location? location;
> >    [Throws] readonly attribute History history;
> > +  readonly attribute CustomElementsRegistry customElements;
> 
> 1). This feature is ready yet, so please having a `Pref` or `Fun` to protect
> it not being exposed to the web by default, like
> https://dxr.mozilla.org/mozilla-central/source/dom/webidl/Document.
> webidl#259.
Will do.
> 
> 2). How about putting it into a separated `partial interface` and specify
> the spec link in the comment? like
> https://dxr.mozilla.org/mozilla-central/source/dom/webidl/Document.webidl#258
I was following our spec[1], and the spec didn't put customElements to a separated `partial interface`. I will specify the spec link in the comment.

[1]https://html.spec.whatwg.org/#window

Comment 8

[John Dai[:jdai]]

Attachment: Part 1: Bug 1275833 - Remove redundant trailing spaces. v2

Address comment 5 and carry on f+.

Comment 9

[John Dai[:jdai]]

Attachment: Bug 1275833 - Add CustomElementsRegistry interface for custom element v1. v2

Address comment 7.

Comment 10

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Comment on attachment 8769545 [details] [diff] [review]
Bug 1275833 - Add CustomElementsRegistry interface for custom element v1. v2

Review of attachment 8769545 [details] [diff] [review]:
-----------------------------------------------------------------

In this new version, you introduce a separated preference to control the custom elements feature.
Custom element is an independent feature under webcomponent, having a separated way to control it is good.
 
But I have some questions:
1. What about the existing code and interface? Do they need to be controlled by this new preference, too?
2. Should custom element be enabled if nsDocument::IsWebComponentsEnabled returns true because custom element is a feature under webcomponent?

::: dom/base/CustomElementsRegistry.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim:set ts=12 sw=12 sts=12 et cindent: */

The configuration should be 'ts=2 sw=2 sts=2'.

@@ +85,5 @@
> +} // namespace dom
> +} // namespace mozilla
> +
> +namespace mozilla {
> +namespace dom {

Same namespace with above namespace block, Merge them together.

::: dom/base/CustomElementsRegistry.h
@@ +27,5 @@
> +} // namespace dom
> +} // namespace mozilla
> +
> +namespace mozilla {
> +namespace dom {

Ditto.

::: dom/base/nsDocument.cpp
@@ -470,5 @@
>      mOwnerData(aOwnerData)
>  {
>  }
>  
> -CustomElementDefinition::CustomElementDefinition(JSObject* aPrototype,

Please split this patch into two which makes individual changes more clear.
1. Add a new interface, window.customElements
2. Move CustomElementDefinition and CustomElementHashKey from nsDocument to CustomElementRegistry

::: dom/base/nsGlobalWindow.h
@@ +1839,5 @@
>    uint32_t                      mTimeoutPublicIdCounter;
>    uint32_t                      mTimeoutFiringDepth;
>    RefPtr<nsLocation>          mLocation;
>    RefPtr<nsHistory>           mHistory;
> +  RefPtr<mozilla::dom::CustomElementsRegistry> mCustomElements;

Reset mCustomElements to nullptr in nsGlobalWindow::CleanUp() and nsGlobalWindow::FreeInnerObjects().

::: dom/webidl/CustomElementsRegistry.webidl
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// https://html.spec.whatwg.org/#dom-window-customelements
> +interface CustomElementsRegistry {

Protect entire interface and also window.customElements by adding Pref or Fun.
And since we protect both interface and window.customElements, there is no way to reference CustomElementsRegistry if the preference is false.
thus, the [Pref=...] for each CustomElementsRegistry's property can be removed.

::: dom/webidl/Window.webidl
@@ +35,5 @@
>    [Throws] attribute DOMString name;
>    [PutForwards=href, Unforgeable, Throws,
>     CrossOriginReadable, CrossOriginWritable] readonly attribute Location? location;
>    [Throws] readonly attribute History history;
> +  readonly attribute CustomElementsRegistry customElements;

As I mentioned above, add [Pref=..] protection for this attribute.

Comment 11

[John Dai[:jdai]]

Attachment: Part 2: Bug 1275833 - Add window.customElements and CustomElementsRegistry interface for custom element v1. v1

Addressed comment 10.

Comment 12

[John Dai[:jdai]]

Attachment: Part 3: Bug 1275833 - Move CustomElementDefinition and CustomElementHashKey from nsDocument to CustomElementRegistry. v1

Addressed comment 10.

Comment 13

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Comment on attachment 8770081 [details] [diff] [review]
Part 2: Bug 1275833 - Add window.customElements and CustomElementsRegistry interface for custom element v1. v1

Review of attachment 8770081 [details] [diff] [review]:
-----------------------------------------------------------------

It seems that we should also check "dom.webcomponents.customelement.enabled" in https://dxr.mozilla.org/mozilla-central/rev/88bebcaca249aeaca9197382e89d35b02be8292e/dom/base/nsDocument.cpp#4778-4780.

::: dom/base/CustomElementsRegistry.cpp
@@ +22,5 @@
> +
> +/* static */ bool
> +CustomElementsRegistry::IsCustomelementEnabled(JSContext* aCx, JSObject* aObject)
> +{
> +  JS::Rooted<JSObject*> obj(aCx, aObject);

Remove this unused parameter.

@@ +92,5 @@
> +  // TODO: This function will be implemented in bug 1275839
> +  aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
> +  return nullptr;
> +}
> +} // namespace dom

add a blank line before this.

::: dom/base/CustomElementsRegistry.h
@@ +31,5 @@
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(CustomElementsRegistry)
> +
> +public:
> +  static bool IsCustomelementEnabled(JSContext* aCx, JSObject* aObject);

s/IsCustomelementEnabled/IsCustomElementEnabled/

::: dom/webidl/CustomElementsRegistry.webidl
@@ +6,5 @@
> +[Func="CustomElementsRegistry::IsCustomelementEnabled"]
> +interface CustomElementsRegistry {
> +  [Throws]
> +  void define(DOMString name, Function functionConstructor,
> +                optional ElementDefinitionOptions options);

Nit: align arguments

Comment 14

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Comment on attachment 8770083 [details] [diff] [review]
Part 3: Bug 1275833 - Move CustomElementDefinition and CustomElementHashKey from nsDocument to CustomElementRegistry. v1

Review of attachment 8770083 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/CustomElementsRegistry.cpp
@@ +111,1 @@
>  } // namespace dom

Nit: add a blank line before this.

Comment 15

[John Dai[:jdai]]

Attachment: Part 2: Bug 1275833 - Add window.customElements and CustomElementsRegistry interface for custom element v1. v2

Addressed comment 13 and carry on f+.

Comment 16

[John Dai[:jdai]]

Attachment: Part 3: Bug 1275833 - Move CustomElementDefinition and CustomElementHashKey from nsDocument to CustomElementRegistry. v2

Addressed comment 14 and carry on f+.

Comment 17

[John Dai[:jdai]]

Attachment: Part 2: Bug 1275833 - Add window.customElements and CustomElementsRegistry interface for custom element v1. v3

Add WebComponentsBinding.h for CustomElementsRegistry.

Comment 18

[John Dai[:jdai]]

Attachment: Part 4: Bug 1275833 - Update wpt and test_interfaces.html.

Comment 19

[John Dai[:jdai]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c6c60fe19b476eb8de48efb1dd1474a41772360&filter-tier=1

Comment 20

[John Dai[:jdai]]

Comment on attachment 8771309 [details] [diff] [review]
Part 2: Bug 1275833 - Add window.customElements and CustomElementsRegistry interface for custom element v1. v3

This also require DOM peer review since I added a CustomElementsRegistry WebIDL file. Hi Olli, may I have your review? Thank you.

Comment 21

[John Dai[:jdai]]

Comment on attachment 8771310 [details] [diff] [review]
Part 4: Bug 1275833 - Update wpt and test_interfaces.html.

This also require DOM peer review since I added a CustomElementsRegistry WebIDL file. Hi Olli, may I have your review? Thank you.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8771309 [details] [diff] [review]
Part 2: Bug 1275833 - Add window.customElements and CustomElementsRegistry interface for custom element v1. v3

> //http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/custom/index.html#dfn-document-register
> partial interface Document {
>-    [Throws, Func="nsDocument::IsWebComponentsEnabled"]
>+    [Throws, Func="CustomElementsRegistry::IsCustomElementEnabled"]
>     object registerElement(DOMString name, optional ElementRegistrationOptions options);
Could you perhaps add a comment that this is deprecated from CustomElements v0

r+ for the .webidl

Comment 23

[William Chen [:wchen]]

Comment on attachment 8771309 [details] [diff] [review]
Part 2: Bug 1275833 - Add window.customElements and CustomElementsRegistry interface for custom element v1. v3

Review of attachment 8771309 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/CustomElementsRegistry.h
@@ +13,5 @@
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsWrapperCache.h"
> +#include "mozilla/dom/FunctionBinding.h"
> +#include <set>

Why do we need to include <set>?

::: testing/profiles/prefs_general.js
@@ +49,5 @@
>  user_pref("media.gmp-manager.url.override", "http://%(server)s/dummy-gmp-manager.xml");
>  user_pref("dom.w3c_touch_events.enabled", 1);
>  user_pref("dom.undo_manager.enabled", true);
>  user_pref("dom.webcomponents.enabled", true);
> +user_pref("dom.webcomponents.customelement.enabled", true);

Can you please rename this from customelement -> customelements? and also ::IsCustomElementEnabled -> ::IsCustomElementsEnabled.

Comment 24

[William Chen [:wchen]]

Comment on attachment 8769544 [details] [diff] [review]
Part 1: Bug 1275833 - Remove redundant trailing spaces. v2

Review of attachment 8769544 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.h
@@ +204,5 @@
>  
>    // stack depth at which timeout is firing
>    uint32_t mFiringDepth;
>  
> +  //

There isn't any point to keeping this line. We might as well get rid of it.

Comment 25

[John Dai[:jdai]]

Attachment: Part 1 (Final): Bug 1275833 - Remove redundant trailing spaces. r=wchen

Addressed comment 24.

Comment 26

[John Dai[:jdai]]

Attachment: Part 2 (Final): Bug 1275833 - Add window.customElements and CustomElementsRegistry interface for custom element v1. r=smaug, wchen

Addressed comment 22 and comment 23.

Comment 27

[John Dai[:jdai]]

Attachment: Part 3 (Final): Bug 1275833 - Move CustomElementDefinition and CustomElementHashKey from nsDocument to CustomElementRegistry. r=wchen

Added reviewer name.

Comment 28

[John Dai[:jdai]]

Attachment: Part 4 (Final): Bug 1275833 - Update wpt and test_interfaces.html. r=smaug, wchen

Added reviewer name.

Comment 29

[John Dai[:jdai]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb5e1e74796ff4f0d04baa02f8a31e9d203c9949&filter-tier=1

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Your Try push is showing a real-looking Hazard Analysis failure.

Function '_ZN7mozilla3dom22CustomElementsRegistry23IsCustomElementsEnabledEP9JSContextP8JSObject$uint8 mozilla::dom::CustomElementsRegistry::IsCustomElementsEnabled(JSContext*, JSObject*)' has unrooted 'aObject' of type 'JSObject*' live across GC call '_ZN7mozilla11Preferences7GetBoolEPKcb$uint8 mozilla::Preferences::GetBool(int8*, uint8)' at dom/base/CustomElementsRegistry.cpp:27
    CustomElementsRegistry.cpp:27: Call(1,2, __temp_2 := GetBool("dom.webcomponents.customelements.enabled",0)) [[GC call]]
    CustomElementsRegistry.cpp:27: Assume(2,3, __temp_2*, false)
    CustomElementsRegistry.cpp:28: Call(3,4, __temp_3 := IsWebComponentsEnabled(aCx*,aObject*))
GC Function: _ZN7mozilla11Preferences7GetBoolEPKcb$uint8 mozilla::Preferences::GetBool(int8*, uint8)
    uint32 mozilla::Preferences::GetBool(int8*, uint8*)
    PREF_GetBoolPref
    PrefHashEntry* pref_HashTableLookup(int8*)
    PLDHashEntryHdr* PLDHashTable::Search(void*)
    PLDHashEntryHdr* PLDHashTable::SearchTable(void*, uint32) [with PLDHashTable::SearchReason Reason = (PLDHashTable::SearchReason)0u; PLDHashNumber = unsigned int]
    IndirectCall: matchEntry

Comment 31

[John Dai[:jdai]]

Attachment: Part 2 (Final): Bug 1275833 - Add window.customElements and CustomElementsRegistry interface for custom element v1. r=smaug, wchen

Addressed comment 30, fixed Hazard Analysis failure.
Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=188480bee83323a06cb2b26d4b98a01f84f5796f&filter-tier=1

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/970ad562913f
Remove redundant trailing spaces. r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/341f218569ba
Add window.customElements and CustomElementsRegistry interface for custom element v1. r=smaug, wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/13a2ea0523a8
Move CustomElementDefinition and CustomElementHashKey from nsDocument to CustomElementRegistry. r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/b419dd976981
Update wpt and test_interfaces.html. r=smaug, wchen

Comment 33

[SUN Haitao]

Attachment: Bug 1275833 - Make LauncherActivity use the main taskAffinity; r?

Review commit: https://reviewboard.mozilla.org/r/66840/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66840/

Comment 34

[SUN Haitao]

I'm so sorry for this wrong push.

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/970ad562913f
https://hg.mozilla.org/mozilla-central/rev/341f218569ba
https://hg.mozilla.org/mozilla-central/rev/13a2ea0523a8
https://hg.mozilla.org/mozilla-central/rev/b419dd976981