940538 - Convert to Promise.jsm in the netmonitor

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Comment 1

[Brandon Benvie [:benvie]]

You can see some previous progress I made working on this before in the netmonitor files here: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=881050&attachment=830486. There was still test failures but that shows some stuff that needs to be fixed.

Comment 2

[Brandon Benvie [:benvie]]

Specifically, the changes to netmonitor-panel.js, netmonitor-view.js, and browser_net_content-type.js.

Comment 3

[Brandon Benvie [:benvie]]

Attachment: promise-netmonitor.patch

My initial stab at this.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 940538

All tests passing

Comment 5

[Brandon Benvie [:benvie]]

Comment on attachment 8336321 [details] [diff] [review]
940538

Victor should also review this.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 940538

Oops, had an extra `console` in there

Comment 7

[Brandon Benvie [:benvie]]

Comment on attachment 8336321 [details] [diff] [review]
940538

Review of attachment 8336321 [details] [diff] [review]:
-----------------------------------------------------------------

This is great! All I have is nits.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +1369,5 @@
>    /**
>     * Populates this view with the specified data.
>     *
>     * @param object aData
>     *        The data source (this should be the attachment of a request item).

Should add @return here now that it returns a Promise.

@@ +1406,2 @@
>     *        The data source (this should be the attachment of a request item).
>     */

@return Promise here.

@@ +1684,5 @@
>     *
>     * @param object aResponse
>     *        The message received from the server.
> +   * @return object
> +   *        A promise that resolves when request headers are set

Nit: period at the end of this.

@@ +1699,5 @@
>     *
>     * @param object aResponse
>     *        The message received from the server.
> +   * @return object
> +   *        A promise that resolves when response headers are set

Nit: period at the end of this.

@@ +1717,5 @@
>     *        The type of headers to populate (request or response).
>     * @param object aResponse
>     *        The message received from the server.
> +   * @return object
> +   *        A promise that resolves when headers are added

Nit: period at the end of this.

@@ +1726,5 @@
>      let text = L10N.getFormatStr("networkMenu.sizeKB", size);
>      let headersScope = this._headers.addScope(aName + " (" + text + ")");
>      headersScope.expanded = true;
>  
> +    return promise.all(aResponse.headers.map(header  => {

Nit: extra space before =>.

@@ +1731,3 @@
>        let headerVar = headersScope.addItem(header.name, {}, true);
> +      return gNetwork.getString(header.value)
> +        .then(aString => headerVar.setGrip(aString));

Nit: line up the periods after gNetwork.

@@ +1771,5 @@
>     * @param string aName
>     *        The type of cookies to populate (request or response).
>     * @param object aResponse
>     *        The message received from the server.
>     */

@return Promise here too.

@@ +1983,5 @@
>              }
>            }
>          });
>        }
> +      }).then(() => window.emit(EVENTS.RESPONSE_BODY_DISPLAYED));

Nit: indentation.

