1275839 - Implement CustomElementsRegistry whenDefined function

Status: RESOLVED FIXED

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

Description

[John Dai[:jdai]]

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

Comment 1

[Andrew Overholt [:overholt]]

(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.)

Comment 2

[John Dai[:jdai]]

Attachment: wip, v1

Comment 3

[John Dai[:jdai]]

Attachment: Bug 1275839 - Implement CustomElementsRegistry whenDefined function

Comment 4

[Edgar Chen [:edgar]]

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.

Comment 5

[John Dai[:jdai]]

Attachment: Bug 1275839 - Implement CustomElementsRegistry whenDefined function. v2

Addressed comment #4.

Comment 6

[Edgar Chen [:edgar]]

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();".

Comment 7

[John Dai[:jdai]]

Attachment: Bug 1275839 - Implement CustomElementsRegistry whenDefined function. v3

Addressed comment #6.

Comment 8

[John Dai[:jdai]]

Attachment: Bug 1275839 - Implement CustomElementsRegistry whenDefined function. v4

The previous patch isn't correct. I upload a new one.

Comment 9

[Edgar Chen [:edgar]]

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(() => {
 ...
});

Comment 10

[John Dai[:jdai]]

Attachment: Bug 1275839 - Implement CustomElementsRegistry whenDefined function. v5

Addressed comment #9.

Comment 11

[Edgar Chen [:edgar]]

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.

Comment 12

[John Dai[:jdai]]

Attachment: Bug 1275839 - Implement CustomElementsRegistry whenDefined function. v6

Addressed comment #11. In this patch, I also add mWhenDefinedPromiseMap into CYCLE_COLLECTION_UNLINK.

Comment 13

[John Dai[:jdai]]

Attachment: Bug 1275839 - Implement CustomElementsRegistry whenDefined function. v7

In this patch, I put mWhenDefinedPromiseMap into CYCLE_COLLECTION_TRAVERSE.

Comment 14

[Edgar Chen [:edgar]]

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.

Comment 15

[John Dai[:jdai]]

Attachment: Bug 1275839 - Implement CustomElementsRegistry whenDefined function. v8

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

Comment 16

[William Chen [: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.

Comment 17

[John Dai[:jdai]]

Attachment: Bug 1275839 - Implement CustomElementsRegistry whenDefined function. (Final) r=wchen

Addressed comment #16 and carrying r+.
Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7514695182b816f4e991c935a85b2d057a39de7&filter-tier=1

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b8eccc47b2
Implement CustomElementsRegistry whenDefined function. r=wchen

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0b8eccc47b2