1370139 - TreeView path with slash hides another object

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, enhancement, P2)

Description

[Oriol Brufau [:Oriol]]

Load data:application/manifest+json,{"a/b":[1,2],"a":{"b":[3,4]}}

root["a/b"] is expanded, and thus root["a"]["b"] can't be expanded.

> let nodePath = path + "/" + prop;

It's scary that there are three occurrences of this code.

https://dxr.mozilla.org/mozilla-central/rev/8a3aa1701537ea6b8334f432cd030d260d492fa3/devtools/client/jsonview/components/json-panel.js#79

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: tree-paths.patch

There were three getExpandedNodes methods:

 - In json-panel.js, using a MAX_LEVEL constraint.
 - In properties-view.js, using MAX_LEVEL and MAX_NODES constraints. This algorithm attempted not to half-expand subtrees, but this doesn't make much sense when iterating depth-first.
 - In security-panel.js, without constraints.

I removed them all and defined a method in tree-view.js which constructs paths using proper escaping. It needs an additional queue because I used a breadth-first traversal in order to handle MAX_NODES constraint properly.

Despite this, the time needed to create the set of expanded nodes for https://a.4cdn.org/boards.json went down from 70 ms to 45 ms (35% reduction).

I guess I could do even better with a depth-first traversal, but having different algorithms is probably not worth, this is not the bottleneck by far. And thanks to bug 1348772 array shift is now O(1) anyways.

Comment 2

[Oriol Brufau [:Oriol]]

Attachment: tree-paths.patch

Forgot to workaround typeof null==="object"

Comment 3

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

Comment on attachment 8874675 [details] [diff] [review]
tree-paths.patch

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

Thanks for working on this!

R+


Just one note, I am not sure if defining API on React component
is the best we can do. But works for me at this point.
Any tips?

Honza

Comment 4

[Oriol Brufau [:Oriol]]

I'm not used to react. If there is some more appropriate place, I guess it can be moved in another bug.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa7a23344ca4
Escape slashes in TreeView paths. r=Honza

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=106769371&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/524e61a71e2709c4d8cd05bc211737b17c6e4d61

Comment 7

[Oriol Brufau [:Oriol]]

OK, it seems DomTree converts array indices to integers
https://dxr.mozilla.org/mozilla-central/rev/da66c4a05fda49d457d9411a7092fed87cf9e53a/devtools/client/dom/content/reducers/grips.js#61-65

They need to be converted back to strings.

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

Comment 8

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

(In reply to Oriol [:Oriol] from comment #7)
> OK, it seems DomTree converts array indices to integers
> https://dxr.mozilla.org/mozilla-central/rev/
> da66c4a05fda49d457d9411a7092fed87cf9e53a/devtools/client/dom/content/
> reducers/grips.js#61-65
> 
> They need to be converted back to strings.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=84a6c4f68a0a1fd40f39343280625346d824b684
Nicely green, thanks for investigating this!

Honza

Comment 9

[Oriol Brufau [:Oriol]]

Attachment: tree-paths-v3.patch

Using String(subKey).replace instead of subKey.replace

Should land after bug 1370443, which affects surrounding code.

Comment 10

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

Comment on attachment 8879521 [details] [diff] [review]
tree-paths-v3.patch

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

Looks reasonable to me

R+

Honza

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cea239134ab9
Escape slashes in TreeView paths. r=Honza

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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