Closed Bug 1482972 Opened 6 years ago Closed 6 years ago

convert uses of "defer" to "new Promise" in shared/transport/


(DevTools :: General, defect, P4)



(firefox63 fixed)

Firefox 63
Tracking Status
firefox63 --- fixed


(Reporter: jennowilde, Assigned: jennowilde)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

      No description provided.
Blocks: 1283869
Attached patch hg changeset patch (obsolete) — Splinter Review
Proposed patch for cleanup of devtools/shared/transport
Converts defer() to new Promise
Thank you, Jennifer! We need to review the code before merging the patch. I'll look for a reviewer.
Priority: -- → P4
Comment on attachment 8999724 [details] [diff] [review]
hg changeset patch

Alex, can you have a look? :) Thanks
Attachment #8999724 - Flags: review?(poirot.alex)
Assignee: nobody → jennowilde
Hello Jennifer, thanks a lot for your patch.
I pushed it to TRY [1], our CI infrastructure which will run all the test we have to make sure everything is still working as expected.
This part of the code is very tricky and because of the differences of timing between defer and DOM Promise we might have some surprises.
Let's see how it goes !

Thank y'all so much for helping me figure out how to kick off the process for this patch!
This is my first time contributing to an open source project so it's extremely appreciated.

It looks like there are six surprises from the try server. What's the process for running the relevant parts of or the whole test suite locally on my machine so that I can attempt a fix and not have to put any load on the try server?

Also, if you have any recommendations for where to start that would be appreciated as well.
All that comes to mind right now is maybe using the Promise.jsm module instead of DOM Promises.

Thank you again, Soledad and Nicolas!
Haha, I like how you call them 'surprises' :)

The test failures are in the Chrome test suites--you can see that if you hover the surprises: "OS X 10.10 opt Mochitests with e10s test-macosx64/opt-mochitest-devtools-chrome-e10s-3 M-e10s(dt3)"

You can either run this suite locally with ./mach mochitest -f chrome --tag devtools (it takes a long time) or if you click on the surprise, in the lower side of the screen it will show you which test broke and whatever error message. For example here I clicked on the dt3* :

It says: "TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_content-type.js"

So that's the test that didn't finish properly. You could run it locally with 

./mach test devtools/client/netmonitor/test/browser_net_content-type.js

We have some docs on running tests here:
Ever confirmed: true
Comment on attachment 8999724 [details] [diff] [review]
hg changeset patch

Review of attachment 8999724 [details] [diff] [review]:

Hello Jennifer,

Sorry for the review delay.
This is great work, and TRY looks fine to me !
I only have a few comments and questions, but that's already very nice in such a hairy part of the code :)

Also I'm not sure the commit message is properly formatted. It should be:

Bug 1482972 - convert uses of "defer" to "new Promise" in shared/transport/; r=nchevobbe


Here we don't really need the <summary> part since the title of the commit is already self explanatory.

One last thing, we're in the process of deprecating submitting patch via the "attach file" mechanism to favor a better solution: Phabricator. You can read the doc to know how to setting it up .
If that's too much for now, feel free to attach your next patch as a file too, that's fine for me :)

Thanks again !

::: devtools/shared/transport/packets.js
@@ +229,5 @@
>   */
>  function BulkPacket(transport) {
>, transport);
>    this._done = false;
> +  this._resolve;

nit: Could we call this `_readyForWritingResolve` instead so it's really obvious what this "resolve" is ?

Or maybe, we could not bind `_resolve` to `this`, and add it as a property of _readyForWriting. like: 

let _resolve;
this._readyForWriting = new Promise((resolve) => {
  _resolve = resolve;
this._readyForWriting.resolve = _resolve;

this way, we can keep the "defer feel" and keep calling this._readyForWriting.resolve().
What do you think of this ?

::: devtools/shared/transport/stream-utils.js
@@ +73,5 @@
>      this.output.init(output, BUFFER_SIZE);
>    }
>    this._length = length;
>    this._amountLeft = length;
> +  this._resolve;

How would you feel to put `resolve` and `reject` on `this._deferred` instead of binding them to `this` ? (See previous comment)

::: devtools/shared/transport/tests/unit/test_queue.js
@@ -162,3 @@
>    });
> -
> -  return compareDeferred.promise.then(cleanup_files);

Don't we still need to call cleanup_files ?
Attachment #8999724 - Flags: review?(poirot.alex) → review?(nchevobbe)
Hey Nicolas,

I've uploaded a new patch to Phabricator, thank you for your review!

Please let me know if there's anything else that seems off in its formatting or construction.

I decided to go with adding the resolves to the properties of _readyForWriting and _deferred in devtools/shared/transport/packets.js and devtools/shared/transport/stream-utils.js respectively.

Also yes, we do still need to call cleanup_files, that was an error of omission on my part, thank you for catching it!
Comment on attachment 9003789 [details]
Bug 1482972 - convert uses of "defer" to "new Promise" in shared/transport/; r=nchevobbe

Nicolas Chevobbe [:nchevobbe] has approved the revision.
Attachment #9003789 - Flags: review+
Thank you, Jennifer! Super good for a first contribution.

Nicolas, will you land this when try is green? Merci :)
Flags: needinfo?(nchevobbe)
yes I will.
TRY is almost over, and so far so good :)
Flags: needinfo?(nchevobbe)
Pushed by
convert uses of "defer" to "new Promise" in shared/transport/; r=nchevobbe
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Attachment #8999724 - Attachment is obsolete: true
Attachment #8999724 - Flags: review?(nchevobbe)
Hello Jennifer, your patch is now on Firefox Nightly 🎉
Thanks a lot !
You need to log in before you can comment on or make changes to this bug.