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

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P1)

Description

[Julian Descottes [:jdescottes]]

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.

Comment 1

[Julian Descottes [:jdescottes]]

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.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: 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.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: 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.

Comment 4

[Julian Descottes [:jdescottes]]

Redux version update is filed in Bug 1246650

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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?

Comment 7

[Julian Descottes [:jdescottes]]

(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 :)

Comment 8

[Julian Descottes [:jdescottes]]

(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.

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: 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.

Comment 10

[Julian Descottes [:jdescottes]]

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 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: bug1246570.v3.patch

Updated test comments, removed generous timeout.

Carry over r+.

Comment 13

[Julian Descottes [:jdescottes]]

Bug 1246650 landed on fx-team, all needed dependencies should now be available.
Can you push attachment 8717117 [details] [diff] [review] ?

Comment 14

[Pulsebot]

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

Comment 15

[Pulsebot]

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

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 17

[Julian Descottes [: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.

Comment 18

[Julian Descottes [:jdescottes]]

Attachment: 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

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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!

Comment 20

[Julian Descottes [:jdescottes]]

Attachment: 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+)

Comment 21

[Julian Descottes [:jdescottes]]

New try looks good as well. 
Can you push attachment 8718007 [details] [diff] [review] ?

Comment 22

[Pulsebot]

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

Comment 23

[Carsten Book [:Tomcat]]

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