1317651 - Implement Response Panel

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

This is a breakdown task for bug 1308450 in order to integrate HTTP inspector into the Net panel.

- Implement Response Panel

Comment 1

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1317651 - Implement Response Panel

Review commit: https://reviewboard.mozilla.org/r/101732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101732/

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Uploaded patch for first round review. Feature migration is complete within this patch and all test failures are addressed.

Try log:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f93352c9c76ca3876607440cb19995409d51cec1

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823180 [details]
Bug 1317651 - Implement Response Panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101732/diff/1-2/

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823180 [details]
Bug 1317651 - Implement Response Panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101732/diff/2-3/

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823180 [details]
Bug 1317651 - Implement Response Panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101732/diff/3-4/

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823180 [details]
Bug 1317651 - Implement Response Panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101732/diff/4-5/

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823180 [details]
Bug 1317651 - Implement Response Panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101732/diff/5-6/

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823180 [details]
Bug 1317651 - Implement Response Panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101732/diff/6-7/

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Response panel has rebased on top of Properties View (bug 1328828). All test failures has been addressed and ready for review.

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Summary for the patch:

* Add a new l10n string for displaying "Response payload" tree section. Because the source editor is part of TreeView component, it's easy to make UI look so much like the other panels and support collapsible feature.
* Remove the test cases for verifying source editor mode. Netmonitor.editor() API will be removed soon (we can remove it after rebasing ParamsPanel in this patch), so it's hard to get the editor's mode by querySelector in document. We are unable to access CodeMirror instance anymore, but I think that pass mimeType into setMode() is stable enough and there is unnecessary to test it in mochitest.
* We still need to fetch full text in updateReqeust() for `responseContent.content.text` since some responses is incorrect so we can't display editor properly.

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823180 [details]
Bug 1317651 - Implement Response Panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101732/diff/7-8/

Comment 12

[Ricky Chien [:rickychien] (inactive)]

Rebased the patch on top of ParamsPanel (bug 1317650).

Comment 13

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823180 [details]
Bug 1317651 - Implement Response Panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101732/diff/8-9/

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8823180 [details]
Bug 1317651 - Implement Response Panel

https://reviewboard.mozilla.org/r/101732/#review104192

The patch is good, just a few inline comments.

The only (but important) UI problem is that the Source Editor doesn't take the entire vertical space.
But, this is covered by a bug report already, correct?

Honza

::: devtools/client/netmonitor/requests-menu-view.js
(Diff revision 9)
>  
>          if (mimeType.includes("image/")) {
>            payload.responseContentDataUri = formDataURI(mimeType, encoding, response);
>          }
>  
> -        if (mimeType.includes("text/")) {

Why this condition has been removed? What about non-text responses?

::: devtools/client/netmonitor/shared/components/editor.js:25
(Diff revision 9)
>      text: PropTypes.string,
>    },
>  
>    getDefaultProps() {
>      return {
> +      mode: null,

Corresponding entry should be created in propTypes. Also comment explaining what the prop is for would be nice.
(we should have a comment for every prop, see e.g. TreeView.js file)

::: devtools/client/netmonitor/shared/components/editor.js:61
(Diff revision 9)
>  
>      if (prevProps.text !== text) {
>        this.deferEditor.then(() => {
>          // FIXME: Workaround for browser_net_accessibility test to
>          // make sure editor node exists while setting editor text.
>          // deferEditor workround should be removed in bug 1308442

Typo: workround -> workaround

(not introduced in this patch)

Comment 15

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823180 [details]
Bug 1317651 - Implement Response Panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101732/diff/9-10/

Comment 16

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823180 [details]
Bug 1317651 - Implement Response Panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101732/diff/10-11/

Comment 17

[Ricky Chien [:rickychien] (inactive)]

(In reply to Jan Honza Odvarko [:Honza] from comment #14)
> Comment on attachment 8823180 [details]
> Bug 1317651 - Implement Response Panel
> 
> https://reviewboard.mozilla.org/r/101732/#review104192
> 
> The patch is good, just a few inline comments.
> 
> The only (but important) UI problem is that the Source Editor doesn't take
> the entire vertical space.
> But, this is covered by a bug report already, correct?

You're right! It will be fixed in bug 1329068.

> 
> Honza
> 
> ::: devtools/client/netmonitor/requests-menu-view.js
> (Diff revision 9)
> >  
> >          if (mimeType.includes("image/")) {
> >            payload.responseContentDataUri = formDataURI(mimeType, encoding, response);
> >          }
> >  
> > -        if (mimeType.includes("text/")) {
> 
> Why this condition has been removed? What about non-text responses?

Good example is to take a look at http://github.com, source editor would not render js mime type if we only fetch full response for "text/" mime type. There are lots of exceptions which are not pre-fix with text/ like application/javascript, application/ecmascript, application/x-pointplus (css). You can check all mime types in 

https://www.sitepoint.com/web-foundations/mime-types-complete-list/

Therefore, I'd like to fetch full text for all responses to make sure every request can be rendered in source editor.

> 
> ::: devtools/client/netmonitor/shared/components/editor.js:25
> (Diff revision 9)
> >      text: PropTypes.string,
> >    },
> >  
> >    getDefaultProps() {
> >      return {
> > +      mode: null,
> 
> Corresponding entry should be created in propTypes. Also comment explaining
> what the prop is for would be nice.
> (we should have a comment for every prop, see e.g. TreeView.js file)
> 
> ::: devtools/client/netmonitor/shared/components/editor.js:61
> (Diff revision 9)
> >  
> >      if (prevProps.text !== text) {
> >        this.deferEditor.then(() => {
> >          // FIXME: Workaround for browser_net_accessibility test to
> >          // make sure editor node exists while setting editor text.
> >          // deferEditor workround should be removed in bug 1308442
> 
> Typo: workround -> workaround
> 
> (not introduced in this patch)

Comment 18

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823180 [details]
Bug 1317651 - Implement Response Panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101732/diff/11-12/

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8823180 [details]
Bug 1317651 - Implement Response Panel

https://reviewboard.mozilla.org/r/101732/#review104470

Thanks!

You can yet fix the typo mentioned in comment #14

Honza

Comment 20

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8823180 [details]
Bug 1317651 - Implement Response Panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101732/diff/12-13/

Comment 21

[Ricky Chien [:rickychien] (inactive)]

Thanks typo error has fixed.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ea07ae21799
Implement Response Panel r=Honza

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ea07ae21799

Comment 24

[[e-mail]]

There's a new warning with response panel when `ac_add_options --enable-debug-js-modules` is add in mozconfig:

"Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `TreeView`. See https://fb.me/react-warning-keys for more information.

Comment 25

[Ciprian Georgiu, Desktop QA]

This issue is verified fixed on latest Aurora 53.0a2 (2017-01-26) under Windows 10 x64, Mac OS X 10.11.6 and  Ubuntu 16.04 x64 LTS. The Response Panel in netmonitor is working properly.