Add CustomElementsRegistry interface for custom element

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: jdai, Assigned: jdai)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: btpp-active, dom-ce-m1)

Attachments

(4 attachments, 13 obsolete attachments)

4.18 KB, patch
jdai
: review+
jdai
: feedback+
Details | Diff | Splinter Review
9.31 KB, patch
jdai
: review+
jdai
: feedback+
Details | Diff | Splinter Review
8.49 KB, patch
jdai
: review+
jdai
: feedback+
Details | Diff | Splinter Review
20.06 KB, patch
jdai
: review+
jdai
: feedback+
Details | Diff | Splinter Review
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
John will be working on this.
Whiteboard: btpp-fixlater
Assignee: nobody → jdai
Whiteboard: btpp-fixlater → btpp-active
Attachment #8768312 - Flags: feedback?(echen)
Attachment #8768313 - Attachment is obsolete: true
Attachment #8768313 - Flags: feedback?(echen)
Attachment #8768314 - Flags: feedback?(echen)
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.
Attachment #8768312 - Flags: feedback?(echen) → feedback+
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
Attachment #8768314 - Flags: feedback?(echen)
(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
Address comment 5 and carry on f+.
Attachment #8768312 - Attachment is obsolete: true
Attachment #8769544 - Flags: feedback+
Address comment 7.
Attachment #8768314 - Attachment is obsolete: true
Attachment #8769545 - Flags: feedback?(echen)
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.
Attachment #8769545 - Flags: feedback?(echen)
Attachment #8769544 - Attachment description: Bug 1275833 - Remove redundant trailing spaces. v2 → Part 1: Bug 1275833 - Remove redundant trailing spaces. v2
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
Attachment #8770081 - Flags: feedback?(echen) → feedback+
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.
Attachment #8770083 - Flags: feedback?(echen) → feedback+
Addressed comment 13 and carry on f+.
Attachment #8770081 - Attachment is obsolete: true
Attachment #8770371 - Flags: feedback+
Add WebComponentsBinding.h for CustomElementsRegistry.
Attachment #8770371 - Attachment is obsolete: true
Attachment #8771309 - Flags: feedback+
Attachment #8769544 - Flags: review?(wchen)
Attachment #8770372 - Flags: review?(wchen)
Attachment #8771309 - Flags: review?(wchen)
Attachment #8771310 - Flags: feedback?(echen)
Attachment #8771310 - Flags: feedback?(echen) → feedback+
Attachment #8771310 - Flags: review?(wchen)
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.
Attachment #8771309 - Flags: review?(bugs)
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.
Attachment #8771310 - Flags: review?(bugs)
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
Attachment #8771309 - Flags: review?(bugs) → review+
Attachment #8771310 - Flags: review?(bugs) → review+
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.
Attachment #8771309 - Flags: review?(wchen) → review+
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.
Attachment #8769544 - Flags: review?(wchen) → review+
Attachment #8770372 - Flags: review?(wchen) → review+
Attachment #8771310 - Flags: review?(wchen) → review+
Addressed comment 24.
Attachment #8769544 - Attachment is obsolete: true
Attachment #8773625 - Flags: review+
Attachment #8773625 - Flags: feedback+
Added reviewer name.
Attachment #8771310 - Attachment is obsolete: true
Attachment #8773629 - Flags: review+
Attachment #8773629 - Flags: feedback+
Whiteboard: btpp-active → btpp-active, dom-ce-m1
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
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
Attachment #8774364 - Attachment is obsolete: true
Attachment #8774364 - Flags: review?(michael.l.comella)
I'm so sorry for this wrong push.
Depends on: 1289872
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.