1135965 - Response pane should contain both "preview' and "raw" tab views

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

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

Many other bugs very similar to being able to select raw JSON responses.

bug 955933 -  	Can't copy values from the JSON inspector 
bug 1110813 -  	It's not possible to view or copy the plain text response from DevTools -> Network -> Response if the response is JSON 

i think this will solve all those issues.

Comment 1

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

Attachment: responseenhancement.gif

Quick demo.

Comment 2

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

Attachment: 1135965-raw-json.patch

So this is easy, just a matter of if this is the right solution or not. Will add tests if it is, but passes currently.

Comment 3

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

also solves bug 859138

Comment 4

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

also bug 925340

Comment 5

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

Comment on attachment 8568370 [details] [diff] [review]
1135965-raw-json.patch

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

Subtabs are sooo weird. This could work better with some styling to at least hot have the same colors (blue under blue). But I like the approach.

::: browser/devtools/netmonitor/netmonitor.xul
@@ +394,5 @@
> +                    </tabs>
> +                    <tabpanels flex="1">
> +                      <tabpanel class="tabpanel-content">
> +                        <vbox flex="1">
> +                          <vbox flex="1" id="response-content-json" />

Why the vbox inside the vbox?

Comment 6

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

Styling, since the variables view injects more elements as a sibling to the response-content-json, so it contains it. Other uses of tabbox/variablesview do this, and I don't quite understand it, but it works.

As an alternative, we could also have this be another top level tab, rather than nested. "Preview", which would be currently our response tab, and then a "Response" tab. This would be similar to Chrome, and reduce the need for nested tabs, which is a bit weird.

Comment 7

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

Clearing bugs I can't get around to

Comment 8

[Ruturaj Vartak]

Hey Tim,

Was thinking of picking this one, since this one I sorely miss during my development. Its a very old one - Do you have an direction on this one? Or you have anything in mind that I could work on?

Thanks,
Rutu

Comment 9

[Tim Nguyen :ntim]

You can look at https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/components/response-panel.js#152 which shows a JSON tree if you have a JSON response, and shows a editor with the raw response otherwise.

What you want here is a tree like UI:

> Preview
 foo   bar

> Raw
{ foo: bar }


You should simply be able to unwrap the code here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/components/response-panel.js#159 and move it out from the 'else' block.

Btw, there's bug 1353319 which is related to this bug in case you're interested.

Comment 10

[Ruturaj Vartak]

Attachment: tab-suggestion-preview-response.png

Hey Tim,
Thanks. Yes I got that part of the change in the code, but my question was pertaining to the UI. Would something like the screenshot be acceptable?
Rutu

Comment 11

[Ruturaj Vartak]

O... I was reading related bugs related, it seems Preview Panel was actually removed bug#1350229 !! So my screenshot is actually something that re-proposes the original Preview panel :(

However, this would be the difference
- Preview Panel - the existing response panel, generates rendered o/p as is (just renamed to "Preview")
- Response Panel - renders the o/p typically text-based - JSON, HTML, JS, etc as is.

Comment 12

[Tim Nguyen :ntim]

So the preview panel was removed, because we wanted to have less tabs. 

What we'd like to have is a new Preview section (in your screenshot, "JSON" is a section) inside the response tab like I've described in comment 9.

Here's a better ascii mockup than what I've done in comment 9:

---------------------------
 v Preview
---------------------------
  foo  bar
  bar  baz
---------------------------
 v Raw
---------------------------
{
  "foo": "bar",
  "bar": "baz"
}


I hope my ascii mockup is a bit more clear now.

Comment 13

[Tim Nguyen :ntim]

To be clear, the "raw" section is supposed to be an editor, so my mockup should have contained line numbers.

Comment 14

[Ruturaj Vartak]

OK Great... thanks for the quick reply.

Now I understand, So you need to vertical panes (instead of the tabs) within the same tab "Response" (Something similar to Request Headers and Response Headers in Network Side-bar ?)

- Upper Pane ("Preview"), existing as-is
- Lower Pane ("Response Payload"), very much like content-type=HTML 's preview.

I think except for non-JSON o/p, I can omit the lower / raw pane right? Since HTML already just has text. Unless I try to fix even bug#1353319 in here.

If you have more formats to be accommodated, please let me know.

Comment 15

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #14)
> OK Great... thanks for the quick reply.
> 
> Now I understand, So you need to vertical panes (instead of the tabs) within
> the same tab "Response" (Something similar to Request Headers and Response
> Headers in Network Side-bar ?)
Yep, exactly.

> - Upper Pane ("Preview"), existing as-is
> - Lower Pane ("Response Payload"), very much like content-type=HTML 's
> preview.
> 
> I think except for non-JSON o/p, I can omit the lower / raw pane right?
> Since HTML already just has text. Unless I try to fix even bug#1353319 in
> here.

