Closed Bug 1418927 Opened 7 years ago Closed 6 years ago

requestHeaders and responseHeaders should be loaded lazily

Categories

(DevTools :: Netmonitor, defect, P2)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: rickychien, Assigned: rickychien)

References

(Depends on 2 open bugs)

Details

Attachments

(4 files)

Bug 1418927 - requestHeaders and responseHeaders should be loaded lazily
59 bytes, text/x-review-board-request
Honza
: review+
ochameau
: review+
Details
netmonitor fully reloaded
1.02 MB, image/png
Details
netmonitor right after requestsFinished
23.59 KB, image/png
Details
Bug 1418927 - Introduce fetchNetworkUpdatePacket helper in request-utils
59 bytes, text/x-review-board-request
ochameau
: review+
Details 
According to https://bugzilla.mozilla.org/show_bug.cgi?id=1404913#c9, 

Lazy load requestHeaders and responseHeaders looks promising for improving netmonitor perf.
Assignee: nobody → rchien
Status: NEW → ASSIGNED
It looks like the patch depends on patch from bug 1418928
But, if I apply both patches, build and open the Toolbox
the Network panel is not there...

Ricky, do you see the problem?

Honza
Flags: needinfo?(rchien)
Right, it requires the patch from bug 1418928. Did you try using the hg pull command from review-board?

hg pull -r b55cdb7a7fa8e89c33991fdd677a3417f30ff71f https://reviewboard-hg.mozilla.org/gecko

I believe it should work without doing extra setup
Flags: needinfo?(rchien)
damp result about this patch:

damp complicated.netmonitor.requestsFinished.DAMP  -36.06%
damp simple.netmonitor.requestsFinished.DAMP       -58.22%

however, close perf increased

damp complicated.netmonitor.close.DAMP             109.97%
damp simple.netmonitor.close.DAMP                  56.30%

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=30a6a6afc7a3abaa172df726a8b37c568446f03e&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1&selectedTimeRange=172800
Comment on attachment 8934125 [details]
Bug 1418927 - requestHeaders and responseHeaders should be loaded lazily

https://reviewboard.mozilla.org/r/205072/#review211500

This is first round, I'll yet go over the changes in tests.

Thanks for the work Ricky!

Honza

