Implement CustomElementsRegistry define function

RESOLVED FIXED in Firefox 51

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: jdai, Assigned: edgar)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

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

Attachments

(2 attachments, 16 obsolete attachments)

85.98 KB, patch
edgar
: review+
Details | Diff | Splinter Review
51.95 KB, patch
edgar
: review+
Details | Diff | Splinter Review
Reporter

Description

3 years ago
According to session 4.13.4[1], we need to implement CustomElementsRegistry define function, which are window.customElements.define(name, constructor) and window.customElements.define(name, constructor, { extends: baseLocalName }).


[1]https://html.spec.whatwg.org/multipage/scripting.html#element-definition
(I'm marking all the Custom Elements bugs as "backlog" but that's just to indicate they're not something we're fixing urgently and no comment on priority or anything.)
Whiteboard: btpp-backlog
Assignee

Updated

3 years ago
Depends on: 1275833
Assignee

Updated

3 years ago
Assignee: nobody → echen
Assignee

Comment 2

3 years ago
Posted patch WIP patch, v1 (obsolete) — Splinter Review
Just attach WIP patch which checks the prototype. There are still a lot of works need to be done.
Assignee

Comment 3

3 years ago
Posted patch WIP patch, v2 (obsolete) — Splinter Review
Attachment #8772684 - Attachment is obsolete: true
Reporter

Updated

3 years ago
Blocks: 1275838
Reporter

Updated

3 years ago
Depends on: 1275832
Reporter

Updated

3 years ago
Blocks: 1275839
Priority: -- → P2
Whiteboard: btpp-backlog → btpp-backlog, dom-ce-m1
Assignee

Comment 5

3 years ago
Comment on attachment 8774612 [details] [diff] [review]
Part 1: Move custom element codes from nsDocument to CustomElementsRegistry, v1

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

Hi William,
In this bug I am going to implement window.customElements.define() [1].
Since we already have existing codes for custom elements, I plan to reuse them and adjust data-structure/flow to follow the latest spec.

The very first step I am planing to do is moving the data structure from nsDocument to CustomElementsRegistry (As this part1 patch).
And then implement window.customElements.define() with some adjustment on data-structure (this will happen in subsequent part).

I am still working on subsequent part, will attach soon.
But I would like to have your feedback about part1 first.

Here is some details about this part1 patch,
1. Definition/candidate/processingStack now is associated with a window, not a document.
2. I keep the data-structure and call flow the same as mush as possible.
3. Most of the behavior are the same as before [2], except the [3].
   But I think this change make sense because in the spec [4] a element created by a document without docshell won't
   run the custom element setup, so createdCallback won't be triggered in such case.

Please let me know if anything is unclear. Thank you.

[1] https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregistry-define
[2] Verified by mochitest tests.
[3] Please see test_document_shared_registry.html changes in this patch.
[4] https://dom.spec.whatwg.org/#dom-document-createelement
Attachment #8774612 - Flags: feedback?(wchen)
Assignee

Comment 8

3 years ago
These patches aren't perfect enough for review.
But they can show you what the whole picture looks like and what I am going to do in this bug.
Comment on attachment 8775095 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v1

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

Overall, it looks like you are on the right track.