Yep. The upper pane contains a formatted preview of the response (like the existing JSON response preview), or in the case of HTML, the rendered HTML preview. While the lower pane will contain the raw response (actual raw JSON or HTML source).

Comment 16

[Ruturaj Vartak]

Attachment: WIP-1135965-1.patch

- A simple silly WIP patch

- JSON - is it only applicable for JSON?
- Images - continue as-is?
- HTML - continue as-is?
- Javascript - continue as-is?

Problem with this patch, is the UI isn't consistent (2 panes only for JSON) - but it optimal - is that OK?

Also - I don't know if its another bug, even "Content-Type: application/json" JSON responses are being rendered as mime-type text/html. I've verified it against Google Chrome - Hence that hardcoding of application/json

Comment 17

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #16)
> Created attachment 8892835 [details] [diff] [review]
> WIP-1135965-1.patch
> 
> - A simple silly WIP patch
> 
> - JSON - is it only applicable for JSON?
I'm not sure if we have custom previews anywhere else.

> - Images - continue as-is?
Yep.

> - HTML - continue as-is?
HTML should show an iframe preview of the HTML (bug 1353319), which means using 2 panes. But feel free to do this in a separate bug.
> - Javascript - continue as-is?
Yep.

> Problem with this patch, is the UI isn't consistent (2 panes only for JSON)
> - but it optimal - is that OK?
I think it's fine.

> Also - I don't know if its another bug, even "Content-Type:
> application/json" JSON responses are being rendered as mime-type text/html.
> I've verified it against Google Chrome - Hence that hardcoding of
> application/json

As long as they don't match the `HTML` filter button, or mime-type:html, it's fine.

Comment 18

[Tim Nguyen :ntim]

Comment on attachment 8892835 [details] [diff] [review]
WIP-1135965-1.patch

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

::: devtools/client/netmonitor/src/components/response-panel.js
@@ +160,5 @@
> +        EDITOR_CONFIG: {
> +          text,
> +          mode: "application/json",
> +        },
> +      };

The code in the `else` part already does this for other responses, why not simply unwrap it from the else instead of writing it twice ?

if (json) {
  ...
}

object[RESPONSE_PAYLOAD] = {
  EDITOR_CONFIG: {
    text,
    mode: mimeType.replace(/;.+/, "")
  },
};

Comment 19

[Ruturaj Vartak]

Hey Tim,

While I work on your suggestions and get the test-cases running normally as well...

Based on how I test the JSON resource, I'm finding differences as follows.

- compiled - direct JSON resource in a tab - JSON text is base64 encoded
- compiled - JSON resource through XHR - JSON text is valid JSON
- launchpad-based - direct JSON resource in a tab - JSON text is valid JSON
- launchpad-based - JSON resource through XHR - JSON text is valid JSON

So to consider the case of compiled - direct JSON resource - How could I understand the direct JSON resource is hit ? And its in this case when the mime-type = application/vnd.mozilla.json.view

Comment 20

[Ruturaj Vartak]

Attachment: WIP-1135965-2.patch

- worked on your suggestion
- Fixed test cases
- But netmonitor/test/browser_net_jsonp.js test case is timing out with this message, I couldn't pinpoint the issue.

===================================
...

