1288947 - convert uses of "defer" to "new Promise" - client/animationinspector directory

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Neil Lohana]

Attachment: Proposed patch for client/animationinspector directory.

Splitting up: Bug 1283869 - convert uses of "defer" to "new Promise" by directory to make reviews and bookkeeping more manageable.

Comment 1

[Neil Lohana]

I have filed a new bug for the directory I am tackling. I am unsure how to make this bug block the original. I would also appreciate some feedback regarding the proposed patch.

Comment 2

[Tom Tromey :tromey]

Comment on attachment 8774098 [details] [diff] [review]
Proposed patch for client/animationinspector directory.

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

Thank you for working on this.  I think a bit more work is needed to make this patch work -- a bit more, but not too much!

::: devtools/client/animationinspector/animation-controller.js
@@ +138,4 @@
>        yield this.initialized.promise;
>        return;
>      }
> +    this.initialized = new Promise();

I don't think this is sufficient.

A few lines above, this function references |this.initialized.promise| -- something provided by "defer" but not by "new Promise".

Likewise, later, the function refers to |this.initialized.resolve|.

Instead I think the transform has to pretty much follow this plan:

https://groups.google.com/forum/#!msg/mozilla.dev.developer-tools/IYw2U4TCNYU/JwfhnfomAwAJ


The other changes all have this same set of issues.

Comment 3

[Neil Lohana]

I'm sorry, I do not understand what this code snippet entails: https://groups.google.com/d/msg/mozilla.dev.developer-tools/IYw2U4TCNYU/JwfhnfomAwAJ

Comment 4

[Dalimil Hajek [:dalimil]]

Attachment: Bug1288947.patch - rev1

I changed the defer() syntax to Promise syntax but unfortunately the way that Task.async uses 'yield' in its generator makes it impossible to wrap the body in a Promise - we cannot use yield in the inner function. That's why I had to save the resolve() function in a reference.

Comment 5

[Tom Tromey :tromey]

Comment on attachment 8800610 [details] [diff] [review]
Bug1288947.patch - rev1

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

Thank you for doing this.
It looks pretty good; but I found one issue.  It should be simple to fix up though.

::: devtools/client/animationinspector/animation-controller.js
@@ +139,5 @@
> +
> +    let resolver;
> +    this.initialized = new Promise(resolve => {
> +      resolver = resolve;
> +    });

Thanks for the explanation of why this was needed.
It could perhaps be done some other way but I agree this
is reasonable.

@@ +181,2 @@
>  
> +    this.destroyed = new Promise(resolve => {

I think this changes the order in which things occur.
In particular, now "this.destroyed" won't be assigned until
the body is completely run -- but this means that the
function could now be re-entered without having set destroyed.

So I think you'll need to do the "let resolve;" thing here
as well.  Kind of gross but more obviously ok.

::: devtools/client/animationinspector/animation-panel.js
@@ +98,2 @@
>  
> +    this.destroyed = new Promise(resolve => {

Ditto here.

Comment 6

[Dalimil Hajek [:dalimil]]

Attachment: Bug1288947.patch - rev2

Ok, I've made the suggested changes...

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8800849 [details] [diff] [review]
Bug1288947.patch - rev2

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

Thank you.  This looks good.
The next step is to run it through "try".
Is this something you can do?  If not, that's fine, I will do it tomorrow -- just let me know.

Comment 8

[Dalimil Hajek [:dalimil]]

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6f018dfb6a4c32742fc092c8a91f0303f5dd728

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/03a8eec73245
Convert uses of "defer" to "new Promise" - client/animationinspector directory. r=tromey

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03a8eec73245