::: dom/base/CustomElementsRegistry.cpp
@@ +612,5 @@
> +    // its subclass is not allowed.
> +    JS::Rooted<JS::Value> prototypev(cx);
> +    if (!JS_GetProperty(cx, constructorUnwrapped, "prototype", &prototypev)) {
> +      JS_ClearPendingException(cx);
> +      aRv.Throw(NS_ERROR_UNEXPECTED);

I don't think we should throw here. The exception thrown wouldn't match the exception thrown in the spec by step 13.

@@ +622,5 @@
> +      return;
> +    }
> +
> +    constructorPrototype = &prototypev.toObject();
> +    if (IsDOMIfaceAndProtoClass(js::GetObjectClass(constructorPrototype))) {

The duplicate code should be factored out.

@@ +736,5 @@
> +
> +    JS::Rooted<JS::Value> connectedCallbackValue(cx);
> +    if (!JS_GetProperty(cx, constructorPrototype, "connectedCallback", &connectedCallbackValue)) {
> +      JS_ClearPendingException(cx);
> +      aRv.Throw(NS_ERROR_UNEXPECTED);

Spec says to rethrow any exceptions we get from getting the callbacks.

::: dom/base/CustomElementsRegistry.h
@@ +100,5 @@
>    typedef CustomElementHashKey *KeyType;
>    typedef const CustomElementHashKey *KeyTypePointer;
>  
> +  CustomElementHashKey(nsIAtom *aAtom)
> +    : mAtom(aAtom)

Since we don't need the namespace anymore we can just use a nsPtrHashKey<nsIAtom> instead of CustomElementHashKey.
Attachment #8775095 - Flags: feedback+
Attachment #8774612 - Flags: feedback?(wchen)
Assignee

Comment 10

3 years ago
Thanks for your feedback. Will address your comment.
Assignee

Comment 11

3 years ago
Attachment #8774612 - Attachment is obsolete: true
Attachment #8775093 - Attachment is obsolete: true
Assignee

Comment 12

3 years ago
Attachment #8775095 - Attachment is obsolete: true
Assignee

Comment 14

3 years ago
Address comment #9.
Attachment #8779715 - Attachment is obsolete: true
Assignee

Comment 15

3 years ago
There is one more thing, we need to provide a way to lookup definition by constructor for
1). step 6 of https://html.spec.whatwg.org/multipage/scripting.html#concept-custom-element-definition-lifecycle-callbacks
and 2). step 2 of https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor
Assignee

Comment 16

3 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #15)
> There is one more thing, we need to provide a way to lookup definition by
> constructor for
> 1). step 6 of
> https://html.spec.whatwg.org/multipage/scripting.html#concept-custom-element-
> definition-lifecycle-callbacks
> and 2). step 2 of
> https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor

Okay, I plan to support this in bug 1274159 or file a new bug if needed.
Assignee

Updated

3 years ago
Attachment #8780465 - Flags: review?(wchen)
Assignee

Comment 17

3 years ago
Comment on attachment 8780466 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v3

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

Hi William,
With this patch, we can pass most of wpts for define [1], except some test cases for valid custom element names which will be handled in bug 1275832 and registering duplicated constructor (as I mentioned in comment #15 and comment #16).
Thank you.

[1] http://w3c-test.org/custom-elements/custom-elements-registry/define.html
Attachment #8780466 - Flags: review?(wchen)
Assignee

Comment 18

3 years ago
dom/html/test/test_bug1081037.html failed in try run [1].

1287 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_bug1081037.html | non-configurable and not-writable constructor property (thrown:TypeError: can't redefine non-configurable property "constructor")
1290 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_bug1081037.html | number of getOwnPropertyDescriptor calls from registerElement: 0 - got +0, expected 1

Investigating ...

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d58e35653c1ddc7595add1a9e22a766ced965315&filter-tier=1&group_state=expanded
Assignee

Comment 19

3 years ago
Comment on attachment 8780466 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v3

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

::: dom/base/nsDocument.cpp
@@ +5774,3 @@
>    JS::Rooted<JSObject*> protoObject(aCx);
>    {
> +    JSAutoCompartment ac(aCx, global);

Hmm, While I am investigating the failures in comment #18, I found this change causes HTMLElementProto doesn't be wrappered into caller's compartment.
So canceling the review request first. Will provide a new version to fix this and also comment #18.
Attachment #8780466 - Flags: review?(wchen)
Assignee

Comment 20

3 years ago
I got another test failure in dom/base/test/chrome/test_registerElement_content.xul [1].
It seems because we didn't wrapper life-cycle callbacks into correct compartment (caller's compartment maybe). Investigating ...

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/test/chrome/test_registerElement_content.xul
Assignee

Comment 22

3 years ago
Sorry, I just realized that I uploaded a wrong part1 patch. Attach correct one.
Attachment #8780465 - Attachment is obsolete: true
Attachment #8780465 - Flags: review?(wchen)
Attachment #8783828 - Flags: review?(wchen)
Assignee

Comment 23

3 years ago
Comment on attachment 8783536 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v4

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

::: dom/base/CustomElementsRegistry.h
@@ +190,5 @@
> +  void UpgradeCandidates(JSContext* aCx,
> +                         nsIAtom* aKey,
> +                         CustomElementDefinition* aDefinition);
> +
> +  typedef nsClassHashtable<nsPtrHashKey<nsIAtom>, CustomElementDefinition>

nsIAtom is a reference-counted class, but nsPtrHashKey doesn't own a reference of nsIAtom. Hence, key may be released unexpectedly which cause intermittent failure in dom/tests/mochitest/webcomponents/test_document_register_lifecycle.html. We should better use nsISupportsHashKey.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6412ff03b5b0c6060c7e3afad6a62cd4ad7d7f2a&group_state=expanded&filter-tier=1&selectedJob=26133979
Assignee

Comment 24

3 years ago
Addressed comment #23 and try result [1] looks perfect.

I apologize for the review request spam. This version should really be the final one. :p 

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d8e63ef0e396edb0bd6a72c65e1e9d47cf16471&group_state=expanded&filter-tier=1
Attachment #8783536 - Attachment is obsolete: true
Attachment #8783536 - Flags: review?(wchen)
Attachment #8783949 - Flags: review?(wchen)
Assignee

Updated

3 years ago
Blocks: 1274159
Attachment #8783828 - Flags: review?(wchen) → review+
Comment on attachment 8783949 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v5

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

Patch looks good, just have some comments and questions.

::: dom/base/CustomElementsRegistry.cpp
@@ +277,4 @@
>      return;
>    }
>  
> +  nsTArray<nsWeakPtr>* unresolved = mCandidatesMap.Get(typeName);

You should be able to use LookupOrAdd here.

@@ +466,5 @@
>  
> +void
> +CustomElementsRegistry::UpgradeCandidates(JSContext* aCx,
> +                                          nsIAtom* aKey,
> +                                          CustomElementDefinition* aDefinition)

The v0 upgrade is quite different from v1 upgrade, are you planning on updating the algorithm in another patch?

@@ +555,5 @@
> +        aRv.StealExceptionFromJSContext(cx); \
> +        return; \
> +      } \
> +    } \
> +  } \