::: devtools/client/netmonitor/src/components/HeadersPanel.js:75
(Diff revision 7)
>      this.renderValue = this.renderValue.bind(this);
> -    this.maybeFetchPostData = this.maybeFetchPostData.bind(this);
> +    this.maybeFetchData = this.maybeFetchData.bind(this);
>    }
>  
>    componentDidMount() {
> -    this.maybeFetchPostData(this.props);
> +    this.maybeFetchData(this.props);

Nice, thanks for renaming this method.

::: devtools/client/netmonitor/src/components/RequestListItem.js:131
(Diff revision 7)
> +   * The panel will first be empty and then display the content.
> +   */
> +  maybeFetchData(props) {
> +    let { connector, item, requestFilterTypes } = props;
> +
> +    if (!requestFilterTypes.get("xhr") && !requestFilterTypes.get("ws")) {

Why data are not fetched in case of xhr & ws?
Can you make an explanatory comment?

::: devtools/client/netmonitor/src/components/StatisticsPanel.js:73
(Diff revision 7)
> +    this.maybeFetchData(this.props);
> +  }
> +
> +  componentWillReceiveProps(nextProps) {
> +    this.maybeFetchData(nextProps);
> +  }

Just a note. This pattern 'maybe fetching data' in componentDidMount and componentWillReceiveProps is used in every side panel. I am wondering if we could somehow avoid repeating the code.
Obviously, we could use component inheritance, but it isn't recommended by React community (not sure if there is a better way).

::: devtools/client/netmonitor/src/components/StatisticsPanel.js:255
(Diff revision 7)
>    }
>  
>    /**
> +   * When switching to another request, lazily fetch data from the backend.
> +   * The panel will first be empty and then display the content.
> +   */

When loading cnn.com and starting perf analysis - the Statistics panel is rendered, but never ready showing the actual results.

This looks like a problem introduced earlier. Could it be that the `ready` flag in `componentDidUpdate` is never true. 

It's fine to file a follow up (and give it a priority).

::: devtools/client/netmonitor/src/components/Toolbar.js:138
(Diff revision 7)
>    }
>  
>    onSearchBoxFocus() {
>      let { connector, filteredRequests } = this.props;
>  
>      // Fetch responseCookies for building autocomplete list

The comment should be updated (the code is now requesting responseCookies and responseHeaders).

::: devtools/client/netmonitor/src/request-list-context-menu.js:88
(Diff revision 7)
>  
>      copySubmenu.push({
>        id: "request-list-context-copy-as-curl",
>        label: L10N.getStr("netmonitor.context.copyAsCurl"),
>        accesskey: L10N.getStr("netmonitor.context.copyAsCurl.accesskey"),
> -      visible: !!selectedRequest,
> +      visible: !!(selectedRequest && (requestHeadersAvailable || requestHeaders)),

Why we use `requestHeadersAvailable` to enable the menu item. This flag says that request-haders has been requested, but it doesn't have to be received yet. Isn't checking `requestHeaders` field enough?

(the same question for the other items)
Attachment #8934125 - Flags: review?(odvarko) → review-
Comment on attachment 8934125 [details]
Bug 1418927 - requestHeaders and responseHeaders should be loaded lazily

https://reviewboard.mozilla.org/r/205072/#review211500

> Why data are not fetched in case of xhr & ws?
> Can you make an explanatory comment?

Filtering XHR & WS require to lazily fetch requestHeaders & responseHeaders. This commend has added.

> When loading cnn.com and starting perf analysis - the Statistics panel is rendered, but never ready showing the actual results.
> 
> This looks like a problem introduced earlier. Could it be that the `ready` flag in `componentDidUpdate` is never true. 
> 
> It's fine to file a follow up (and give it a priority).

Yep, this is an earlier problem since we don't have a shouldComponentUpdate() for sanity check. I'll file a bug for this.

> Why we use `requestHeadersAvailable` to enable the menu item. This flag says that request-haders has been requested, but it doesn't have to be received yet. Isn't checking `requestHeaders` field enough?
> 
> (the same question for the other items)

All menu item will be visible even if data hasn't arrived, so we need to check all *Available property and then fetch data lazily once user triggers the action.

I've added comments for all menu items for explaining this.
Comment on attachment 8934125 [details]
Bug 1418927 - requestHeaders and responseHeaders should be loaded lazily

https://reviewboard.mozilla.org/r/205072/#review211500

> Just a note. This pattern 'maybe fetching data' in componentDidMount and componentWillReceiveProps is used in every side panel. I am wondering if we could somehow avoid repeating the code.
> Obviously, we could use component inheritance, but it isn't recommended by React community (not sure if there is a better way).

Nice catch! I've extracted this repeated code to request-utils.js for reducing duplicated code.
All issues are addressed, please review it again!
Comment on attachment 8934125 [details]
Bug 1418927 - requestHeaders and responseHeaders should be loaded lazily

https://reviewboard.mozilla.org/r/205072/#review211800

Thanks for the update!

As we discussed at the meeting the tests are now extensively using
this construct:

`yield waitUntil(() => document.querySelectorAll(<selector>)` 

This construct might be unnecessary and I filled a follow-up
for creating an example tests showing how to properly
wait for typical network panel (async) phases
1) async wait for fetching data
2) async wait for updatint the UI

Bug 1423868


Honza

::: devtools/client/netmonitor/src/components/HeadersPanel.js:20
(Diff revisions 7 - 9)
>  const {
>    getHeadersURL,
>    getHTTPStatusCodeURL,
>  } = require("../utils/mdn-utils");
> -const { writeHeaderText } = require("../utils/request-utils");
> +const {
> +  fetchNetwortUpdatePacket,

typo: fetchNetwortUpdatePacket -> fetchNetworkUpdatePacket

::: devtools/client/netmonitor/src/utils/request-utils.js:73
(Diff revisions 7 - 9)
>    }
>    return headers;
>  }
>  
>  /**
> + * Fetch network event update parckets from actor server

typo: parckets -> packets

::: devtools/client/netmonitor/src/utils/request-utils.js:76
(Diff revisions 7 - 9)
>  
>  /**
> + * Fetch network event update parckets from actor server
> + * Expect to fetch a couple of network update packets from a given request.
> + *
> + * @param {funciton} requestData - requestData function for lazily fetch data

typo: funciton -> function
Attachment #8934125 - Flags: review?(odvarko) → review-
Thanks for the quick review! All typos have been addressed.

After adopting lazily fetch approach, many tests will need to insert wait code as comment 19 described. There are many articles talking about how to deal with react async rendering in E2E or integration test. However, a waiting approach would be necessary since we're doing lots of async updates in React's life cycle methods (componentDidMount, componentWillReceiveProps...etc). 

In order to avoid being off-topic and introducing another refactoring for mochitests within this bug, I'd suggest that we focus on handling & reviewing lazy fetching feature here, but move all mochitests fixes to bug 1423868. bug 1423868 should not be a blocker of this and so we can put more resources and time in investigating mochitest enhancement and figuring out a better solution to wait for react component update.
Comment on attachment 8934125 [details]
Bug 1418927 - requestHeaders and responseHeaders should be loaded lazily

https://reviewboard.mozilla.org/r/205072/#review211852

This can't land as-is, it requires some tweaks in DAMP.
The results your posted are irrelevant as it broke DAMP waiting code.
See: https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#782-816
This is based on PAYLOAD_READY, and so, DAMP is no longer going to wait for header requests and stop much earlier.
I'm expecting to see a win on DAMP, but something close to cookies/responseContent patches.

Here is some tips to help your debug that.

You can run DAMP like this:
  ./mach talos-test --activeTests damp --cycles 1 --tppagecycles 1 --subtest complicated.netmonitor
And apply this patch to record netmonitor panel state when DAMP considers it fully reloaded:
```
diff --git a/testing/talos/talos/tests/devtools/addon/content/damp.js b/testing/talos/talos/tests/devtools/addon/content/damp.js
index a4c6bafd7f73..a83b752f5a82 100644
--- a/testing/talos/talos/tests/devtools/addon/content/damp.js
+++ b/testing/talos/talos/tests/devtools/addon/content/damp.js
@@ -649,6 +649,8 @@ async _consoleOpenWithCachedMessagesTest() {
         const requestsDone = this.waitForNetworkRequests(label + ".netmonitor", toolbox);
         await this.reloadPageAndLog(label + ".netmonitor");
         await requestsDone;
+        await new Promise(d=>{});
+
         await this.closeToolboxAndLog(label + ".netmonitor");
         await this.testTeardown();
       },
@@ -804,6 +806,13 @@ async _consoleOpenWithCachedMessagesTest() {
           return;
         }
 
+        let canvas = window.document.createElementNS("http://www.w3.org/1999/xhtml", "html:canvas");
+        let context = canvas.getContext("2d");
+        canvas.width = window.innerWidth;
+        canvas.height = window.innerHeight;
+        context.drawWindow(window, 0, 0, canvas.width, canvas.height, "white");
+        dump(" >> "+canvas.toDataURL() + "\n");
+
         // All requests are done - unsubscribe from events and resolve!
         window.off(EVENTS.NETWORK_EVENT, onRequest);
         window.off(EVENTS.PAYLOAD_READY, onPayloadReady);
```
Copy and paste the data uri that is in stdout in a browser to see how was the netmonitor when waitForNetworkRequests resolved. The data uri screenshot should look like the final state of netmonitor. i.e. "B4K_BWwP7P5.png" file should be the last item of the request list and everything should be rendered.

I imagine you need to add the same logic you added into netmonitor's head.js.

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:385
(Diff revision 10)
> -          responseContentAvailable: true,
>          });
>          break;
>        case "eventTimings":
>          this.pushRequestToQueue(actor, { totalTime: networkInfo.totalTime });
> -        this.requestPayloadData(actor, updateType);
> +        this.onPayloadDataReceived(actor);

This line is unecessary as you also call it from line 393.

Also, can't you call `await this._requestData(actor, "eventTimings");` from here instead of `onPayloadDataReceived`. It looks unrelated to have it fetch from there.

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:400
(Diff revision 10)
>      emit(EVENTS.NETWORK_EVENT_UPDATED, actor);
>    }
>  
>    /**
> -   * Wrapper method for requesting HTTP details data for the payload.
> -   *
> +   * Notify actions when all the sync request from onNetworkEventUpdate are done,
> +   * or, everytime requestData is called for fetching data lazily.

This comment seems outdated and slightly wrong.
* sync request: there is no synchronous request. There is payload related requests, or "forced" requests. But really, nothing is synchronous here.
We wait for two things:
  * received all the networkEventUpdate events (so that all xxxAvailable fields are true
  * fetched eventTimings
* "or, everytime requestData is called for fetching data lazily." is it outdated? it is not true with the current patch, it is only called from onNetworkEventUpdate.
Attachment #8934125 - Flags: review-
Comment on attachment 8934125 [details]
Bug 1418927 - requestHeaders and responseHeaders should be loaded lazily

https://reviewboard.mozilla.org/r/205072/#review211852

I'm switching over between the m-c changeset (without my patch) and my patch to see what happens in damp test. Unfortunately your tip doens't work and the B4K_BWwP7P5.png never be the last item in request list after retrying many times :/ I think it's interesting that we cannot ensure the damp test is able to wait until all reqeusts rendered properly.

I believe the main reason isn't about not waiting for requestHeaders since we can't wait for requestHeaders & responseHeaders (same as RECEIVED_REQUEST_HEADERS & RECEIVED_RESPONSE_HEADERS events) in waitForAllRequestsFinished() anymore once we start fetching them lazily. RECEIVED_REQUEST_HEADERS & RECEIVED_RESPONSE_HEADERS events will be emitted if someone triggers a requestData(id, "requestHeaders") call in some conditions such as filtering requsts.

Hovever, there is another issue I'd like to clarify. I made a mistake in my patch to accidentlly remove `await` at this.actions.updateRequest [1]. This is the root cause why there are many intermittent failures arose and need to add some extra waitUntil calls in many tests. Now those redundant waitUntil() are removed and so the tests should be robust again.

DAMP result will be affacted if we remove `await` at [1], because DAMP test is waiting for PAYLOAD_READY event at [2]. This PAYLOAD_READY will be emitted immediately once we don't wait for this.actions.updateRequest complete, leading to incorrect damp result.

After reverting the mistake of `await this.actions.updateRequest()` and unnecessary waitUntil calls in tests, the rest of the test changes are necessary to use waitUntil / waitForDOM approaches for waiting for lazy fetch data.

[1] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-data-provider.js#479
[2] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-data-provider.js#484
Patch has updated to reflect the fix in comment 25 & comment 22. A follow-up damp test has pushed as well, I believe after fixing the missing `await` code, we can get more accurate damp test report.
(In reply to Ricky Chien [:rickychien] from comment #25)
> Comment on attachment 8934125 [details]
> Bug 1418927 - requestHeaders and responseHeaders should be loaded lazily
> 
> https://reviewboard.mozilla.org/r/205072/#review211852
> 
> I'm switching over between the m-c changeset (without my patch) and my patch
> to see what happens in damp test. Unfortunately your tip doens't work and
> the B4K_BWwP7P5.png never be the last item in request list after retrying
> many times :/ I think it's interesting that we cannot ensure the damp test
> is able to wait until all reqeusts rendered properly.

Sorry, I should not have mentioned one particular file. The order in which the requests appear is very random.
The important point is that the netmonitor screenshot printed in stdout should look like
the netmonitor after the test is fully finished.

Today, in m-c, the wait of reload isn't perfect, we miss the waterfall update,
but at least we can see all requests updated in the table.
Here is how netmonitor looks like after page reload. We see all requests and the waterfall is displayed. I take this screenschot when running DAMP via:
./mach talos-test --activeTests damp --cycles 1 --tppagecycles 1 --subtest complicated.netmonitor

With the patch from comment 22. I record the screenshot during the call to:
  await new Promise(d=>{});
in damp.js, which freeze DAMP test execution just after complicated.requestsFinished execution. So that I can see how the netmonitor settles.
And here is the screenshot done via comment 22's patch, printed on stdout.
It highlights the need to tweak DAMP to at least wait as much as we wait today on mozilla-central. We should at least see all the requests in the table, with all the columns updated.
We can see here that requestsFinished resolves almost immediately, there is only two requests rendered.

You most likely need to tweak this function:
https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#766-816
Which is used from requestsFinished test over here:
https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#646-654
Comment on attachment 8934125 [details]
Bug 1418927 - requestHeaders and responseHeaders should be loaded lazily

https://reviewboard.mozilla.org/r/205072/#review212810

::: devtools/client/netmonitor/src/components/Toolbar.js:23
(Diff revision 23)
>    isNetworkDetailsToggleButtonDisabled,
>  } = require("../selectors/index");
> -
>  const { autocompleteProvider } = require("../utils/filter-autocomplete-provider");
>  const { L10N } = require("../utils/l10n");
> +const { fetchNetworkUpdatePacket } = require("../utils/request-utils");

It looks like you may be able to land this helper independantly, before this patch.

::: devtools/client/netmonitor/src/connector/firefox-connector.js
(Diff revision 23)
>          this.timelineFront.off("doc-loading", this.onDocLoadingMarker);
>          await this.timelineFront.destroy();
>        }
> -
> -      this.tabTarget.off("will-navigate");
> -      this.tabTarget = null;

Why do you stop nullifying tabTarget?

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:246
(Diff revision 23)
>          }
>        }
>  
> -      // Lock down requestCookiesAvailable once we fetch data from back-end.
> +      // Lock down responseCookiesAvailable once we fetch data from back-end.
>        // Using this as flag to prevent fetching arrived data again.
> -      payload.requestCookiesAvailable = false;
> +      payload.responseCookiesAvailable = false;

These changes aren't related to this bug and could have been done independantly.

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:267
(Diff revision 23)
>     *
> -   * @param {string} id request id
> +   * @param {string} id request actor id
>     * @param {object} payload request data payload
>     */
>    pushRequestToQueue(id, payload) {
> -    let request = this.getRequestFromQueue(id);
> +    this.payloadQueue.set(id, { ...this.payloadQueue.get(id), ...payload });

Wouldn't this be enough:
  Object.assign(this.parloadQueue.get(id), payload);
?

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:364
(Diff revision 23)
> -          responseContentAvailable: true,
>          });
>          break;
>        case "eventTimings":
>          this.pushRequestToQueue(actor, { totalTime: networkInfo.totalTime });
> -        this.requestPayloadData(actor, updateType);
> +        this._requestData(actor, updateType);

