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

RESOLVED FIXED in Firefox 46

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: magicp.jp, Assigned: jdescottes)

Tracking

(Blocks 1 bug)

Trunk
Firefox 46
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

3 years ago
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.
Reporter

Updated

3 years ago
Has STR: --- → yes
Component: Untriaged → Developer Tools: Memory
OS: Unspecified → All
Hardware: Unspecified → All
Priority: -- → P3
Assignee

Updated

3 years ago
Assignee: nobody → jdescottes
Assignee

Comment 1

3 years ago
Posted patch bug1240344.v1.patch (obsolete) — Splinter Review
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.
Assignee

Updated

3 years ago
Attachment #8710960 - Flags: review?(nfitzgerald)
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.
Attachment #8710960 - Flags: review?(nfitzgerald) → review+
Assignee

Comment 4

3 years ago
Posted patch bug1240344.v2.patch (obsolete) — Splinter Review
Attachment #8710960 - Attachment is obsolete: true
Attachment #8711050 - Flags: review+
Assignee

Comment 5

3 years ago
Forgot one of the review changes.
Attachment #8711050 - Attachment is obsolete: true
Attachment #8711057 - Flags: review+
Assignee

Comment 6

3 years ago
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!
Keywords: checkin-needed

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c09a55daebd3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46

Comment 9

3 years ago
[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

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.