Can you implement these helpers as static methods/functions instead of macros?

@@ +648,5 @@
> +  } // Leave constructorProtoUnwrapped's compartment.
> +
> +  /**
> +   * 3. If name is not a valid custom element name, then throw a "SyntaxError"
> +   *    DOMException and abort these steps.

The next few steps have moved up 1 spot.

@@ +669,5 @@
> +  /**
> +   * 6. If this CustomElementsRegistry contains an entry with constructor constructor,
> +   *    then throw a "NotSupportedError" DOMException and abort these steps.
> +   */
> +  // TODO: Bug 1274159 - Add [HTMLConstructor] to support custom element feature.

Why does this step depend on bug 1274159?

@@ +708,5 @@
> +      return;
> +    }
> +
> +    localName.Assign(aOptions.mExtends.Value());
> +  }

Step 2 in this patch moves here and you should also add the "element definition is running" flag.

@@ +782,5 @@
> +   */
> +  mCustomDefinitions.Put(nameAtom, definition);
> +
> +  // 16. 17. 18. Upgrade candidates
> +  UpgradeCandidates(cx, nameAtom, definition);

UpgradeCandidate will try to upgrade immediately, the spec says that elements should be upgraded in an element upgrade reaction so the timing will be a bit different here. Is this going to be done in another patch?
Attachment #8783949 - Flags: review?(wchen)
Assignee

Comment 26

3 years ago
Comment on attachment 8783949 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v5

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

::: dom/base/CustomElementsRegistry.cpp
@@ +277,4 @@
>      return;
>    }
>  
> +  nsTArray<nsWeakPtr>* unresolved = mCandidatesMap.Get(typeName);

Got it.

@@ +466,5 @@
>  
> +void
> +CustomElementsRegistry::UpgradeCandidates(JSContext* aCx,
> +                                          nsIAtom* aKey,
> +                                          CustomElementDefinition* aDefinition)

Yes, I am planning on handling v1 upgrade algorithm in a separated bug.
Will file the bug soon and add some comments with bug id.

@@ +555,5 @@
> +        aRv.StealExceptionFromJSContext(cx); \
> +        return; \
> +      } \
> +    } \
> +  } \

Will do.

