Closed Bug 1328532 Opened 5 years ago Closed 5 years ago

Refactor updateRequest to fetch full text for responseContent and requestPostData

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Iteration:
53.4 - Jan 9
Tracking Status
firefox53 --- fixed

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file)

updateRequest method in requests-menu-view.js should fetch full text for responseContent and requestPostData, this task is going to refactor this function and fix broken mochitest.
Comment on attachment 8823544 [details]
Bug 1328532 - Refactor updateRequest to fetch full text

https://reviewboard.mozilla.org/r/102078/#review102480

::: devtools/client/netmonitor/netmonitor-controller.js:714
(Diff revision 4)
> +    // FIXME: this.webConsoleClient will be undefined in mochitest,
> +    // so we return string instantly to skip undefined error

Just curious: which test was failing with this error? When I remove this change, all netmonitor mochitests still succeed for me locally.

::: devtools/client/netmonitor/requests-menu-view.js:214
(Diff revision 4)
>        let request = getRequestById(this.store.getState(), action.id);
> -      let { text, encoding } = responseContent.content;
>        if (request) {
>          let { mimeType } = request;
> +        let { text, encoding } = responseContent.content;
> +        let response = yield gNetwork.getString(text);

Are you sure we want to retrieve the full responseContent string here, for all requests?

I thought we needed it only for image/* (for image thumbnails) and for text/html (for the Preview tab).

Is it going to be needed by the "Response" tab?

In any case, we should file a followup bug to fetch this information lazily, only after the tab that displays the response body is really selected.

::: devtools/client/netmonitor/requests-menu-view.js:221
(Diff revision 4)
> +        responseContent.content.text = response;
> +        payload.responseContent = Object.assign({}, responseContent);

You're modifying the responseContent object that comes from the "data" parameter of the updateRequest method. That should be immutable.

The purpose of the Object.assign call is probably to create a copy of the parameter, so that you can freely modify it? Then it doesn't have the desired effect here. Also note that Object.assign does a shallow copy. Objects deeper in the hiearchy are still shared.
Attachment #8823544 - Flags: review?(jsnajdr) → review+
(In reply to Jarda Snajdr [:jsnajdr] from comment #5)
> Comment on attachment 8823544 [details]
> Bug 1328532 - Refactor updateRequest to fetch full text
> 
> https://reviewboard.mozilla.org/r/102078/#review102480
> 
> ::: devtools/client/netmonitor/netmonitor-controller.js:714
> (Diff revision 4)
> > +    // FIXME: this.webConsoleClient will be undefined in mochitest,
> > +    // so we return string instantly to skip undefined error
> 
> Just curious: which test was failing with this error? When I remove this
> change, all netmonitor mochitests still succeed for me locally.

Ah, good question! You're right, it's weird my mochitest succeeded locally as well but it fails on Try. You can see my log at https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd727eae5fbb706353c633e9800e2014df086477

> 
> ::: devtools/client/netmonitor/requests-menu-view.js:214
> (Diff revision 4)
> >        let request = getRequestById(this.store.getState(), action.id);
> > -      let { text, encoding } = responseContent.content;
> >        if (request) {
> >          let { mimeType } = request;
> > +        let { text, encoding } = responseContent.content;
> > +        let response = yield gNetwork.getString(text);
> 
> Are you sure we want to retrieve the full responseContent string here, for
> all requests?
> 
> I thought we needed it only for image/* (for image thumbnails) and for
> text/html (for the Preview tab).
> 
> Is it going to be needed by the "Response" tab?

Yes, Response panel will try to render various type of response content as far as possible. It is not only for text/html, but there are also plenty of types (js, css) which are human readable.
I'll modify this part to fetch text/* but not all response types. However, we have to find a better way to display human readable data in source editor. (text/* doesn't cover all css and javascript mime type, because there are some exceptions like application/javascript or application/ecmascript for js and application/x-pointplus for css).

> 
> In any case, we should file a followup bug to fetch this information lazily,
> only after the tab that displays the response body is really selected.
> 
> ::: devtools/client/netmonitor/requests-menu-view.js:221
> (Diff revision 4)
> > +        responseContent.content.text = response;
> > +        payload.responseContent = Object.assign({}, responseContent);
> 
> You're modifying the responseContent object that comes from the "data"
> parameter of the updateRequest method. That should be immutable.
> 
> The purpose of the Object.assign call is probably to create a copy of the
> parameter, so that you can freely modify it? Then it doesn't have the
> desired effect here. Also note that Object.assign does a shallow copy.
> Objects deeper in the hiearchy are still shared.
Iteration: --- → 53.4 - Jan 9
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [netmonitor][triage] → [netmonitor]
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a762568b0299
Refactor updateRequest to fetch full text r=jsnajdr
I'm fixing this issue right now. My previous try runs didn't cover mochitest-clipboard, I will update and try again.
Flags: needinfo?(rchien)
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2e2db86ef78
Refactor updateRequest to fetch full text r=jsnajdr
https://hg.mozilla.org/mozilla-central/rev/d2e2db86ef78
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Flags: qe-verify? → qe-verify-
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.