1152902 - Add a fast path to promises for being resolved with a promise

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Instead of taking the generic thenable path, I think we should be able to do better.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a way to flag a method in webidl as being public, so we'll expose it from bindings to other C++ code

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add a fast path for the case when a Promise is resolved with another Promise

Comment 3

[Boris Zbarsky [:bzbarsky]]

There's actually an API question here, though.  I can expose the function as I did, or we can add a boolean-returning thing like PromiseBinding::IsThenMethod() which takes a JSObject* and does the checks under the hood.  Then the caller won't need to be aware of the promiseWrapper complexities...  Peter, do you have a preference between those?

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8590401 [details] [diff] [review]
part 2.  Add a fast path for the case when a Promise is resolved with another Promise

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

::: dom/promise/Promise.cpp
@@ +1190,5 @@
> +          NS_SUCCEEDED(UNWRAP_OBJECT(Promise, valueObj, nextPromise))) {
> +        // If we were taking the codepath that involves ThenableResolverTask and
> +        // PromiseInit below, then eventually, in ThenableResolverTask::Run, we
> +        // would create some JSFunctions in the compartment of
> +        // this->GetWrapper() an pass them to the PromiseInit. So by the time

Nit: s/an/and

Comment 5

[Boris Zbarsky [:bzbarsky]]

Talked to Peter.  We're going to set things up so we have a mozilla::dom::PromiseBinding::IsThen() method and a [MethodIdentityTestable] extended attr.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a way to flag a method in webidl as being public, so we'll expose it from bindings to other C++ code

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 update to use IsThenMethod

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 with updated commit message too

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c5ed66652c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/462f2f668259

Comment 10

[Phil Ringnalda (:philor)]

Going to have to back this out once IT's tree-closing window ends, the tomfoolery in https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/test/unit/settings/layout_item_test.js doesn't seem to like it, https://treeherder.mozilla.org/logviewer.html#?job_id=9021345&repo=mozilla-inbound (which is Gu, gaia-unit, in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c503b0418516, thanks for the utterly useless logs TaskCluster).

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e5af7bbae31e

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK.  Dealing with trying to debug what gaia-unit-test is doing is just not worth it to me, given how painful it is to run locally.  If someone wants to take over this bug, please feel free to.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Actually, I think I know what's going on here.  The new code changed the ordering of promise callbacks in a way we apparently had no test for.  Fixing that makes Gu green on try.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch on top of part 2 to fix the ordering issue

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8594424 [details] [diff] [review]
Patch on top of part 2 to fix the ordering issue

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

Sorry, I should have caught the lack of async-ness in the first patch.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90f39251be10
https://hg.mozilla.org/integration/mozilla-inbound/rev/e372fae4d9b6

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/90f39251be10
https://hg.mozilla.org/mozilla-central/rev/e372fae4d9b6