You should wait for this request to finish before emitting PAYLOAD_READY:
  await this._requestData(actor, updateType);

::: devtools/client/netmonitor/test/head.js:360
(Diff revision 23)
> -      ["RECEIVED_RESPONSE_HEADERS", onGenericEvent],
> -      ["UPDATING_EVENT_TIMINGS", onGenericEvent],
> -      ["RECEIVED_EVENT_TIMINGS", onGenericEvent],
>        ["PAYLOAD_READY", onPayloadReady]
>      ];
>      let expectedGenericEvents = awaitedEventsToListeners

This function can be simplified as we now only listen for two different events.
No need of awaitedEventsToListeners and expectedGenericEvents lists.

::: devtools/client/netmonitor/test/head.js:528
(Diff revision 23)
>      let tooltip = target.querySelector(".requests-list-status").getAttribute("title");
> +
> +    yield waitUntil(() => {
> +      tooltip = target.querySelector(".requests-list-status").getAttribute("title");
> +      return tooltip;
> +    });

This waiting code looks unexpected right here, in middle of this function.
It would be clearer if the callsite was waiting correctly before calling verifyRequestItemTarget.

::: devtools/client/styleeditor/test/browser_styleeditor_fetch-from-cache.js
(Diff revision 23)
>  // A test to ensure Style Editor doesn't bybass cache when loading style sheet
>  // contents (bug 978688).
>  
> -Services.scriptloader.loadSubScript(
> -  "chrome://mochitests/content/browser/devtools/client/netmonitor/test/shared-head.js", this);
> -