@@ +648,5 @@
> +  } // Leave constructorProtoUnwrapped's compartment.
> +
> +  /**
> +   * 3. If name is not a valid custom element name, then throw a "SyntaxError"
> +   *    DOMException and abort these steps.

Ah, yes, will update the comments. 
And it is because the checks for HTMLElement at definition time was removed from spec in https://github.com/w3c/webcomponents/issues/541 few weeks ago. I think we should also remove them.

@@ +669,5 @@
> +  /**
> +   * 6. If this CustomElementsRegistry contains an entry with constructor constructor,
> +   *    then throw a "NotSupportedError" DOMException and abort these steps.
> +   */
> +  // TODO: Bug 1274159 - Add [HTMLConstructor] to support custom element feature.

Step 3 of HTMLConstructor [1] also needs a way to look up definition by using constructor. So I plans to figure out a solution to support both of them in bug 1274159 (maybe create another hash table which uses constructor as key).

[1] https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor

@@ +708,5 @@
> +      return;
> +    }
> +
> +    localName.Assign(aOptions.mExtends.Value());
> +  }

Will add "element definition is running" flag.

@@ +782,5 @@
> +   */
> +  mCustomDefinitions.Put(nameAtom, definition);
> +
> +  // 16. 17. 18. Upgrade candidates
> +  UpgradeCandidates(cx, nameAtom, definition);

Yes, I am planing on implementing v1 upgrade in another bug.
Assignee

Comment 27

3 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #26)
> Comment on attachment 8783949 [details] [diff] [review]
> Part 2: Implement CustomElementsRegistry define function, v5
> 
> Review of attachment 8783949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/CustomElementsRegistry.cpp
> 
> @@ +466,5 @@
> >  
> > +void
> > +CustomElementsRegistry::UpgradeCandidates(JSContext* aCx,
> > +                                          nsIAtom* aKey,
> > +                                          CustomElementDefinition* aDefinition)
> 
> Yes, I am planning on handling v1 upgrade algorithm in a separated bug.
> Will file the bug soon and add some comments with bug id.
> 
> @@ +782,5 @@
> > +   */
> > +  mCustomDefinitions.Put(nameAtom, definition);
> > +
> > +  // 16. 17. 18. Upgrade candidates
> > +  UpgradeCandidates(cx, nameAtom, definition);
> 
> Yes, I am planing on implementing v1 upgrade in another bug.

Filed bug 1299363 for v1 upgrade.
Assignee

Comment 29

3 years ago
Attachment #8783949 - Attachment is obsolete: true
Assignee

Comment 30

3 years ago
Posted patch interdiff_part2_v5_v6 (obsolete) — Splinter Review
Assignee

Comment 31

3 years ago
Comment on attachment 8786683 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v6

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

Address review comment #25
- Use LookupOrAdd in RegisterUnresolv().
- Implement helpers as static function.
- Implement "element definition is running" flag.
- Sync comment with latest spec.
- Remove prototype checks in Define() per spec change [1] and add the protoptype check in RegisterElement() back.
  (Corresponding wpt tests are removed from upstream [2], but the change isn't merged to our codebase yet.)  

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9ed15a7eda7276296a3cf60d0a54d96a57e0536&filter-tier=1&group_state=expanded 

[1] https://github.com/w3c/webcomponents/issues/541
[2] https://github.com/w3c/web-platform-tests/commit/a64b7d648bf218d7882461352dad6fce99df6ac5
Attachment #8786683 - Flags: review?(wchen)
Comment on attachment 8786683 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v6

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

Looks good, but I'd like to see the updated patch before r+

::: dom/base/CustomElementsRegistry.cpp
@@ +229,5 @@
>  CustomElementsRegistry::sProcessingStack;
>  
>  CustomElementsRegistry::CustomElementsRegistry(nsPIDOMWindowInner* aWindow)
>   : mWindow(aWindow)
> + , IsCustomDefinitionRunning(false)

mIsCustomDefinitionRunning

@@ +528,5 @@
> +  "detachedCallback"
> +};
> +
> +static bool
> +CheckLifeCycleCallbacks(JSContext* aCx,

Instead of returning a success bool, the callers can check if aRv failed. When you implement the callbacks, you will probably want to return the callback from this function to store in the definition.

@@ +746,5 @@
> +        aRv.Throw(NS_ERROR_FAILURE);
> +        break;
> +      }
> +    } // Leave constructorProtoUnwrapped's compartment.
> +  } while(false);

Instead of using this pattern, make a small RAII class to handle setting/unsetting the "is custom element definition running" flag and return whenever aRv failed.
Attachment #8786683 - Flags: review?(wchen)
Assignee

