1240344 - [DevTools][Memory] Filter word is gone after change to "Dominators" in select-view

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P3)

Description

[magicp]

Attachment: devtools-memory-filter.mp4

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160116030240

Steps to reproduce:

Please refer to attached video.
1. Start latest Nightly
2. Open Developer Tools and enable Memory tool
3. Select Memory tab
4. Run "Take snapshot"
5. Input filter word (e.g. other)
6. Change view from "Aggregate" to "Dominators"
7. Change view from "Dominators" to "Aggregate"


Actual results:

Filter word is gone after change to "Dominators" in select-view. But filter is still enable.


Expected results:

Filter word is not gone after change to "Dominators" in select-view.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: bug1240344.v1.patch

Bug 1240344 - fix filter propname used when creating toolbar;r=fitzgen

This commit fixes the prop name used when creating the Toolbar component
from app.js : Toolbar expects fitlerString, app was providing filter.

Extended browser_memory_filter_01.js to check the filter value is preserved
after switching views.

Comment 2

[Julian Descottes [:jdescottes]]

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

Comment 3

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

Comment on attachment 8710960 [details] [diff] [review]
bug1240344.v1.patch

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

Thanks, Julian!

::: devtools/client/memory/app.js
@@ +108,5 @@
>            allocations,
>            inverted,
>            onToggleInverted: () =>
>              dispatch(toggleInvertedAndRefresh(heapWorker)),
> +          filterString: filter,

D'oh!

::: devtools/client/memory/test/browser/browser_memory_filter_01.js
@@ +24,5 @@
> +            });
> +  };
> +  info(`Waiting for dominator trees to be of state: ${expected}`);
> +  return waitUntilState(store, predicate);
> +}

May be worth throwing this into head.js to deter copying or reimplementing this function next time we need this. If you do, please add a short comment describing what it does and @params.

@@ +59,5 @@
> +    "and filter input contains the user value");
> +
> +  ok(true, "check that the filter is preserved after switching view");
> +
> +  dispatch(changeViewAndRefresh(viewState.DOMINATOR_TREE, heapWorker));

Nit: add a comment above this line something like:

// Now switch to a different view, switch back, and assert that the filter
// word is still in place.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: bug1240344.v2.patch

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: bug1240344.v2.patch

Forgot one of the review changes.

Comment 6

[Julian Descottes [:jdescottes]]

Thanks for the review!
> 
> May be worth throwing this into head.js to deter copying or reimplementing
> this function next time we need this. If you do, please add a short comment
> describing what it does and @params.
> 

Done! 
For some reason I thought we had many other occurrences of this in the testsuite and wanted to move it to head.js + use it in other tests in a separate bug. But browser_memory_dominator_trees_01.js is the only other test where we use it. So it's in head.js and it is also used by browser_memory_dominator_trees_01.js

> 
> Nit: add a comment above this line something like:
> 
> // Now switch to a different view, switch back, and assert that the filter
> // word is still in place.

Done!

Comment 7

[Pulsebot]

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

Comment 8

[Phil Ringnalda (:philor)]

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

Comment 9

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments: Test successful

STR: clear.


Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Yes

Actual Results: 
As expected