It looks like you can remove shared-head.js,
there seem to be no more usage of it.
Attachment #8934125 - Flags: review-
Comment on attachment 8934125 [details]
Bug 1418927 - requestHeaders and responseHeaders should be loaded lazily

https://reviewboard.mozilla.org/r/205072/#review212810

> You should wait for this request to finish before emitting PAYLOAD_READY:
>   await this._requestData(actor, updateType);

I believe eventTimes can be fetched lazily as well. Let's discuss this in allhands.

> This function can be simplified as we now only listen for two different events.
> No need of awaitedEventsToListeners and expectedGenericEvents lists.

I'd like to drop this change here but move the change in bug 1423868 since this code is very similar in damp.js. They should be aligned and use same approach to wait for network requests.
Comment on attachment 8934125 [details]
Bug 1418927 - requestHeaders and responseHeaders should be loaded lazily

https://reviewboard.mozilla.org/r/205072/#review212810

> It looks like you may be able to land this helper independantly, before this patch.

Sounds great! I'll split it into another patch.

> Why do you stop nullifying tabTarget?

In order to fix `this.tabTarget is undefined` issue, `this.tabTarget.off("will-navigate");` needs to call before `await this.timelineFront...`. I've fixed.

> These changes aren't related to this bug and could have been done independantly.