::: browser/devtools/netmonitor/test/browser_net_content-type.js
@@ +76,5 @@
>          document.querySelectorAll("#details-pane tab")[3]);
>  
> +      Task.spawn(function*() {
> +        yield waitForResponseBodyDisplayed();
> +        yield testResponseTab("xml")

Nit: missin semicolons for most of the lines in this function*.

::: browser/devtools/netmonitor/test/head.js
@@ +3,5 @@
>  "use strict";
>  
>  const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
>  
> +const console = Cu.import("resource://gre/modules/devtools/Console.jsm", {}).console;

Accidentally left this.

@@ +284,5 @@
>      }
>    }
>  }
> +
> +function waitFor (subject, eventName) {

Add javadocs comment documenting this function.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

> ::: browser/devtools/netmonitor/test/head.js
> @@ +3,5 @@
> >  "use strict";
> >  
> >  const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> >  
> > +const console = Cu.import("resource://gre/modules/devtools/Console.jsm", {}).console;
> 
> Accidentally left this.

I think we should leave the console whenever we add it. It makes it a lot easier for the next person debugging tests because they don't have to add the lengthy import.

Comment 9

[Brandon Benvie [:benvie]]

Let's hope this doesn't cause any collateral damage: https://tbpl.mozilla.org/?tree=Try&rev=89541e260fc8

Comment 10

[Brandon Benvie [:benvie]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #8)
> > ::: browser/devtools/netmonitor/test/head.js
> > @@ +3,5 @@
> > >  "use strict";
> > >  
> > >  const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> > >  
> > > +const console = Cu.import("resource://gre/modules/devtools/Console.jsm", {}).console;
> > 
> > Accidentally left this.
> 
> I think we should leave the console whenever we add it. It makes it a lot
> easier for the next person debugging tests because they don't have to add
> the lengthy import.

I'm on board with this.

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 940538

Fixed changes mentioned in Benvie's review

Comment 12

[Victor Porof [:vporof][:vp]]

Comment on attachment 8336356 [details] [diff] [review]
940538

Review of attachment 8336356 [details] [diff] [review]:
-----------------------------------------------------------------

Metal. r+ with comments addressed.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +1381,5 @@
> +      NetMonitorView.NetworkDetails;
> +
> +    return view.populate(aData).then(() =>
> +      $("#details-pane").selectedIndex = isCustom ? 0 : 1
> +    ).then(() => window.emit(EVENTS.SIDEBAR_POPULATED));

The second .then is not necessary here.

@@ +1415,5 @@
>      $("#custom-headers-value").value =
>         writeHeaderText(aData.requestHeaders.headers);
>  
> +    let view = this;
> +    let deferredPromise = null;

Nit: please rename this to postDataPromise.

@@ +1429,4 @@
>      }
>  
> +    return deferredPromise.then(() => view.updateCustomQuery(aData.url))
> +      .then(() => window.emit(EVENTS.CUSTOMREQUESTVIEW_POPULATED));

Nit: please align the .then()s, or use Task.spawn. (that's the formatting style I'd like to maintain in this tool, like in netmonitor-panel.js):

return deferredPromise
  .then(() => view.updateCustomQuery(aData.url))
  .then(() => window.emit(EVENTS.CUSTOMREQUESTVIEW_POPULATED));

@@ +1602,5 @@
>      this._json.empty();
>  
>      this._dataSrc = { src: aData, populated: [] };
>      this._onTabSelect();
> +    window.emit(EVENTS.NETWORKDETAILSVIEW_POPULATED);

Uber nit: add a newline before this.

@@ +1642,5 @@
> +          yield view._setTimingsInformation(src.eventTimings);
> +          break;
> +      }
> +      populated[tab] = true;
> +    }).then(() => window.emit(EVENTS.TAB_UPDATED));

You can emit inside the generator, no need for the extra .then().

@@ +1901,4 @@
>     */
>    _setResponseBody: function(aUrl, aResponse) {
>      if (!aResponse) {
> +      return promise.resolve();

Would it be better to have this (and friends in other functions) be a rejection with a meaningful message instead? Opinions welcome :)

::: browser/devtools/netmonitor/test/browser_net_post-data-02.js
@@ +18,5 @@
>  
>      waitForNetworkEvents(aMonitor, 0, 1).then(() => {
>        NetMonitorView.toggleDetailsPane({ visible: true }, 2)
>        RequestsMenu.selectedIndex = 0;
> +      

Nit: unnecessary whitespace here.

@@ +55,5 @@
> +          "baz", "The second query param name was incorrect.");
> +        is(postScope.querySelectorAll(".variables-view-variable .value")[1].getAttribute("value"),
> +          "\"123\"", "The second query param value was incorrect.");
> +      }).then(() => teardown(aMonitor))
> +      .then(finish);

You can leave teardown(aMonitor).then(finish) inside the callback, no need for the extra .then().

::: browser/devtools/netmonitor/test/browser_net_simple-request-details.js
@@ +40,5 @@
> +      }).then(() => {
> +        testTimingsTab();
> +        return teardown(aMonitor);
> +      })
> +      .then(finish);

Nit: all of this would look nicer in a Task.spawn.

@@ +171,5 @@
>  
>      function testResponseTab() {
>        EventUtils.sendMouseEvent({ type: "mousedown" },
>          document.querySelectorAll("#details-pane tab")[3]);
> +      

Nit: redundant whitespace.

Comment 13

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 940538

Fixed nits, added some Task.spawns

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/29dd3fb48a44

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/29dd3fb48a44