GECKO(2518) | JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/sourceeditor/editor.js, line 578: TypeError: cm is undefined
142 INFO Console message: [JavaScript Error: "TypeError: cm is undefined" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/sourceeditor/editor.js" line: 578}]
Failed to retrieve MOZ_UPLOAD_DIR env var
143 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_jsonp.js | Test timed out - 
144 INFO Removing tab.
145 INFO Waiting for event: 'TabClose' on [object XULElement].
146 INFO Got event: 'TabClose' on [object XULElement].
147 INFO Tab removed and finished closing
148 INFO finish() was called, cleaning up...
GECKO(2518) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
GECKO(2518) | MEMORY STAT | vsize 2249MB | residentFast 329MB | heapAllocated 125MB
149 INFO TEST-OK | devtools/client/netmonitor/test/browser_net_jsonp.js | took 45328ms
150 INFO checking window state
GECKO(2518) | console.log: TELEMETRY PING: {"client_id":"86cba1e1-6c88-4672-b182-0db8b665993f","addon_version":"0.0.0","locale":"en-US","session_id":"{30abd5f1-6ae0-4506-977f-c89dd99239a9}","page":"about:newtab","action":"activity_stream_session","perf":{"load_trigger_type":"unexpected","topsites_first_painted_ts":1501911420259.8267}}
GECKO(2518) | Completed ShutdownLeaks collections in process 2626
GECKO(2518) | Completed ShutdownLeaks collections in process 2633
GECKO(2518) | Completed ShutdownLeaks collections in process 2673
GECKO(2518) | Completed ShutdownLeaks collections in process 2573
GECKO(2518) | Completed ShutdownLeaks collections in process 2518
151 INFO TEST-START | Shutdown
152 INFO Browser Chrome Test Summary
153 INFO Passed:  66
154 INFO Failed:  1
155 INFO Todo:    0
156 INFO Mode:    e10s
157 INFO *** End BrowserChrome Test Results ***
GECKO(2518) | *** UTM:SVC TimerManager:registerTimer called after profile-before-change notification. Ignoring timer registration for id: telemetry_modules_ping
TEST-INFO | Main app process: exit 0
runtests.py | Application ran for: 0:00:50.713472
zombiecheck | Reading PID log: /tmp/tmpLvohv5pidlog
==> process 2518 launched child process 2541
==> process 2518 launched child process 2573
==> process 2518 launched child process 2626
==> process 2518 launched child process 2633
==> process 2518 launched child process 2673
zombiecheck | Checking for orphan process with PID: 2541
zombiecheck | Checking for orphan process with PID: 2573
zombiecheck | Checking for orphan process with PID: 2626
zombiecheck | Checking for orphan process with PID: 2633
zombiecheck | Checking for orphan process with PID: 2673
Stopping web server
Stopping web socket server
Stopping ssltunnel
leakcheck | refcount logging is off, so leaks can't be detected!
runtests.py | Running tests: end.
Buffered messages finished
TEST-INFO | checking window state
Browser Chrome Test Summary
	Passed: 66
	Failed: 1
	Todo: 0
	Mode: e10s
*** End BrowserChrome Test Results ***
The following tests failed:
158 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_jsonp.js | Test timed out - 
Buffered messages finished
SUITE-END | took 51s

===================================

Comment 21

[Tim Nguyen :ntim]

Attachment: Test fix

Here's a test fix.

You were encountering a timing problem. The current test only waits for the `.tree-section` node to appear. With your changes, an editor is shown in the response panel but the editor appears only after `.tree-section`, so we end up executing the rest of the test without the editor loaded in the respone panel, which is problematic since we refresh the panel with the editor unloaded afterwards, which probably is causing the test to throw, hence timing out. You could see that with the error message "cm (codemirror) is undefined". The fix for that is simply to wait for the editor to load.

I've added some info() statements so it's more clear what the test is doing.

Also, I've updated a test expected result (it was previously expecting *not* to have an editor for JSON, but that's no longer true).

I suspect other tests might need to be fixed as well.

Comment 22

[Tim Nguyen :ntim]

Comment on attachment 8894045 [details] [diff] [review]
WIP-1135965-2.patch

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

Thanks for working on this! The UI and the changes in response-panel.js look good to me.

r- mainly because the tests aren't doing their job at the moment :) Let me know if you need any help.

::: devtools/client/netmonitor/test/browser_net_content-type.js
@@ +168,5 @@
>        let jsonView = tabpanel.querySelector(".tree-section .treeLabel") || {};
>        is(jsonView.textContent !== L10N.getStr("jsonScopeName"),
>          box != "json",
>          "The response json view doesn't display");
> +      if (type === "json") {

No need for this extra if condition.

The original code is:

is(tabpanel.querySelector(".CodeMirror-code") === null,
  box != "textarea",
  "The response editor doesn't display");

Just change this to:


is(tabpanel.querySelector(".CodeMirror-code") === null,
  !(box === "textarea" || box === "json"),
  "The response editor doesn't display");

or with De Morgan's law:

is(tabpanel.querySelector(".CodeMirror-code") === null,
  box !== "textarea" && box !== "json"),
  "The response editor doesn't display");

I don't have a preference over these 2.

::: devtools/client/netmonitor/test/browser_net_json-b64.js
@@ +37,5 @@
>    let jsonView = tabpanel.querySelector(".tree-section .treeLabel") || {};
>    is(jsonView.textContent === L10N.getStr("jsonScopeName"), true,
>      "The response json view has the intended visibility.");
>    is(tabpanel.querySelector(".CodeMirror-code") === null, true,
>      "The response editor doesn't have the intended visibility.");

This seems wrong. There should be an editor for JSON, so the expected result is `false`, not `true`. The test needs to be updated.

(It will probably fail if you change it to `false` because the editor isn't loaded yet, so you'll have to fix that too, or I can take care of this if you prefer)

::: devtools/client/netmonitor/test/browser_net_json-long.js
@@ +67,5 @@
>      let jsonView = tabpanel.querySelector(".tree-section .treeLabel") || {};
>      is(jsonView.textContent === L10N.getStr("jsonScopeName"), true,
>        "The response json view has the intended visibility.");
>      is(tabpanel.querySelector(".CodeMirror-code") === null, true,
>        "The response editor doesn't have the intended visibility.");

Same here.

::: devtools/client/netmonitor/test/browser_net_json_custom_mime.js
@@ +61,5 @@
>      let jsonView = tabpanel.querySelector(".tree-section .treeLabel") || {};
>      is(jsonView.textContent === L10N.getStr("jsonScopeName"), true,
>        "The response json view has the intended visibility.");
>      is(tabpanel.querySelector(".CodeMirror-code") === null, true,
>        "The response editor doesn't have the intended visibility.");

Same here.

Comment 23

[Ruturaj Vartak]

Attachment: fix-1135965-1.patch

- worked on your suggestions for if / then statement
- fixed all JSON test cases to have "have" .CodeMirror-code DOM element

I had another question, if I add another yield waitForDOM(..), why does a test time out?

Comment 24

[Tim Nguyen :ntim]

Comment on attachment 8894764 [details] [diff] [review]
fix-1135965-1.patch

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

Can you check whether one of these need to be updated ? https://dxr.mozilla.org/mozilla-central/search?q=tabpanel.querySelector(%22.CodeMirror-code%22)+%3D%3D%3D+null&redirect=false

::: devtools/client/netmonitor/test/browser_net_json-b64.js
@@ +36,5 @@
>      "The response error header doesn't have the intended visibility.");
>    let jsonView = tabpanel.querySelector(".tree-section .treeLabel") || {};
>    is(jsonView.textContent === L10N.getStr("jsonScopeName"), true,
>      "The response json view has the intended visibility.");
> +  is(tabpanel.querySelector(".CodeMirror-code") !== null, true,

Please be consistent with the check below, which uses `===`. You should change true to false instead.

::: devtools/client/netmonitor/test/browser_net_json-long.js
@@ +66,5 @@
>        "The response error header doesn't have the intended visibility.");
>      let jsonView = tabpanel.querySelector(".tree-section .treeLabel") || {};
>      is(jsonView.textContent === L10N.getStr("jsonScopeName"), true,
>        "The response json view has the intended visibility.");
> +    is(tabpanel.querySelector(".CodeMirror-code") !== null, true,

Same here.

::: devtools/client/netmonitor/test/browser_net_json-null.js
@@ +54,5 @@
>        "The response error header doesn't have the intended visibility.");
>      let jsonView = panel.querySelector(".tree-section .treeLabel") || {};
>      is(jsonView.textContent === L10N.getStr("jsonScopeName"), true,
>        "The response json view has the intended visibility.");
> +    is(panel.querySelector(".CodeMirror-code") !== null, true,

Same here.

::: devtools/client/netmonitor/test/browser_net_json_custom_mime.js
@@ +60,5 @@
>        "The response error header doesn't have the intended visibility.");
>      let jsonView = tabpanel.querySelector(".tree-section .treeLabel") || {};
>      is(jsonView.textContent === L10N.getStr("jsonScopeName"), true,
>        "The response json view has the intended visibility.");
> +    is(tabpanel.querySelector(".CodeMirror-code") !== null, true,

Same here.

::: devtools/client/netmonitor/test/browser_net_jsonp.js
@@ +85,5 @@
>        "The response error header doesn't have the intended visibility.");
>      is(tabpanel.querySelector(".tree-section .treeLabel").textContent,
>        L10N.getFormatStr("jsonpScopeName", func),
>        "The response json view has the intened visibility and correct title.");
> +    is(tabpanel.querySelector(".CodeMirror-code") !== null, true,

Same here.

Comment 25

[Ruturaj Vartak]

Attachment: fix-1135965-2.patch

- Fixed the === consistency issue.

Out of the search on dxr ...
analyses params o/p
- devtools/client/netmonitor/test/browser_net_complex-params.js
- devtools/client/netmonitor/test/browser_net_post-data-01.js
do we have to show Request Payload for JSON ?

touched / worked
- devtools/client/netmonitor/test/browser_net_content-type.js
- devtools/client/netmonitor/test/browser_net_json-b64.js
- devtools/client/netmonitor/test/browser_net_json-long.js
- devtools/client/netmonitor/test/browser_net_json-malformed.js
- devtools/client/netmonitor/test/browser_net_json_custom_mime.js
- devtools/client/netmonitor/test/browser_net_json_text_mime.js
- devtools/client/netmonitor/test/browser_net_jsonp.js

Comment 26

[Tim Nguyen :ntim]

Comment on attachment 8896938 [details] [diff] [review]
fix-1135965-2.patch

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

LGTM, I don't see any more issues.

Comment 27

[Ruturaj Vartak]

Thanks Tim,
I've assigned the keyword checkin-needed - hope that was the next thing required.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d266b8859616
JSON Response pane should contain both "Preview" and "Response Payload" views. r=ntim

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d266b8859616