Closed Bug 1370139 Opened 5 years ago Closed 5 years ago

TreeView path with slash hides another object

Categories

(DevTools :: JSON Viewer, enhancement, P2)

enhancement

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Attached patch tree-paths.patch (obsolete) — Splinter Review
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.
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Attachment #8874654 - Flags: review?(odvarko)
Component: Developer Tools: JSON Viewer → Developer Tools
Summary: Path with / prevents expanding object → TreeView path with slash hides another object
Attached patch tree-paths.patch (obsolete) — Splinter Review
Forgot to workaround typeof null==="object"
Attachment #8874654 - Attachment is obsolete: true
Attachment #8874654 - Flags: review?(odvarko)
Attachment #8874675 - Flags: review?(odvarko)
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
Attachment #8874675 - Flags: review?(odvarko) → review+
I'm not used to react. If there is some more appropriate place, I guess it can be moved in another bug.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa7a23344ca4
Escape slashes in TreeView paths. r=Honza
Keywords: checkin-needed
Priority: -- → P2
Component: Developer Tools → Developer Tools: JSON Viewer
(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
Using String(subKey).replace instead of subKey.replace

Should land after bug 1370443, which affects surrounding code.
Attachment #8874675 - Attachment is obsolete: true
Attachment #8879521 - Flags: review?(odvarko)
Comment on attachment 8879521 [details] [diff] [review]
tree-paths-v3.patch

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

Looks reasonable to me

R+

Honza
Attachment #8879521 - Flags: review?(odvarko) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cea239134ab9
Escape slashes in TreeView paths. r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cea239134ab9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.