JSON Viewer: manually auto-expand documents

RESOLVED FIXED in Firefox 62

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: Honza, Assigned: smclt30p)

Tracking

unspecified
Firefox 62
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

This is a follow up for Bug 1247064

JSON documents that are not bigger than 100KB are autoexpanded, but there should also be a toolbar-button that can be used to auto expand manually any document.

Honza
Priority: -- → P3
Assignee

Comment 1

a year ago
This is a patch that adds two buttons to the toolbar in the "JSON" tab.
The buttons are:

"Expand All" - Expands the whole JSON tree
"Collapse All" - Collapses the whole JSON tree

See the attached screenshot for a reference.
Attachment #8964276 - Flags: review?(odvarko)
Comment on attachment 8964276 [details] [diff] [review]
1264557-add-expand-collapse.patch

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

This is nice feature, thanks for the patch!

Please see my inline comments.

Also, my biggest worry is about performance.
Expanding big JSON docs can be time consuming and it can freeze the browser
for significant amount of time. We want to avoid that.

I think that the `Expand All` button should be there only for documents
smaller then AUTO_EXPAND_MAX_SIZE.

The Collapse button seem to be fine, I don't see perf penalties there.

Honza

::: devtools/client/jsonview/json-viewer.js
@@ +86,5 @@
>        input.prettified = !input.prettified;
>      },
> +
> +    onCollapseTree: function() {
> +      input.expandedNodes.clear();

`input.expandedNodes` is null if the document is bigger then AUTO_EXPAND_MAX_SIZE.

The `expandedNodes` set it only set if the tree is auto expanded.

Search for this comment:
// Expand the document by default if its size isn't bigger than 100KB.

I think that it should be initialized to an empty set when the application state object `input` is defined (at the top of the file).

@@ +91,5 @@
> +      theApp.forceUpdate();
> +    },
> +
> +    onExpandTree: function() {
> +      input.expandedNodes.clear();

`input.expandedNodes` is null if the document is bigger then AUTO_EXPAND_MAX_SIZE.
Attachment #8964276 - Flags: review?(odvarko)
Duplicate of this bug: 1451277
Assignee

Comment 5

a year ago
I think that the button should stay functional regardless of the document size. 

It makes sense to disallow such a long-running operation on a document that is automatically loaded, but I think it is acceptable on a action that is triggered by the user.

I would like to hear your comment about this, and then I will fix the nullpointer issue you pointed out.

Thanks for the time!
Comment on attachment 8964276 [details] [diff] [review]
1264557-add-expand-collapse.patch

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

::: devtools/client/jsonview/json-viewer.js
@@ +94,5 @@
> +    onExpandTree: function() {
> +      input.expandedNodes.clear();
> +      TreeViewClass.getExpandedNodes(
> +        input.json
> +      ).forEach((item) => {

Creating a set with all nodes can be expensive. Maybe it would be better to have a TreeView flag which says whether nodes are expanded or collapsed by default. If they are hidden by default, expandedNodes contains the ones which are expanded. If they are expanded by default, introduce a new collapsedNodes set which contains the ones which are collapsed.

What do you think? This would require some changes in https://searchfox.org/mozilla-central/source/devtools/client/shared/components/tree/TreeView.js

@@ +95,5 @@
> +      input.expandedNodes.clear();
> +      TreeViewClass.getExpandedNodes(
> +        input.json
> +      ).forEach((item) => {
> +        input.expandedNodes.add(item);

Why do you copy all set items into the old set, one by one?
I would just replace the set, i.e.

  input.expandedNodes = TreeViewClass.getExpandedNodes(input.json);
  theApp.setState({expandedNodes: input.expandedNodes});

@@ +97,5 @@
> +        input.json
> +      ).forEach((item) => {
> +        input.expandedNodes.add(item);
> +      });
> +      theApp.forceUpdate();

If you replace the set (see comment above), I think forceUpdate is not needed.
Assignee

Comment 7

a year ago
I copied all the stuff as the tree was not updating after a replace and forceUpdate(). No idea what was that.

I think the whole idea of removing and adding tree items is very expensive. Could we maybe collapse the rows in the TreeView to 0px instead of removing them?
(In reply to Ognjen Galic from comment #7)
> I think the whole idea of removing and adding tree items is very expensive.
> Could we maybe collapse the rows in the TreeView to 0px instead of removing
> them?

React can't handle big trees in a reasonable amount of time. If you always render all rows (even if then you hide them with CSS) I suspect it would be much worse.
(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> Also, my biggest worry is about performance.
> Expanding big JSON docs can be time consuming and it can freeze the browser
> for significant amount of time. We want to avoid that.
> 
> I think that the `Expand All` button should be there only for documents
> smaller then AUTO_EXPAND_MAX_SIZE.
> 
> The Collapse button seem to be fine, I don't see perf penalties there.

(In reply to Ognjen Galic from comment #5)
> I think that the button should stay functional regardless of the document
> size. 
> 
> It makes sense to disallow such a long-running operation on a document that
> is automatically loaded, but I think it is acceptable on a action that is
> triggered by the user.

@Harald, what do you think about this?
Should we let the use to expand-all (manually) even if it can freeze the browser?

Honza
Flags: needinfo?(hkirschner)
> React can't handle big trees in a reasonable amount of time. If you always render all rows (even if then you hide them with CSS) I suspect it would be much worse.

Where is the time spent? Would React 16's new async methods that stay within frame budget help here as follow up work?

> Should we let the use to expand-all (manually) even if it can freeze the browser?

Sounds OK, as long as there is visual feedback (active state + compositor based animation).

- Are there other issues that users could run into like memory that could lead to worse experience than hangs?
- Does this hang main or content thread?
Flags: needinfo?(hkirschner)
Assignee

Comment 11

a year ago
(In reply to :Harald Kirschner :digitarald from comment #10)
> > React can't handle big trees in a reasonable amount of time. If you always render all rows (even if then you hide them with CSS) I suspect it would be much worse.
> 
> Where is the time spent? Would React 16's new async methods that stay within
> frame budget help here as follow up work?
> 
> > Should we let the use to expand-all (manually) even if it can freeze the browser?
> 
> Sounds OK, as long as there is visual feedback (active state + compositor
> based animation).

Is there any examples I could use for info about Firefox's internal threads or any documentation how to offload work from the display thread?

> 
> - Are there other issues that users could run into like memory that could
> lead to worse experience than hangs?

I did some memory profiles and seems that the garbage collector does it's job fine, no object retention.

> - Does this hang main or content thread?

It does hang some thread. Don't know which.
Posted image jsonview-hang.png
(In reply to :Harald Kirschner :digitarald from comment #10)
> Where is the time spent?

Time is spent in react, which is blatantly slow.
If all rows are rendered, then the render() method will take a lot.
And then, anytime something changes, react will diff. Since both the old and new trees will be huge, this will also be very expensive.

> Would React 16's new async methods that stay within frame budget help here as follow up work?

I don't know these methods, but I suspect there is no hope unless we get rid of react.

> Does this hang main or content thread?

Content thread. See attached image.
Assignee

Comment 13

a year ago
This is a patch that has a couple of changes in relation to the past patch.

* The expandedNodes set is not copied one-by-one, but is replaced as suggested by Oriol
* The "Expand All" button is now hidden for documents larger than 100kB, the same size 
   that disables auto-expand, as suggested by Honza.
Attachment #8964276 - Attachment is obsolete: true
Attachment #8973434 - Flags: review?(odvarko)
Assignee

Comment 14

a year ago
Sorry, this is the same patch as the previous one but a rogue "console.log" has been removed.
Attachment #8973438 - Flags: review?(odvarko)
Assignee

Updated

a year ago
Attachment #8973438 - Flags: review?(odvarko)
Assignee

Updated

a year ago
Attachment #8973434 - Flags: review?(odvarko)
Assignee

Updated

a year ago
Attachment #8973438 - Flags: review?(odvarko)
Assignee

Updated

a year ago
Attachment #8973434 - Attachment is obsolete: true
Assignee

Comment 15

a year ago
Again, sorry for the mess, haven't used Bugzilla in a while.
Comment on attachment 8973438 [details] [diff] [review]
1264557-add-expand-collapse-v3.patch

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

Looks good to me, thanks!

We also need a test for this new feature. You can take a look at test/browser_jsonview_copy_json.js and get some inspiration (the new test will be very similar).

Honza
Attachment #8973438 - Flags: review?(odvarko)
Assignee

Updated

a year ago
Attachment #8973438 - Attachment is obsolete: true
Assignee

Comment 17

a year ago
This is a small, little copy mistake that I did not saw. It did not impact the functionality but it inhibited testing. What I did was assign the same class for the "Expand All" and "Collapse All" buttons. 

That is fixed in this v4 patch, its just a one-line change.
Attachment #8974293 - Flags: review?(odvarko)
Assignee

Comment 18

a year ago
This are the suggested changes. This is a test-case for the expand and collapse all buttons.
Attachment #8974294 - Flags: review?(odvarko)
Comment on attachment 8974293 [details] [diff] [review]
1264557-add-expand-collapse-v4.patch

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

> This is a small, little copy mistake that I did not saw. It did not impact the functionality but it inhibited testing.
> What I did was assign the same class for the "Expand All" and "Collapse All" buttons. 

I see, looks good, thanks!
Honza
Attachment #8974293 - Flags: review?(odvarko) → review+
Comment on attachment 8974294 [details] [diff] [review]
1264557-add-test-cases-for-expand-collapse-buttons.patch

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

Thanks for implementing the test!

Looks good, just couple of typos

R+ assuming try is green.

You don't have to ask for another review after fixing the typos.
(just set the review flag to R+ manually, and don't forget to add checkin-needed keyword when the patch is ready)

Honza

::: devtools/client/jsonview/test/browser_jsonview_expand_collapse.js
@@ +15,5 @@
> +  let browser = gBrowser.selectedBrowser, selector, countAfter, countBefore, json;
> +
> +  /* Initial sanity check */
> +  countBefore = await getElementCount(".treeRow");
> +  ok(countBefore == 6, "There must be six row");

nit: six row => six rows

@@ +27,5 @@
> +  /* Test the "Expand All" button */
> +  selector = ".jsonPanelBox .toolbar button.expand";
> +  await BrowserTestUtils.synthesizeMouseAtCenter(selector, {}, browser);
> +  countAfter = await getElementCount(".treeRow");
> +  ok(countAfter == 6, "There mus be six expanded rows");

nit: there mus => there must
Attachment #8974294 - Flags: review?(odvarko) → review+
Assignee

Comment 21

a year ago
Thanks for the reviews and pointers. This fixes the nits.
Attachment #8974294 - Attachment is obsolete: true
Attachment #8974324 - Flags: review+
Assignee

Comment 22

a year ago
Seems like I can't edit the keywords. Is it because I did not report the bug?
(In reply to Ognjen Galic from comment #22)
> Seems like I can't edit the keywords. Is it because I did not report the bug?
I see, it requires commit access.

You might consider becoming Mozilla committer. Read more here:
https://www.mozilla.org/en-US/about/governance/policies/commit/

I am adding the keyword now.

Thanks for the help!
Honza
Keywords: checkin-needed

Comment 24

a year ago
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3bf0a95e4e4
Add expand all/collapse all buttons. r=honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/e17cd801b578
Add test cases for expand/collapse buttons. r=honza
Keywords: checkin-needed
Backed out for ESlint failure.

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/8cd4cabdce0bc4c55cbb6a6c783563286af0bcc2
push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ac582f3509d43aea21251329dae9f314b4875681
failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=177834397&repo=mozilla-inbound&lineNumber=252

[task 2018-05-10T10:33:36.871Z] New python executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python2.7
[task 2018-05-10T10:33:36.871Z] Also creating executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python
[task 2018-05-10T10:33:38.514Z] Installing setuptools, pip, wheel...done.
[task 2018-05-10T10:33:39.560Z] running build_ext
[task 2018-05-10T10:33:39.560Z] building 'psutil._psutil_linux' extension
[task 2018-05-10T10:33:39.560Z] creating build
[task 2018-05-10T10:33:39.560Z] creating build/temp.linux-x86_64-2.7
[task 2018-05-10T10:33:39.560Z] creating build/temp.linux-x86_64-2.7/psutil
[task 2018-05-10T10:33:39.560Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-05-10T10:33:39.561Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-05-10T10:33:39.561Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o
[task 2018-05-10T10:33:39.561Z] creating build/lib.linux-x86_64-2.7
[task 2018-05-10T10:33:39.562Z] creating build/lib.linux-x86_64-2.7/psutil
[task 2018-05-10T10:33:39.562Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so
[task 2018-05-10T10:33:39.562Z] building 'psutil._psutil_posix' extension
[task 2018-05-10T10:33:39.563Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-05-10T10:33:39.563Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-05-10T10:33:39.564Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2018-05-10T10:33:39.564Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2018-05-10T10:33:39.564Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-05-10T10:33:39.564Z] 
[task 2018-05-10T10:33:39.564Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-05-10T10:38:46.689Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/jsonview/test/browser_jsonview_expand_collapse.js:39:1 | Block must not be padded by blank lines. (padded-blocks)
[taskcluster 2018-05-10 10:38:46.963Z] === Task Finished ===
[taskcluster 2018-05-10 10:38:46.963Z] Unsuccessful task run with exit code: 1 completed in 552.276 seconds
Flags: needinfo?(smclt30p)
Flags: needinfo?(odvarko)
@Ognjen: from the log:

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/jsonview/test/browser_jsonview_expand_collapse.js:39:1 | Block must not be padded by blank lines. (padded-blocks)

Honza
Flags: needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> I see, it requires commit access.

More than commit access, I think it requires the bugzilla editbugs privilege.
https://developer.mozilla.org/docs/Mozilla/Bugzilla/What_to_do_and_what_not_to_do_in_Bugzilla#Editbugs_privilege

To obtain it, see
https://bugzilla.mozilla.org/page.cgi?id=get_permissions.html
Assignee: nobody → smclt30p
Status: NEW → ASSIGNED
Assignee

Comment 28

a year ago
This is a really funny issue. The whole fix is removing a single newline. 
This patch is the same as the previous, the only difference is the newline on line 39.
Attachment #8974324 - Attachment is obsolete: true
Flags: needinfo?(smclt30p)
Attachment #8974781 - Flags: review+
Assignee

Updated

a year ago
Keywords: checkin-needed
Backed out changeset c45f75d2190f (bug 1264557) for failing pt-test-verify-e10s on devtools/client/jsonview/test/browser_jsonview_expand_collapse.j on a CLOSED TREE
Failing log: https://treeherder.mozilla.org/logviewer.html#?job_id=177963750&repo=mozilla-inbound&lineNumber=1965
Backout revision https://hg.mozilla.org/integration/mozilla-inbound/rev/23e1618c1c3959196c3b22fc44d9e69733306115
Failing revision https://hg.mozilla.org/integration/mozilla-inbound/rev/c45f75d2190f9ac306c9694d4add288408ea1aba


[task 2018-05-10T23:03:43.387Z] 23:03:43     INFO - TEST-START | devtools/client/jsonview/test/browser_jsonview_expand_collapse.js
[task 2018-05-10T23:03:44.801Z] 23:03:44     INFO - TEST-INFO | started process screentopng
[task 2018-05-10T23:03:45.213Z] 23:03:45     INFO - TEST-INFO | screentopng: exit 0
[task 2018-05-10T23:03:45.213Z] 23:03:45     INFO - Buffered messages logged at 23:03:43
[task 2018-05-10T23:03:45.217Z] 23:03:45     INFO - Entering test bound 
[task 2018-05-10T23:03:45.218Z] 23:03:45     INFO - Test expand/collapse JSON started
[task 2018-05-10T23:03:45.219Z] 23:03:45     INFO - Adding a new JSON tab with URL: 'http://example.com/browser/devtools/client/jsonview/test/array_json.json'
[task 2018-05-10T23:03:45.220Z] 23:03:45     INFO - Adding a new tab with URL: http://example.com/browser/devtools/client/jsonview/test/array_json.json
[task 2018-05-10T23:03:45.222Z] 23:03:45     INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 170}]
[task 2018-05-10T23:03:45.223Z] 23:03:45     INFO - Buffered messages logged at 23:03:44
[task 2018-05-10T23:03:45.225Z] 23:03:45     INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 170}]
[task 2018-05-10T23:03:45.226Z] 23:03:45     INFO - Loading the helper frame script chrome://mochitests/content/browser/devtools/client/shared/test/frame-script-utils.js
[task 2018-05-10T23:03:45.227Z] 23:03:45     INFO - DocReadyState is "loading". Await "complete"
[task 2018-05-10T23:03:45.229Z] 23:03:45     INFO - DocReadyState is "interactive". Await "complete"
[task 2018-05-10T23:03:45.230Z] 23:03:45     INFO - AppReadyState is "uninitialized". Await "complete"
[task 2018-05-10T23:03:45.231Z] 23:03:45     INFO - Tab added and finished loading
[task 2018-05-10T23:03:45.232Z] 23:03:45     INFO - AppReadyState is "loading". Await "complete"
[task 2018-05-10T23:03:45.233Z] 23:03:45     INFO - Get element count: '.treeRow'
[task 2018-05-10T23:03:45.235Z] 23:03:45     INFO - Sending message Test:JsonView:GetElementCount to content
[task 2018-05-10T23:03:45.236Z] 23:03:45     INFO - Expecting message Test:JsonView:GetElementCount from content
[task 2018-05-10T23:03:45.237Z] 23:03:45     INFO - TEST-PASS | devtools/client/jsonview/test/browser_jsonview_expand_collapse.js | There must be six rows - 
[task 2018-05-10T23:03:45.238Z] 23:03:45     INFO - Get element count: '.treeRow'
[task 2018-05-10T23:03:45.240Z] 23:03:45     INFO - Sending message Test:JsonView:GetElementCount to content
[task 2018-05-10T23:03:45.241Z] 23:03:45     INFO - Expecting message Test:JsonView:GetElementCount from content
[task 2018-05-10T23:03:45.242Z] 23:03:45     INFO - Buffered messages finished
[task 2018-05-10T23:03:45.243Z] 23:03:45     INFO - TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_jsonview_expand_collapse.js | There must be three rows - 
[task 2018-05-10T23:03:45.244Z] 23:03:45     INFO - Stack trace:
[task 2018-05-10T23:03:45.245Z] 23:03:45     INFO - chrome://mochitests/content/browser/devtools/client/jsonview/test/browser_jsonview_expand_collapse.js:null:25
[task 2018-05-10T23:03:45.247Z] 23:03:45     INFO - Get element count: '.treeRow'
[task 2018-05-10T23:03:45.248Z] 23:03:45     INFO - Sending message Test:JsonView:GetElementCount to content
[task 2018-05-10T23:03:45.249Z] 23:03:45     INFO - Expecting message Test:JsonView:GetElementCount from content
[task 2018-05-10T23:03:45.250Z] 23:03:45     INFO - TEST-PASS | devtools/client/jsonview/test/browser_jsonview_expand_collapse.js | There must be six expanded rows - 
[task 2018-05-10T23:03:45.251Z] 23:03:45     INFO - TEST-PASS | devtools/client/jsonview/test/browser_jsonview_expand_collapse.js | The generated JSON must be larger than 100kB -
Flags: needinfo?(smclt30p)
Assignee

Comment 31

a year ago
Is this failing because I need to roll up the two patches into one in order for it to be tested as one unit?
Flags: needinfo?(odvarko)
Assignee

Comment 32

a year ago
This has failed because the patch set has not been applied in a series but the tests have been run without the previous patch that implements that functionality.

See: https://hg.mozilla.org/integration/mozilla-inbound/file/c45f75d2190f9ac306c9694d4add288408ea1aba/devtools/client/jsonview/components/JsonToolbar.js

There are no buttons in that revision for the test to click.
Flags: needinfo?(smclt30p)
Assignee

Comment 33

a year ago
So, ehm, what to do here? Roll up the patch or?
(In reply to Ognjen Galic from comment #33)
> So, ehm, what to do here? Roll up the patch or?

I don't know what that happened. But, please merge both
patches and attach just one file.

Thanks!
Honza
Flags: needinfo?(odvarko) → needinfo?(smclt30p)
Assignee

Comment 35

a year ago
This is the patchset squashed into one patch.
Attachment #8974293 - Attachment is obsolete: true
Attachment #8974781 - Attachment is obsolete: true
Flags: needinfo?(smclt30p)
Attachment #8976641 - Flags: review+
Assignee

Updated

a year ago
Keywords: checkin-needed

Comment 36

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e03a8d74e9eb
Add expand all/collapse all buttons. r=Honza
Keywords: checkin-needed

Comment 37

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e03a8d74e9eb
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.