Comment 33

3 years ago
Comment on attachment 8786683 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v6

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

::: dom/base/CustomElementsRegistry.cpp
@@ +746,5 @@
> +        aRv.Throw(NS_ERROR_FAILURE);
> +        break;
> +      }
> +    } // Leave constructorProtoUnwrapped's compartment.
> +  } while(false);

Oh, yes, using RAII pattern is a much better way. Will do.
Assignee

Comment 34

3 years ago
Attachment #8786683 - Attachment is obsolete: true
Assignee

Comment 35

3 years ago
Posted patch interdiff_part2_v6_v7 (obsolete) — Splinter Review
Attachment #8786687 - Attachment is obsolete: true
Assignee

Comment 36

3 years ago
Comment on attachment 8787094 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v7

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

Address review comment #32
- s/IsCustomDefinitionRunning/mIsCustomDefinitionRunning/.
- CheckLifeCycleCallbacks() don't need to return a success bool.
- make a small RAII class to handle setting/unsetting the "is custom element definition running" flag.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ae770e1d30dfb10c0cc3708472d4463676f67f4&filter-tier=1&group_state=expanded
Attachment #8787094 - Flags: review?(wchen)
Comment on attachment 8787094 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v7

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

::: dom/base/CustomElementsRegistry.h
@@ +219,5 @@
> +  // It is used to prevent reentrant invocations of element definition.
> +  bool mIsCustomDefinitionRunning;
> +
> +private:
> +  class FlagMarker {

Add MOZ_RAII and final annotation to the class. Please also rename the class to AutoSomething to make it more obvious that this is an RAII class, maybe AutoSetRunningFlag. This class can also be simpler, you can just hold a raw pointer to the CustomElementRegistry and set the flag to true in the constructor and false in the destructor. Also add an assertion in the constructor that the flag is initially false.
Attachment #8787094 - Flags: review?(wchen) → review+
Assignee

Comment 38

3 years ago
Address comment #37. Thanks for the review, William.
Attachment #8787094 - Attachment is obsolete: true
Attachment #8787095 - Attachment is obsolete: true
Attachment #8787489 - Flags: review+

Comment 40

3 years ago
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f2f37ec0fd3
Part 1: Move custom element codes from nsDocument to CustomElementsRegistry; r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cc407277fd0
Part 2: Implement CustomElementsRegistry define function; r=wchen

Comment 41

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1f2f37ec0fd3
https://hg.mozilla.org/mozilla-central/rev/9cc407277fd0
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
this increased the num_constructors:
== Change summary for alert #2944 (as of September 02 2016 02:52 UTC) ==

Regressions:

  1%  compiler_metrics num_constructors linux64 pgo       170 -> 171
  1%  compiler_metrics num_constructors linux32 opt       170 -> 171
  0%  compiler_metrics num_constructors linux32 debug     231 -> 232
  0%  compiler_metrics num_constructors linux64 debug     231 -> 232

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=2944

:froydnj, can you make sure this is proper?
Flags: needinfo?(nfroyd)
(In reply to Joel Maher ( :jmaher) from comment #42)
> this increased the num_constructors:
> == Change summary for alert #2944 (as of September 02 2016 02:52 UTC) ==
> 
> Regressions:
> 
>   1%  compiler_metrics num_constructors linux64 pgo       170 -> 171
>   1%  compiler_metrics num_constructors linux32 opt       170 -> 171
>   0%  compiler_metrics num_constructors linux32 debug     231 -> 232
>   0%  compiler_metrics num_constructors linux64 debug     231 -> 232
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=2944
> 
> :froydnj, can you make sure this is proper?

I think this is an unfortunate side-effect of moving nsDocument::sProcessingStack to CustomElementsRegistry::sProcessingStack.  Due to unified compilation, whatever unified subset of files CustomElementsRegistry.cpp belongs to didn't have a static constructor, whereas whatever unified subset of files nsDocument.cpp belongs to already had a static constructor (from sProcessingStack and whatever else lives in those unified files).  Moving sProcessingStack into CustomElementsRegistry.cpp, then, caused a static constructor to be created for that subset of unified files, while not eliminating the one in nsDocument.cpp's subset.  So there's not any change in the amount of code we're running, merely where it's living.
Flags: needinfo?(nfroyd)
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.