1417039 - Do not defer loading JSON Viewer

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, enhancement, P3)

Description

[Oriol Brufau [:Oriol]]

Currently the JSON Viewer is not loaded until all the JSON has been received.

It would be better to start loading it as soon as possible.

Since we don't have a partial JSON parser, the tree can't be displayed until all data has been received. Then I would select the Raw Data tab by default, so that the user can see how the JSON is being downloaded, and at the end automatically switch to the JSON tab.

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: Bug 1417039 - Do not defer loading JSON Viewer.

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

Comment 2

[Oriol Brufau [:Oriol]]

This ended up being a bigger patch than I expected, but mostly it's due to tests. In order to be able to test this, I added a JSONView.readyState (inspired by document.readyState) to know the load state of the app. I modified the addJsonViewTab test function to use these states instead of a timeout, and adapted the only test that used that timeout. While I was at it, I sorted some browser.ini entries which were at the wrong position.

Other than this, the patch basically passes a DOM text node to react instead of a string. This way the node can be updated and the change happens immediately. Passing a string or an array of string chunks to react had awful, awful, awful performance for big JSON data.

Comment 3

[Oriol Brufau [:Oriol]]

Comment on attachment 8928664 [details]
Bug 1417039 - Do not defer loading JSON Viewer.

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

Comment 4

[Oriol Brufau [:Oriol]]

Rebased the patch.

Comment 5

[Oriol Brufau [:Oriol]]

Comment on attachment 8928664 [details]
Bug 1417039 - Do not defer loading JSON Viewer.

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

Comment 6

[Oriol Brufau [:Oriol]]

Comment on attachment 8928664 [details]
Bug 1417039 - Do not defer loading JSON Viewer.

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

Comment 7

[Oriol Brufau [:Oriol]]

Found a less hacky way to wait until the tab is ready but not loaded. And renamed the "initial" JSONView.readyState to "uninitialized" because that's the non-standard document.readyState value that Firefox uses.

Comment 8

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

Comment on attachment 8928664 [details]
Bug 1417039 - Do not defer loading JSON Viewer.

https://reviewboard.mozilla.org/r/199902/#review208470

Thanks for working on this.

Just a few nit inline comments

Also, do you have a test page I could try to test the new behavior?

Honza

::: devtools/client/jsonview/test/browser_jsonview_chunked_json.js:19
(Diff revision 4)
> +    appReadyState: "interactive",
> +    docReadyState: "loading",
> +  });
> +
> +  is(await getElementCount(".rawdata.is-active"), 1,
> +     "The Raw Data tab is selected.");

nit: indentation should be 2 spaces

::: devtools/client/jsonview/test/browser_jsonview_chunked_json.js:25
(Diff revision 4)
> +
> +  // Write some text and check that it is displayed.
> +  await write("[");
> +  await checkText();
> +
> +  // Reapeat just in case.

typo: Reapeat -> Repeat

::: devtools/client/jsonview/test/browser_jsonview_chunked_json.js:30
(Diff revision 4)
> +  // Reapeat just in case.
> +  await write("1,");
> +  await checkText();
> +
> +  is(await getElementCount("button.prettyprint"), 0,
> +     "There is no pretty print button during load");

nit: indentation should be 2 spaces

::: devtools/client/jsonview/test/browser_jsonview_chunked_json.js:37
(Diff revision 4)
> +  await selectJsonViewContentTab("json");
> +  is(await getElementText(".jsonPanelBox > .panelContent"), "", "There is no JSON tree");
> +
> +  await selectJsonViewContentTab("headers");
> +  ok(await getElementText(".headersPanelBox .netInfoHeadersTable"),
> +     "The headers table has been filled.");

nit: indentation should be 2 spaces

::: devtools/client/jsonview/test/browser_jsonview_chunked_json.js:54
(Diff revision 4)
> +  await server("close");
> +
> +  is(await getElementCount(".json.is-active"), 1, "The JSON tab is selected.");
> +
> +  is(await getElementCount(".jsonPanelBox .treeTable .treeRow"), 2,
> +     "There is a tree with 2 rows.");

nit: indentation should be 2 spaces

Comment 9

[Oriol Brufau [:Oriol]]

(In reply to Jan Honza Odvarko [:Honza] from comment #8)
> Also, do you have a test page I could try to test the new behavior?

If you have a server with php, this will do the trick:

<?php
header('Content-type: application/json');
set_time_limit(0);
$i = 0;
echo "[\n";
while($i < 10) {
  ++$i;
  echo "    ", $i, ",\n";
  ob_end_flush();
  flush();
  ob_start();
  sleep(1);
}
echo "  0\n]";
?>

Comment 10

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

Comment on attachment 8928664 [details]
Bug 1417039 - Do not defer loading JSON Viewer.

https://reviewboard.mozilla.org/r/199902/#review208966

(In reply to Oriol Brufau [:Oriol] from comment #9)
> (In reply to Jan Honza Odvarko [:Honza] from comment #8)
> > Also, do you have a test page I could try to test the new behavior?
> 
> If you have a server with php, this will do the trick:
> 
> <?php
> header('Content-type: application/json');
> set_time_limit(0);
> $i = 0;
> echo "[\n";
> while($i < 10) {
>   ++$i;
>   echo "    ", $i, ",\n";
>   ob_end_flush();
>   flush();
>   ob_start();
>   sleep(1);
> }
> echo "  0\n]";
> ?>
Thanks!

Just for the record, I had to also switch off compression to make it work (in my .htaccess file)
<FilesMatch "\.(php|html?)$">
php_flag zlib.output_compression off
</FilesMatch>

Works great for me!

R+

Honza

Comment 11

[Oriol Brufau [:Oriol]]

Comment on attachment 8928664 [details]
Bug 1417039 - Do not defer loading JSON Viewer.

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

Comment 12

[Oriol Brufau [:Oriol]]

Rebased the patch and fixed the nits.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65334d5e3012
Do not defer loading JSON Viewer. r=Honza

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/65334d5e3012

Comment 15

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 65334d5e3012 (bug 1417039) for frequently failing in devtools/client/jsonview/test/browser_jsonview_chunked_json.js (Bug 1421520) r=backout a=backout

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

More information is available in Bug 1421520

Comment 18

[Oriol Brufau [:Oriol]]

OK, so I guess that after closing the connection I should wait an AppReadyStateChange event to make sure the app is loaded.

Comment 19

[Oriol Brufau [:Oriol]]

Comment on attachment 8928664 [details]
Bug 1417039 - Do not defer loading JSON Viewer.

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

Comment 20

[Oriol Brufau [:Oriol]]

Comment on attachment 8928664 [details]
Bug 1417039 - Do not defer loading JSON Viewer.

I think now it should work.

Comment 21

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

Comment on attachment 8928664 [details]
Bug 1417039 - Do not defer loading JSON Viewer.

https://reviewboard.mozilla.org/r/199902/#review210102

Looks reasonable
R+

Honza

Comment 22

[Oriol Brufau [:Oriol]]

Probably the patch caused bug 1421882 and bug 1421956.

INFO - Adding a new JSON tab with URL: 'data:application/json,{"x":{"type":"string"}}'
INFO - Adding a new tab with URL: data:application/json,{"x":{"type":"string"}}
INFO - Expecting message Test:JsonView:DocReadyStateChange from content
INFO - Tab added and finished loading
INFO - Buffered messages finished
INFO - TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_jsonview_object-type.js | Test timed out - 

A time out while waiting for Test:JsonView:DocReadyStateChange. Maybe the ready state changes between reading it and adding the listener? Normally this shouldn't happen, but it seems there is some dark magic that allows to access the CPOW, and if it's in another process I guess it's possible.

  let {document} = content.window;
  while (docReadyStates.indexOf(document.readyState) < docReadyIndex) {
    await waitForContentMessage("Test:JsonView:DocReadyStateChange");
  }

Comment 23

[Oriol Brufau [:Oriol]]

Comment on attachment 8928664 [details]
Bug 1417039 - Do not defer loading JSON Viewer.

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

Comment 24

[Oriol Brufau [:Oriol]]

This should fix bug 1421882. Will think if I can do something for bug 1421956.

Comment 25

[Oriol Brufau [:Oriol]]

Comment on attachment 8928664 [details]
Bug 1417039 - Do not defer loading JSON Viewer.

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

Comment 26

[Oriol Brufau [:Oriol]]

Comment on attachment 8928664 [details]
Bug 1417039 - Do not defer loading JSON Viewer.

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

Comment 27

[Oriol Brufau [:Oriol]]

I think using ContentTask.spawn to run the code in the content process might solve bug 1421956.

Comment 28

[Oriol Brufau [:Oriol]]

Comment on attachment 8928664 [details]
Bug 1417039 - Do not defer loading JSON Viewer.

I did some try pushes and all seem OK
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd0dcb9b86e6
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d522e34d5257
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe5ef3f024d1
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05b72eb65795

Comment 29

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

Comment on attachment 8928664 [details]
Bug 1417039 - Do not defer loading JSON Viewer.

https://reviewboard.mozilla.org/r/199902/#review210544

Thanks for the update Oriol, looks reasonable.

Honza

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b81d2b60acf
Do not defer loading JSON Viewer. r=Honza

Comment 31

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8b81d2b60acf