I just swap the function definition order in order to make requestCookies & requestHeaders settle together.
(In reply to Ricky Chien [:rickychien] from comment #42)
> > You should wait for this request to finish before emitting PAYLOAD_READY:
> >   await this._requestData(actor, updateType);
> 
> I believe eventTimes can be fetched lazily as well. Let's discuss this in
> allhands.

I won't be part of Austin all hands.
eventTimings could be fetched lazily, but please focus on headers in this bug.
Making it lazy will make it harder to track its progress in tests,
whereas if you wait correctly for its completion before emitting PAYLOAD_READY,
you know you are going to wait correctly for it.

> > This function can be simplified as we now only listen for two different events.
> > No need of awaitedEventsToListeners and expectedGenericEvents lists.
> 
> I'd like to drop this change here but move the change in bug 1423868 since
> this code is very similar in damp.js. They should be aligned and use same
> approach to wait for network requests.

Just to be clear, you can't land this patch without fixing DAMP.
It's fine moving head.js's changes to a followup, but DAMP has to be fixed here and not in a followup.

(In reply to Ricky Chien [:rickychien] from comment #43)
> > Why do you stop nullifying tabTarget?
> 
> In order to fix `this.tabTarget is undefined` issue,
> `this.tabTarget.off("will-navigate");` needs to call before `await
> this.timelineFront...`. I've fixed.

Can you keep nullifying tabTarget after the call to timeLineFront?
Sorry I haven't pushed updated patch yesterday, but now open issues have been addressed.

(In reply to Alexandre Poirot [:ochameau] from comment #40)
> Created attachment 8936265 [details]
> netmonitor right after requestsFinished
> 
> And here is the screenshot done via comment 22's patch, printed on stdout.

Not sure what's the stdout you mean here. I suppose you were not saying the log in terminal, but the netmonitor's request list (screenshot) on Firefox's devtools. I've tried the same thing but I cannot reproduce your result in comment 40. 

After applying your test code from comment 22, I can see the same & correct result as the attachment of comment 39 when it freezes at `await new Promise(d=>{})` line. And now I've updated my patch and still see the right result. Not sure if I did something wrong, but maybe you can help me verify again.

> It highlights the need to tweak DAMP to at least wait as much as we wait
> today on mozilla-central. We should at least see all the requests in the
> table, with all the columns updated.
> We can see here that requestsFinished resolves almost immediately, there is
> only two requests rendered.
> 
> You most likely need to tweak this function:
> https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/
> devtools/addon/content/damp.js#766-816
> Which is used from requestsFinished test over here:
> https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/
> devtools/addon/content/damp.js#646-654
head.js has been simplified in latest patch update.
Comment on attachment 8936418 [details]
Bug 1418927 - Introduce fetchNetworkUpdatePacket helper in request-utils

https://reviewboard.mozilla.org/r/207126/#review213092

Thanks for splitting that out. Looks good;
Attachment #8936418 - Flags: review?(poirot.alex) → review+
Comment on attachment 8934125 [details]
Bug 1418927 - requestHeaders and responseHeaders should be loaded lazily

https://reviewboard.mozilla.org/r/205072/#review213082

Thanks a lot Ricky, I confirm that DAMP is fixed.
I have some comments, but this is mostly tests.

::: devtools/client/netmonitor/src/connector/firefox-connector.js:74
(Diff revision 27)
>      this.actions.batchReset();
>  
>      this.removeListeners();
>  
>      if (this.tabTarget) {
> +      this.tabTarget.off("will-navigate");

Can you write a comment saying why it is important to unregister will-navigate before timelineFront?

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:267
(Diff revision 27)
>     *
> -   * @param {string} id request id
> +   * @param {string} id request actor id
>     * @param {object} payload request data payload
>     */
>    pushRequestToQueue(id, payload) {
> -    let request = this.getRequestFromQueue(id);
> +    this.payloadQueue.set(id, { ...this.payloadQueue.get(id), ...payload });

You should really use Object.assign.
It will prevent an object copy everytime you call this method!
  let payloadFromQueue = this.payloadQueue.get(id);
  if (!payloadFromQueue) {
    payloadFromQueue = {};
    this.payloadQueue.set(id, payloadFromQueue);
  }
  Object.assign(payloadFromQueue, payload);

::: devtools/client/netmonitor/src/utils/filter-autocomplete-provider.js:111
(Diff revision 27)
>      values.push(...getAutocompleteValuesForFlag(flag, request));
>    }
>    values = [...new Set(values)];
>  
>    return values
> +    .filter(value => value)

Instead of filtering twice, it would be faster to keep doing only once by doing this:
   .filter(value => {
-      if (typedFlagValue) {
+      if (typedFlagValue && value) {

::: devtools/client/netmonitor/test/browser_net_filter-01.js:321
(Diff revision 27)
>        "There should still be a selected item after filtering.");
>      is(getSelectedIndex(store.getState()), 0,
>        "The first item should be still selected after filtering.");
>  
> -    const items = getSortedRequests(store.getState());
> -    const visibleItems = getDisplayedRequests(store.getState());
> +    let items = getSortedRequests(store.getState());
> +    let visibleItems = getDisplayedRequests(store.getState());

There is no need to compute items and visibleItems as you are going to recompute them anyway in waitUntil.
Here, this should be enough:
  let items, visibleItems;

Please review most of your waitUntil usage, you are doing this in a couple of tests.

::: devtools/client/netmonitor/test/browser_net_filter-01.js:327
(Diff revision 27)
> +
> +    // Filter results will be updated asynchronously, so we should wait until
> +    // displayed requests reach final state.
> +    yield waitUntil(() => {
> +      visibleItems = getDisplayedRequests(store.getState());
> +      return visibleItems.size === visibility.filter(e => e).length;

Could you compute visibility.filter(e=>e).length only once, before waitUntil?

::: devtools/client/netmonitor/test/browser_net_filter-flags.js:158
(Diff revision 27)
> +
>    function setFreetextFilter(value) {
> -    store.dispatch(Actions.setRequestFilterText(value));
> +    let filterBox = document.querySelector(".devtools-filterinput");
> +    filterBox.focus();
> +    filterBox.value = "";
> +    type(value);

Could you add a comment explaining why your are no longer using an action for that?

::: devtools/client/netmonitor/test/browser_net_headers_sorted.js:23
(Diff revision 27)
>  
> -  tab.linkedBrowser.reload();
> -
>    let wait = waitForNetworkEvents(monitor, 1);
> +  tab.linkedBrowser.reload();
>    yield wait;

Good catch!

::: devtools/client/netmonitor/test/browser_net_persistent_logs.js:26
(Diff revision 27)
>  
>    yield reloadAndWait();
>  
> +  // Using waitUntil in the test is necessary to ensure all requests are added correctly.
> +  // Because reloadAndWait call may catch early uncaught requests from initNetMonitor, so
> +  // the actual number of requests after reloadAndWait could be wrong since all reqeusts

s/reqeusts/requests/

::: devtools/client/netmonitor/test/browser_net_resend.js:59
(Diff revision 27)
>    wait = waitForNetworkEvents(monitor, 1);
>    store.dispatch(Actions.sendCustomRequest(connector));
>    yield wait;
>  
> -  let sentItem = getSelectedRequest(store.getState());
> +  let sentItem;
> +  // Testing sent request will requrie updated requestHeaders and requestPostData,

s/requrie/require/

::: devtools/client/netmonitor/test/browser_net_resend_headers.js:46
(Diff revision 27)
>  
>    let item = getSortedRequests(store.getState()).get(0);
> +
> +  if (item.requestHeadersAvailable && !item.requestHeaders) {
> +    requestData(item.id, "requestHeaders");
> +  }

It looks like this test will fail if requestHeadersAvailable is false here.
Shouldn't we assert this instead of making waitUntil timeout?
  ok(item.requestHeadersAvailable, "headers are available");

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_attach.js:76
(Diff revision 27)
> +  let { ui } = toolbox.getCurrentPanel().hud;
> +  let proxy = ui.jsterm.hud.proxy;
> +  return waitUntil(() => {
> +    return !proxy.networkDataProvider.lazyRequestData.size;
> +  });
> +}

I think it would be helpful for console contributors to explain this method in a comment.
Attachment #8934125 - Flags: review?(poirot.alex) → review+
Comment on attachment 8934125 [details]
Bug 1418927 - requestHeaders and responseHeaders should be loaded lazily

https://reviewboard.mozilla.org/r/205072/#review213082

> Could you compute visibility.filter(e=>e).length only once, before waitUntil?

No, everytime we read a new visibleItem from redux store via store.getState() in order to get the latest state, otherwise you will get previous state which is incorrect.
Comment on attachment 8934125 [details]
Bug 1418927 - requestHeaders and responseHeaders should be loaded lazily

https://reviewboard.mozilla.org/r/205072/#review213482

I don't have any further comments and Alex did detailed review already.
R+, assuming try is green.
Greate job here!

Honza
Attachment #8934125 - Flags: review?(odvarko) → review+
Try is green, let's land it. Thanks!
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a512e69ff1b
Introduce fetchNetworkUpdatePacket helper in request-utils r=ochameau
https://hg.mozilla.org/integration/autoland/rev/acc656fd0c97
requestHeaders and responseHeaders should be loaded lazily r=Honza,ochameau
https://hg.mozilla.org/mozilla-central/rev/9a512e69ff1b
https://hg.mozilla.org/mozilla-central/rev/acc656fd0c97
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Depends on: 1426809
Depends on: 1425733
Depends on: 1429803
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: