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)

defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Iteration:
55.3 - Apr 17
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)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170317213149
Component: Untriaged → Developer Tools: Netmonitor
Blocks: 1283869
Looks like good work here!

Honza
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [netmonitor][triage]
Flags: qe-verify? → qe-verify-
Whiteboard: [netmonitor][triage] → [netmonitor]
Priority: P3 → P2
Attached patch patch for review (obsolete) — Splinter Review
Attachment #8851445 - Attachment is obsolete: true
Attached patch bug1350648.patch (obsolete) — Splinter Review
Attachment #8853707 - Attachment is obsolete: true
Assignee: nobody → powell.759
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.
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.
Attached patch updated patch (obsolete) — Splinter Review
Attachment #8853837 - Attachment is obsolete: true
Attachment #8856265 - Flags: review?(ttromey)
Attached patch updated patch (obsolete) — Splinter Review
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 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+
According to the try log there is a missing "new" somewhere...
Flags: needinfo?(powell.759)
(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)
Attached patch update patch for bug 1350648 (obsolete) — Splinter Review
Attachment #8856266 - Attachment is obsolete: true
Attachment #8857042 - Flags: review?(ttromey)
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+
Keywords: checkin-needed
Patch needs to be rebased.
Keywords: checkin-needed
Hi Maxwell - could you rebase this?
Flags: needinfo?(powell.759)
Attached patch rebased patch for bug 1350648 (obsolete) — Splinter Review
Flags: needinfo?(powell.759)
Attachment #8857661 - Flags: review?(ttromey)
Attachment #8857042 - Attachment is obsolete: true
Attachment #8857661 - Flags: review?(ttromey) → review+
Keywords: checkin-needed
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)
Attached patch bug1350648.patch (obsolete) — Splinter Review
Attachment #8857661 - Attachment is obsolete: true
Attachment #8857681 - Flags: review?(ttromey)
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.
Flags: needinfo?(ttromey)
Keywords: checkin-needed
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+
Welcome! Try caught an error so I'm attaching an updated patch now.
Attachment #8857681 - Attachment is obsolete: true
Attachment #8857991 - Flags: review?(ttromey)
Attachment #8857991 - Flags: review?(ttromey) → review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/2c4240b6dff9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.3 - Apr 17
Priority: P3 → P1
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: