Keyboard : left arrow no longer selects parent node in memory tree table

RESOLVED FIXED in Firefox 47

Status

()

Firefox
Developer Tools: Memory
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

47 Branch
Firefox 47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

STRs:
- open memory tool
- take snapshot
- expand the node "objects"
- select a childnode of "objects"
- press LEFT

Actual results:
Highlighting disappears, no row is selected anymore

Expected results:
The parent node "objects" should be selected.
In Bug 1246017, the parentMap was changed from a {nodeId -> node} map to a {nodeId -> nodeId} map.

=> Now for the census and dominator views, getParent returns a key instead of a node. 

I think the best way to fix this here is to change getParent to getParentKey, and let the tree component perform the "find by key", since it already has the logic to perform traversals.
Assignee: nobody → jdescottes
Created attachment 8716927 [details] [diff] [review]
bug1246570.part1.v1.patch

This commit replaces the getParent interface expected by the Tree component with a getParentKey interface, easier to use with the new parentMap.
Attachment #8716927 - Flags: review?(nfitzgerald)
Created attachment 8716930 [details] [diff] [review]
bug1246570.part2.v1.patch

This is the integration test for the previous commit. I moved it to a separate commit, because as you can see, it needs a newer Redux version (need the bug fix from https://github.com/rackt/redux/commit/b7031ce3acb23b6ecadbd977b1cfa32486447904). The "hack" I included in this patch is just here for review purposes. I will remove it from the final commit.

Additional node about this test, I plan to reuse it for Bug 1243131.

I will file a separate bug to update the redux version.
Attachment #8716930 - Flags: review?(nfitzgerald)
Depends on: 1246650
Redux version update is filed in Bug 1246650
Comment on attachment 8716927 [details] [diff] [review]
bug1246570.part1.v1.patch

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

I don't think that making the Tree take getParentKey is the right approach: we shouldn't need to do a full tree traversal just to find a node's parent. If we were ok with walking the full tree we wouldn't need any kind of parent API, we could just ask for every node's children and figure out which one was the parent that way. Either way, it's an inefficiency we'd like to avoid.

Instead, we should be able to make createParentMap map from node id -> node again. I think that part of that commit was unnecessary, I shouldn't have left it in, and should be reverted.

I can do this, if you don't want to.
Attachment #8716927 - Flags: review?(nfitzgerald)
Comment on attachment 8716930 [details] [diff] [review]
bug1246570.part2.v1.patch

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

Thanks, this is a solid test.

::: devtools/client/shared/components/tree.js
@@ +396,5 @@
>      });
>    }),
>  
> +  _onKeyUp(e) {
> +    e.nativeEvent.preventDefault();

Why?

@@ +418,5 @@
>  
>      switch (e.key) {
>        case "ArrowUp":
>          this._focusPrevNode();
> +        e.nativeEvent.preventDefault();

This should be handled by the ViewHelpers.preventScrolling call just above. Is it not for some reason?
Attachment #8716930 - Flags: review?(nfitzgerald) → review+
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #6)
> Comment on attachment 8716930 [details] [diff] [review]
> bug1246570.part2.v1.patch
> 
> Review of attachment 8716930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, this is a solid test.
> 
> ::: devtools/client/shared/components/tree.js
> @@ +396,5 @@
> >      });
> >    }),
> >  
> > +  _onKeyUp(e) {
> > +    e.nativeEvent.preventDefault();
> 
> Why?
> 
> @@ +418,5 @@
> >  
> >      switch (e.key) {
> >        case "ArrowUp":
> >          this._focusPrevNode();
> > +        e.nativeEvent.preventDefault();
> 
> This should be handled by the ViewHelpers.preventScrolling call just above.
> Is it not for some reason?

Argh good catch! These are leftovers from my investigation on the redux issue I mentioned. I guess I got a bit tired at the end :)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #5)
> Comment on attachment 8716927 [details] [diff] [review]
> bug1246570.part1.v1.patch
> 
> Review of attachment 8716927 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think that making the Tree take getParentKey is the right approach:
> we shouldn't need to do a full tree traversal just to find a node's parent.
> If we were ok with walking the full tree we wouldn't need any kind of parent
> API, we could just ask for every node's children and figure out which one
> was the parent that way. Either way, it's an inefficiency we'd like to avoid.
> 
> Instead, we should be able to make createParentMap map from node id -> node
> again. I think that part of that commit was unnecessary, I shouldn't have
> left it in, and should be reverted.
> 
> I can do this, if you don't want to.

You're right, using the map is totally pointless in this case, sorry :/ 
I'll update createParentMap as you suggested.
Created attachment 8717025 [details] [diff] [review]
bug1246570.v2.patch

Thanks for the reviews! 
I merged both patches in a single one since the code change change was really minor.

We will just have to wait for the new react version before landing this.
Attachment #8716927 - Attachment is obsolete: true
Attachment #8716930 - Attachment is obsolete: true
Attachment #8717025 - Flags: review?(nfitzgerald)
s/react/redux

Try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=97b1cab13ecf (on top of a commit upgrading redux to 3.3.0)
Comment on attachment 8717025 [details] [diff] [review]
bug1246570.v2.patch

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

Awesome, thanks! \o/

::: devtools/client/memory/test/browser/browser.ini
@@ +13,5 @@
>  [browser_memory_diff_01.js]
>  [browser_memory_dominator_trees_01.js]
>  [browser_memory_dominator_trees_02.js]
>  [browser_memory_filter_01.js]
> +[browser_memory_keyboard.js]

/me crosses fingers and toes that this doesn't start leaking windows in DEBUG builds
Attachment #8717025 - Flags: review?(nfitzgerald) → review+
Has STR: --- → yes
Priority: -- → P1
Created attachment 8717117 [details] [diff] [review]
bug1246570.v3.patch

Updated test comments, removed generous timeout.

Carry over r+.
Attachment #8717025 - Attachment is obsolete: true
Attachment #8717117 - Flags: review+
Bug 1246650 landed on fx-team, all needed dependencies should now be available.
Can you push attachment 8717117 [details] [diff] [review] ?
Keywords: checkin-needed
Backed out for XPCshell failures:

Backout: https://hg.mozilla.org/integration/fx-team/rev/e9f20bd7a8a7

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=9fd143cd5996
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=7193074&repo=fx-team

03:23:06     INFO -  TEST-START | devtools/client/memory/test/unit/test_action_diffing_02.js
03:28:06  WARNING -  TEST-UNEXPECTED-TIMEOUT | devtools/client/memory/test/unit/test_action_diffing_02.js | Test timed out
Flags: needinfo?(jdescottes)
XPCShell tests timed out on : Linux opt, Linux pgo, Linux debug, Linux 64 opt, Linux 64 pgo, Linux 64 debug, OSX 10.6 opt, OSX 10.6 debug.

Locally I noticed the tests were got slower, but since they passed I didn't think it would actually timeout. And I obviously forgot to include them in my try push.
Status: NEW → ASSIGNED
Flags: needinfo?(jdescottes)
Created attachment 8717958 [details] [diff] [review]
bug1246570.backout.v1.patch

The XPCShell timeouts are caused because we stringify all messages exchanged by HeapAnalysesClient and HeapAnalysesWorker.
The parentMap is now created by the worker, and is also now a much bigger object, which takes too much time to stringify and stream to the console.

I also took the opportunity to swap dumpv.wantLogging to dumpv.wantVerbose. Reading DevtoolsUtils code, it looks like the flags to use to enable logging are either dumpn.wantLogging or dumpv.wantVerbose, but I couldn't find any other reference to dumpv.wantLogging in the code base so I assume it was a mistake.

This commit disables verbose logging for XPCShell tests. Let me know how you feel about this change.

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9d01c7846ab
Attachment #8717958 - Flags: feedback?(nfitzgerald)
Comment on attachment 8717958 [details] [diff] [review]
bug1246570.backout.v1.patch

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

D'oh, this is what I had originally intended -- good catch!
Attachment #8717958 - Flags: feedback?(nfitzgerald) → review+
Created attachment 8718007 [details] [diff] [review]
bug1246570.backout.v2.patch (merged fix for backout + initial fix)

Try looks good.

Restarted a try push for the merged patch : https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6ec797b55fa

(carry over r+)
Attachment #8717117 - Attachment is obsolete: true
Attachment #8717958 - Attachment is obsolete: true
Attachment #8718007 - Flags: review+
New try looks good as well. 
Can you push attachment 8718007 [details] [diff] [review] ?
Keywords: checkin-needed

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ac48acdd0a10
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.