Closed Bug 1288947 Opened 5 years ago Closed 4 years ago

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

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox52 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: neil_lohana, Assigned: dalimil, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Splitting up: Bug 1283869 - convert uses of "defer" to "new Promise" by directory to make reviews and bookkeeping more manageable.
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.
Flags: needinfo?(ttromey)
Blocks: 1283869
Severity: blocker → normal
Attachment #8774098 - Flags: review?(ttromey)
Flags: needinfo?(ttromey)
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.
Attachment #8774098 - Flags: review?(ttromey)
Severity: normal → enhancement
I'm sorry, I do not understand what this code snippet entails: https://groups.google.com/d/msg/mozilla.dev.developer-tools/IYw2U4TCNYU/JwfhnfomAwAJ
Attached patch Bug1288947.patch - rev1 (obsolete) — Splinter Review
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.
Assignee: nobody → dalimilhajek
Attachment #8774098 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8800610 - Flags: review?(ttromey)
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.
Attachment #8800610 - Flags: review?(ttromey)
Ok, I've made the suggested changes...
Attachment #8800849 - Flags: review?(ttromey)
Attachment #8800610 - Attachment is obsolete: true
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.
Attachment #8800849 - Flags: review?(ttromey) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/03a8eec73245
Convert uses of "defer" to "new Promise" - client/animationinspector directory. r=tromey
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/03a8eec73245
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.