Closed
Bug 1350648
Opened 7 years ago
Closed 7 years ago
convert uses of "defer" to "new Promise" - client/netmonitor directory
Categories
(DevTools :: Netmonitor, defect, P1)
DevTools
Netmonitor
Tracking
(firefox55 fixed)
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: powell.759, Assigned: powell.759)
References
(Blocks 1 open bug)
Details
(Whiteboard: [netmonitor-reserve])
Attachments
(1 file, 8 obsolete files)
8.58 KB,
patch
|
tromey
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Build ID: 20170317213149
Comment 2•7 years ago
|
||
Looks like good work here! Honza
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [netmonitor][triage]
Updated•7 years ago
|
Blocks: netmonitor-phaseII
Flags: qe-verify? → qe-verify-
Whiteboard: [netmonitor][triage] → [netmonitor]
Updated•7 years ago
|
Priority: P3 → P2
Attachment #8851445 -
Attachment is obsolete: true
Attachment #8853707 -
Attachment is obsolete: true
Updated•7 years ago
|
Assignee: nobody → powell.759
Comment 5•7 years ago
|
||
Comment on attachment 8853837 [details] [diff] [review] bug1350648.patch Review of attachment 8853837 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for doing this. I have a few nits but in general this is looking good. Can you submit the updated patch to try? If not, let me know, and I will do it. Also, you should append ", r?tromey" to the commit message. ::: devtools/client/netmonitor/test/browser_net_throttle.js @@ +15,5 @@ > > let { monitor } = yield initNetMonitor(SIMPLE_URL); > let { gStore, windowRequire, NetMonitorController } = monitor.panelWin; > + let { ACTIVITY_TYPE } = windowRequire("devtools/client/netmonitor/src/constants"); > + let { EVENTS } = windowRequire("devtools/client/netmonitor/src/constants"); These seem like unrelated changes. @@ +40,4 @@ > let client = NetMonitorController.webConsoleClient; > > info("sending throttle request"); > + yield new Promise((resolve, reject) => { I think you can drop ", reject" here. Keeping it seems like it would trigger an eslint warning. ::: devtools/client/netmonitor/test/head.js @@ +13,4 @@ > "chrome://mochitests/content/browser/devtools/client/framework/test/shared-head.js", > this); > > +const { EVENTS } = require("devtools/client/netmonitor/src/constants"); This seems like an unrelated change. @@ +180,4 @@ > } > > function waitForNetworkEvents(monitor, getRequests, postRequests = 0) { > + return Promise((resolve, reject) => { I think this doesn't need "reject". @@ +384,4 @@ > * Returns a promise that resolves upon firing of the event. > */ > function waitFor(subject, eventName) { > + return new Promise((resolve, reject) => { Likewise. @@ +499,4 @@ > function waitForContentMessage(name) { > let mm = gBrowser.selectedBrowser.messageManager; > > + return new Promise((resolve, reject) => { Likewise.
Updated•7 years ago
|
Status: NEW → ASSIGNED
Priority: P2 → P3
Whiteboard: [netmonitor] → [netmonitor-reserve]
This is my first time using Hg so I'm having a little trouble with changes I pulled mixing up with my own. I should have just made my changes on my own branch. I'll get on the IRC and see if I can get a little guidance on keeping my patches clean. I'll remove all those "reject"s and resubmit a patch. I'll try to push to try but If I have trouble I'll check back in. Copy on the commit message.
Attachment #8853837 -
Attachment is obsolete: true
Attachment #8856265 -
Flags: review?(ttromey)
Attachment #8856265 -
Attachment is obsolete: true
Attachment #8856265 -
Flags: review?(ttromey)
Attachment #8856266 -
Flags: review?(ttromey)
@trom(In reply to Tom Tromey :tromey from comment #5) > Comment on attachment 8853837 [details] [diff] [review] > bug1350648.patch > > Review of attachment 8853837 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thank you for doing this. > I have a few nits but in general this is looking good. > > Can you submit the updated patch to try? > If not, let me know, and I will do it. > > Also, you should append ", r?tromey" to the commit message. > > ::: devtools/client/netmonitor/test/browser_net_throttle.js > @@ +15,5 @@ > > > > let { monitor } = yield initNetMonitor(SIMPLE_URL); > > let { gStore, windowRequire, NetMonitorController } = monitor.panelWin; > > + let { ACTIVITY_TYPE } = windowRequire("devtools/client/netmonitor/src/constants"); > > + let { EVENTS } = windowRequire("devtools/client/netmonitor/src/constants"); > > These seem like unrelated changes. > > @@ +40,4 @@ > > let client = NetMonitorController.webConsoleClient; > > > > info("sending throttle request"); > > + yield new Promise((resolve, reject) => { > > I think you can drop ", reject" here. > Keeping it seems like it would trigger an eslint warning. > > ::: devtools/client/netmonitor/test/head.js > @@ +13,4 @@ > > "chrome://mochitests/content/browser/devtools/client/framework/test/shared-head.js", > > this); > > > > +const { EVENTS } = require("devtools/client/netmonitor/src/constants"); > > This seems like an unrelated change. > > @@ +180,4 @@ > > } > > > > function waitForNetworkEvents(monitor, getRequests, postRequests = 0) { > > + return Promise((resolve, reject) => { > > I think this doesn't need "reject". > > @@ +384,4 @@ > > * Returns a promise that resolves upon firing of the event. > > */ > > function waitFor(subject, eventName) { > > + return new Promise((resolve, reject) => { > > Likewise. > > @@ +499,4 @@ > > function waitForContentMessage(name) { > > let mm = gBrowser.selectedBrowser.messageManager; > > > > + return new Promise((resolve, reject) => { > > Likewise. I've went ahead and made these changes and attached an updated patch.I don't have the permission yet to push to try. So you could do that?
Comment 10•7 years ago
|
||
Comment on attachment 8856266 [details] [diff] [review] updated patch Review of attachment 8856266 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks! I'll push it through try soon.
Attachment #8856266 -
Flags: review?(ttromey) → review+
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a87f94865e3d93b8fd4abd42f091990a702f4036
Comment 12•7 years ago
|
||
According to the try log there is a missing "new" somewhere...
Flags: needinfo?(powell.759)
Assignee | ||
Comment 13•7 years ago
|
||
(In reply to Tom Tromey :tromey from comment #12) > According to the try log there is a missing "new" somewhere... I found the missing "new". Attaching an updated patch with this and the lint warnings corrected.
Flags: needinfo?(powell.759)
Assignee | ||
Comment 14•7 years ago
|
||
Attachment #8856266 -
Attachment is obsolete: true
Attachment #8857042 -
Flags: review?(ttromey)
Comment 15•7 years ago
|
||
Comment on attachment 8857042 [details] [diff] [review] update patch for bug 1350648 Review of attachment 8857042 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. I will push this through try momentarily.
Attachment #8857042 -
Flags: review?(ttromey) → review+
Comment 16•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=56a58945115a006e6e36a9e389acbdb0050ede30
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 19•7 years ago
|
||
Flags: needinfo?(powell.759)
Attachment #8857661 -
Flags: review?(ttromey)
Updated•7 years ago
|
Attachment #8857042 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8857661 -
Flags: review?(ttromey) → review+
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 20•7 years ago
|
||
Cancel that, I'm going to need to rewrite parts. It looks like changes were made within the functions I rewrote so I will need to sort those out
Flags: needinfo?(ttromey)
Assignee | ||
Comment 21•7 years ago
|
||
Attachment #8857661 -
Attachment is obsolete: true
Attachment #8857681 -
Flags: review?(ttromey)
Assignee | ||
Comment 22•7 years ago
|
||
Apologies for the confusion here, I wasn't able to rebase completely since there were some conflicting changes. I have done my best to remedy this but it should probably be ran through the try server again before check-in because I am not confident everything is correct here.
Updated•7 years ago
|
Flags: needinfo?(ttromey)
Keywords: checkin-needed
Comment 23•7 years ago
|
||
Comment on attachment 8857681 [details] [diff] [review] bug1350648.patch Review of attachment 8857681 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the rebase. It still looks good to me! I'm going to push it through try again as you suggested.
Attachment #8857681 -
Flags: review?(ttromey) → review+
Comment 24•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0633917d9d12410375657a590755fc6ebccee324
Assignee | ||
Comment 25•7 years ago
|
||
Welcome! Try caught an error so I'm attaching an updated patch now.
Assignee | ||
Comment 26•7 years ago
|
||
Attachment #8857681 -
Attachment is obsolete: true
Attachment #8857991 -
Flags: review?(ttromey)
Updated•7 years ago
|
Attachment #8857991 -
Flags: review?(ttromey) → review+
Comment 27•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c88957844661284bbf6bbdae0d370584287aaebb
Updated•7 years ago
|
Keywords: checkin-needed
Comment 28•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2c4240b6dff9 Replace defer with new Promise in devtools/client/netmonitor directory. r=tromey
Keywords: checkin-needed
Comment 29•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2c4240b6dff9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•7 years ago
|
Iteration: --- → 55.3 - Apr 17
Priority: P3 → P1
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•