Closed Bug 1275839 Opened 7 years ago Closed 6 years ago

Implement CustomElementsRegistry whenDefined function

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jdai, Assigned: jdai)

References

Details

(Whiteboard: dom-ce-m1)

Attachments

(1 file, 9 obsolete files)

According to session 4.13.4[1], we need to implement CustomElementsRegistry whenDefined function.

[1] https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregistry-whendefined
(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
Depends on: 1275833
Assignee: nobody → jdai
Depends on: 1275835
Priority: -- → P2
Whiteboard: btpp-backlog → dom-ce-m1
Attached patch wip, v1 (obsolete) — Splinter Review
Depends on: 1275832
Attachment #8774640 - Attachment is obsolete: true
Attachment #8793217 - Flags: feedback?(echen)
Comment on attachment 8793217 [details] [diff] [review]
Bug 1275839 - Implement CustomElementsRegistry whenDefined function

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

::: dom/base/CustomElementsRegistry.cpp
@@ +804,5 @@
>     *     2. Resolve promise with undefined.
>     *     3. Delete the entry with key name from this CustomElementsRegistry's
>     *        when-defined promise map.
>     */
>    // TODO: Bug 1275839 - Implement CustomElementsRegistry whenDefined function

Remove this comment.

@@ +830,4 @@
>  {
> +  RefPtr<Promise> promise = CreatePromise();
> +  if (!promise) {
> +    return nullptr;

This api should always return a promise, if we are not able to create a promise for some reason, I think we should throw an exception instead of just returning a nullptr.

@@ +839,5 @@
> +    return promise.forget();
> +  }
> +
> +  CustomElementDefinition* data = mCustomDefinitions.Get(nameAtom);
> +  if (data) {

if (mCustomDefinitions.Get(nameAtom)) {

@@ +849,5 @@
> +  if (!whenDefineValue) {
> +    WhenDefinedPromiseData* whenDefineData = new WhenDefinedPromiseData(promise);
> +    mWhenDefinedPromiseMap.Put(nameAtom, whenDefineData);
> +    promise = whenDefineData->mPromise;
> +    return promise.forget();

You can just reuse whenDefineValue.
Then we don't need to have another return here. The return outside if block can cover both case (already existing or need to create a new one).

::: dom/base/CustomElementsRegistry.h
@@ +202,5 @@
>    // Custom prototypes are stored in the compartment where
>    // registerElement was called.
>    DefinitionMap mCustomDefinitions;
>  
> +  typedef nsClassHashtable<nsISupportsHashKey, WhenDefinedPromiseData>

For reference-counted class, you can use nsRefPtrHashtable, then we don't need introduce the WhenDefinePromiseData to hold a reference of a Promise.
Attachment #8793217 - Flags: feedback?(echen)
Addressed comment #4.
Attachment #8793217 - Attachment is obsolete: true
Attachment #8794127 - Flags: feedback?(echen)
Comment on attachment 8794127 [details] [diff] [review]
Bug 1275839 - Implement CustomElementsRegistry whenDefined function. v2

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

::: dom/base/CustomElementsRegistry.cpp
@@ +808,5 @@
> +  if (mWhenDefinedPromiseMap.Get(nameAtom, getter_AddRefs(promise))) {
> +    promise->MaybeResolveWithUndefined();
> +    RefPtr<Promise> doomed;
> +    mWhenDefinedPromiseMap.Remove(nameAtom, getter_AddRefs(doomed));
> +  }

You can simplify this as shown below,

RefPtr<Promise> promise;
mWhenDefinedPromiseMap.Remove(nameAtom, getter_AddRefs(promise));
if (promise) {
  promise->MaybeResolveWithUndefined();
}

@@ +852,5 @@
> +  if (!mWhenDefinedPromiseMap.Get(nameAtom, getter_AddRefs(p))) {
> +    mWhenDefinedPromiseMap.Put(nameAtom, promise);
> +  } else {
> +    promise = p;
> +  }

if (mWhenDefinedPromiseMap.Contains(nameAtom)) {
  mWhenDefinedPromiseMap.Get(nameAtom, getter_AddRefs(promise));
} else {
  mWhenDefinedPromiseMap.Put(nameAtom, promise);
}

@@ -828,5 @@
>      mCallbacks(aCallbacks),
>      mDocOrder(aDocOrder)
>  {
>  }
> -

Nit: Keep this blank line.

::: dom/tests/mochitest/webcomponents/test_custom_element_when_defined.html
@@ +26,5 @@
> +  // map with key name and whose value is a new promise.
> +  const promiseBeforeDefined = customElements.whenDefined('x-foo');
> +  const promiseNotDefined = customElements.whenDefined('x-bar');
> +
> +  Promise.all([promiseBeforeDefined, promiseNotDefined])

This is not right. 
The promiseBeforeDefined and promiseNotDefined will never be resolved.
That means Promise.all() will not be resolved, either. And the checks in then() will not be executed.

The reason why you can pass this test is because you didn't setup the promise chain correctly.
To setup it correct, this function should return a promise.

@@ +45,5 @@
> +  return new Promise((aResolve, aReject) => {
> +    const nameToBeDefined = 'x-foo';
> +    const promiseBeforeDefined = customElements.whenDefined(nameToBeDefined);
> +    promiseBeforeDefined.then(() => beforeDefined = true)
> +    .then(() => customElements.define(nameToBeDefined, class {}))

This is not correct. 'promiseBeforeDefined' will never be resolved, right?
And the following then() will never be execute.

@@ +59,5 @@
> +        const newPromise = customElements.whenDefined(nameToBeDefined);
> +        newPromise.then(() => isnot(promiseAfterDefined, newPromise,
> +          "Once name is defined, whenDefined() always returns a new promise."));
> +        ok(beforeDefined, "Promise before defined should be resolved.");
> +        ok(afterDefined, "Promise after defined should be resolved.");

afterDefined is redundant, you checks `afterDefined` when promise is resolved and it always true, actually.
Same with beforeDefined.

@@ +62,5 @@
> +        ok(beforeDefined, "Promise before defined should be resolved.");
> +        ok(afterDefined, "Promise after defined should be resolved.");
> +      });
> +    });
> +    aResolve();

The reason you can pass this test is because you call aResolve() immediately.
BTW, `new Promise` seems not necessary to me, you can just use the then Promise created by whenDefined().

@@ +73,5 @@
> +      function() {
> +        let nameNotDefined = true;
> +        const promiseNotDefined = customElements.whenDefined('x-bar');
> +        promiseNotDefined.then(() => nameNotDefined = false);
> +        aResolve(nameNotDefined);

This didn't test what we wanna test, actually.
You should put only checks in setTimeout(), the idea is we queue another event to make sure the promise is not resolved before next thing fires.

e.g.

function test() {
  let isResolved = false;
  customElements.whenDefined('x-bar').then(() => {
    isResolved = true;
  });
  return new Promise((aResolve, aReject) => {
    setTimeout(function() {
      ok(!isResolved, "Promise for not defined name should not be resolved.");
      aResolve();
    }, 0);
  }
}

@@ +120,5 @@
> +         "CustomElements with invalid name should throw SyntaxError.");
> +    });
> +
> +    expectSyntaxErrorPromise.then(() => {
> +      ok(false, "CustomElements with invalid name should throw SyntaxError.");

You can merge .catch into .then,

expectSyntaxErrorPromise.then(() => {
  // resolve ...
}, (ex) => {
  // reject ...
})

Like this test, if other test doesn't expect to get a rejected promise, please add ok(false, ...) in rejected handler, so that we could debug easier if something unexpected happens.

@@ +126,5 @@
> +
> +    promises.push(expectSyntaxErrorPromise);
> +  });
> +
> +  Promise.all(promises);

return Promise.all(promises);

@@ +131,5 @@
> +}
> +
> +const testWindow = iframe.contentDocument.defaultView;
> +const customElements = testWindow.customElements;
> +const expectSyntaxError = 'SyntaxError';

Move these consts upward, right after "SimpleTest.waitForExplicitFinish();".
Attachment #8794127 - Flags: feedback?(echen)
Addressed comment #6.
Attachment #8794127 - Attachment is obsolete: true
Attachment #8794756 - Flags: feedback?(echen)
The previous patch isn't correct. I upload a new one.
Attachment #8794756 - Attachment is obsolete: true
Attachment #8794756 - Flags: feedback?(echen)
Attachment #8794759 - Flags: feedback?(echen)
Comment on attachment 8794759 [details] [diff] [review]
Bug 1275839 - Implement CustomElementsRegistry whenDefined function. v4

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

::: dom/tests/mochitest/webcomponents/test_custom_element_when_defined.html
@@ +31,5 @@
> +  const promiseElement1 = customElements.whenDefined('x-element1');
> +  const promiseElement2 = customElements.whenDefined('x-element2');
> +
> +  if (promiseElement1 instanceof testWindow.Promise &&
> +      promiseElement2 instanceof testWindow.Promise) {

What if promiseElement1 or promiseElement2 is not a Promise? This test case still get passed, I think it is probably not what we expect.

@@ +36,5 @@
> +    // 5. Let promise be the value of the entry in map with key name.
> +    // 6. Return promise
> +    const sameAsPromiseElement1 = customElements.whenDefined('x-element1');
> +
> +    if (sameAsPromiseElement1 instanceof testWindow.Promise) {

Ditto.

@@ +52,5 @@
> +  const nameToBeDefined = 'x-foo';
> +
> +  const promiseBeforeDefined = customElements.whenDefined(nameToBeDefined);
> +  promiseBeforeDefined.then(() => {
> +    beforeDefined = true

This is still not right,
1. beforeDefined is an unused variable.
2. We should have a way to make sure `promiseBeforeDefined` will be resolved.
   Let's say, if promiseBeforeDefined doesn't be resolved, current tests will still get passed and it is not what we expect. Right?

@@ +62,5 @@
> +  // then return a new promise resolved with undefined and abort these
> +  // steps.
> +  const promiseAfterDefined = customElements.whenDefined(nameToBeDefined);
> +  promiseAfterDefined.then(() => {
> +    afterDefined = true;

Ditto, afterDefined is an unused variable.

@@ +68,5 @@
> +          "When name is defined, we should have a new promise.");
> +
> +    const newPromise = customElements.whenDefined(nameToBeDefined);
> +    newPromise.then(() => {
> +      isnot(promiseAfterDefined, newPromise,

If you just wanna test the equality of two promises, you could just do the checks without waiting promise resolved.

@@ +71,5 @@
> +    newPromise.then(() => {
> +      isnot(promiseAfterDefined, newPromise,
> +            "Once name is defined, whenDefined() always returns a new promise.")
> +    });
> +  });

The promise chain of this function is still incorrect.
It doesn't make sure all tests of this function is done before executing next test set.

You could rewrite to, something like,

function test() {
  let name = "x-foo";
  let promise = customElements.whenDefined(name);

  customElements.define(name, class {});

  return promise.then(() => {
    // after define ....
    let promise2 = customElements.whenDefined(name);
    isnot(promise, promise2, ...);

    let promise3 = customElement.whenDefined(name)
    isnot(promise2, promise3, ...);

    return Promise.all([promise2, promise3]);
  });
}

@@ +128,5 @@
> +      is(ex.name, expectSyntaxError,
> +         "CustomElements with invalid name should throw SyntaxError.");
> +    });
> +
> +    promises.push(expectSyntaxErrorPromise);

You should push the promise created by expectSyntaxErrorPromise.then();

promises.push(expectSyntaxErrorPromise.then(() => {
 ...
});
Attachment #8794759 - Flags: feedback?(echen)
Addressed comment #9.
Attachment #8794759 - Attachment is obsolete: true
Attachment #8795149 - Flags: feedback?(echen)
Comment on attachment 8795149 [details] [diff] [review]
Bug 1275839 - Implement CustomElementsRegistry whenDefined function. v5

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

::: dom/tests/mochitest/webcomponents/test_custom_element_when_defined.html
@@ +32,5 @@
> +  let promiseElement2 = customElements.whenDefined('x-element2');
> +
> +  if (!promiseElement1 instanceof testWindow.Promise ||
> +      !promiseElement2 instanceof testWindow.Promise) {
> +    ok(false, "promiseElement1 and promiseElement2 should return promises.");

Don't need to wrap with if block and revise the message to "whenDefined should return a promise".

ok(promiseElement1 instanceof testWindow.Promise &&
   promiseElement2 instanceof testWindow.Promise,
   "whenDefined should return a promise.");

@@ +40,5 @@
> +  // 6. Return promise
> +  let sameAsPromiseElement1 = customElements.whenDefined('x-element1');
> +
> +  if (!sameAsPromiseElement1 instanceof testWindow.Promise) {
> +    ok(false, "sameAsPromiseElement1 should return promise.");

Ditto.

@@ +66,5 @@
> +
> +    let newPromise = customElements.whenDefined(name);
> +    isnot(afterDefinedPromise, newPromise,
> +          "Once name is defined, whenDefined() always returns a new promise.");
> +    return Promise.all([beforeDefinedPromise, afterDefinedPromise]);

s/beforeDefinedPromise/newPromise/ ?

@@ +118,5 @@
> +  let promises = [];
> +  invalidCustomElementNames.forEach(name => {
> +    const expectSyntaxErrorPromise = customElements.whenDefined(name);
> +
> +    promises.push(expectSyntaxErrorPromise.then((aResolve, aReject) => {

aResolve and aReject are unused arguments and the promise created by whenDefined() will never be resolved with two arguments.

@@ +123,5 @@
> +      ok(false, "CustomElements with invalid name should throw SyntaxError.");
> +    }, (ex) => {
> +      is(ex.name, expectSyntaxError,
> +         "CustomElements with invalid name should throw SyntaxError.");
> +      aResolve();

This will throw a ReferenceError and cause the promise being rejected.
And the Promise.all() will be rejected, too.

@@ +136,5 @@
> + .then(() => testCustomElementsPromiseEqually())
> + .then(() => testCustomElementsNameDefined())
> + .then(() => testCustomElementsNameNotDefined())
> + .then(() => testCustomElementsInvalidName())
> + .catch(() => SimpleTest.finish());

This is not right. Why we expect the promise being rejected?
The rejection is caused by the ReferenceError in testCustomElementsInvalidName() which is not what we expect.
Attachment #8795149 - Flags: feedback?(echen)
Addressed comment #11. In this patch, I also add mWhenDefinedPromiseMap into CYCLE_COLLECTION_UNLINK.
Attachment #8795149 - Attachment is obsolete: true
Attachment #8796433 - Flags: feedback?(echen)
In this patch, I put mWhenDefinedPromiseMap into CYCLE_COLLECTION_TRAVERSE.
Attachment #8796433 - Attachment is obsolete: true
Attachment #8796433 - Flags: feedback?(echen)
Attachment #8796483 - Flags: feedback?(echen)
Comment on attachment 8796483 [details] [diff] [review]
Bug 1275839 - Implement CustomElementsRegistry whenDefined function. v7

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

f=me with comment addressed. Thank you.

::: dom/base/CustomElementsRegistry.cpp
@@ +118,5 @@
>  NS_IMPL_CYCLE_COLLECTION_CLASS(CustomElementsRegistry)
>  
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(CustomElementsRegistry)
>    tmp->mCustomDefinitions.Clear();
> +  tmp->mWhenDefinedPromiseMap.Clear();

nsRefPtrHashtable implements ImplCycleCollectionUnlink(), so you can use NS_IMPL_CYCLE_COLLECTION_UNLINK macro.

e.g.
NS_IMPL_CYCLE_COLLECTION_UNLINK(mWhenDefinedPromiseMap)

::: dom/tests/mochitest/webcomponents/test_custom_element_when_defined.html
@@ +33,5 @@
> +
> +  ok(promiseElement1 instanceof testWindow.Promise &&
> +     promiseElement2 instanceof testWindow.Promise,
> +     "promiseElement1 and promiseElement2 should return promises.");
> +  // 5. Let promise be the value of the entry in map with key name.

Nit: Add empty line before this.
Attachment #8796483 - Flags: feedback?(echen) → feedback+
Addressed comment #14. Thanks for the feedback, Edgar.
Hi William, 
May I have your review? 

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f67d3665b9d712a032315a462d8f1e8a43d2d508&filter-tier=1
Attachment #8796483 - Attachment is obsolete: true
Attachment #8797106 - Flags: review?(wchen)
Comment on attachment 8797106 [details] [diff] [review]
Bug 1275839 - Implement CustomElementsRegistry whenDefined function. v8

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

::: dom/base/CustomElementsRegistry.cpp
@@ +835,3 @@
>  {
> +  RefPtr<Promise> promise = CreatePromise(aRv);
> +  if (!promise) {

Can you change CreatePromise to throw an error in aRv and then check for aRv.Failed() here instead of !promise. That way whenDefined will throw if we fail to create a promise instead of returning null unexpectedly.
Attachment #8797106 - Flags: review?(wchen) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b8eccc47b2
Implement CustomElementsRegistry whenDefined function. r=wchen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b0b8eccc47b2
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Component: DOM → DOM: Core & HTML
Regressions: 1693068
You need to log in before you can comment on or make changes to this bug.