Closed
Bug 1482972
Opened 6 years ago
Closed 6 years ago
convert uses of "defer" to "new Promise" in shared/transport/
Categories
(DevTools :: General, defect, P4)
DevTools
General
Tracking
(firefox63 fixed)
RESOLVED
FIXED
Firefox 63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: jennowilde, Assigned: jennowilde)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
No description provided.
Assignee | ||
Comment 1•6 years ago
|
||
Proposed patch for cleanup of devtools/shared/transport
Converts defer() to new Promise
Comment 2•6 years ago
|
||
Thank you, Jennifer! We need to review the code before merging the patch. I'll look for a reviewer.
Priority: -- → P4
Comment 3•6 years ago
|
||
Comment on attachment 8999724 [details] [diff] [review]
hg changeset patch
Alex, can you have a look? :) Thanks
Attachment #8999724 -
Flags: review?(poirot.alex)
Updated•6 years ago
|
Assignee: nobody → jennowilde
Comment 4•6 years ago
|
||
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 !
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4f71b1fb4b3fe1b0078e1d36d109ed95e4975b6
Assignee | ||
Comment 5•6 years ago
|
||
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!
Comment 6•6 years ago
|
||
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* : https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4f71b1fb4b3fe1b0078e1d36d109ed95e4975b6&selectedJob=193883782
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: https://docs.firefox-dev.tools/tests/
Updated•6 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 7•6 years ago
|
||
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
<summary>
```
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 https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html .
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) {
> Packet.call(this, 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)
Assignee | ||
Comment 8•6 years ago
|
||
Assignee | ||
Comment 9•6 years ago
|
||
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 10•6 years ago
|
||
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+
Comment 11•6 years ago
|
||
Thank you, Jennifer! Super good for a first contribution.
Nicolas, will you land this when try is green? Merci :)
Flags: needinfo?(nchevobbe)
Comment 12•6 years ago
|
||
yes I will.
TRY is almost over, and so far so good :)
Flags: needinfo?(nchevobbe)
Comment 13•6 years ago
|
||
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b34fe8dbf3f
convert uses of "defer" to "new Promise" in shared/transport/; r=nchevobbe
Comment 14•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Updated•6 years ago
|
Attachment #8999724 -
Attachment is obsolete: true
Attachment #8999724 -
Flags: review?(nchevobbe)
Comment 15•6 years ago
|
||
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.
Description
•