Closed Bug 1288947 Opened 5 years ago Closed 5 years ago

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


(DevTools :: General, enhancement, P3)



(firefox52 fixed)

Firefox 52
Tracking Status
firefox52 --- fixed


(Reporter: neil_lohana, Assigned: dalimil, Mentored)


(Blocks 1 open bug)



(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:!msg/

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:
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
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
Convert uses of "defer" to "new Promise" - client/animationinspector directory. r=tromey
Keywords: checkin-needed
Closed: 5 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.