Closed
Bug 1240344
Opened 7 years ago
Closed 7 years ago
[DevTools][Memory] Filter word is gone after change to "Dominators" in select-view
Categories
(DevTools :: Memory, defect, P3)
DevTools
Memory
Tracking
(firefox46 fixed)
RESOLVED
FIXED
Firefox 46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: magicp.jp, Assigned: jdescottes)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
314.41 KB,
video/mp4
|
Details | |
8.23 KB,
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
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.
Has STR: --- → yes
status-firefox46:
--- → affected
Component: Untriaged → Developer Tools: Memory
OS: Unspecified → All
Hardware: Unspecified → All
Updated•7 years ago
|
Blocks: memory-frontend
Priority: -- → P3
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jdescottes
Assignee | ||
Comment 1•7 years ago
|
||
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 | ||
Comment 2•7 years ago
|
||
try https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0c8a1e93688
Assignee | ||
Updated•7 years ago
|
Attachment #8710960 -
Flags: review?(nfitzgerald)
Comment 3•7 years ago
|
||
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•7 years ago
|
||
Attachment #8710960 -
Attachment is obsolete: true
Attachment #8711050 -
Flags: review+
Assignee | ||
Comment 5•7 years ago
|
||
Forgot one of the review changes.
Attachment #8711050 -
Attachment is obsolete: true
Attachment #8711057 -
Flags: review+
Assignee | ||
Comment 6•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c09a55daebd3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Comment 9•7 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•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•