STRs : - open memory tool - take snapshot - switch to dominators view => root node is expanded, but the arrow doesn't have the appropriate style - using the mouse, click on the root node arrow => arrow style is now correct, but root node is still expanded - click again => root node is collapsed, but arrow style is not updated etc ... The dominator tree item is not refreshed when its expanded state changes.

Bug 1241070 - refresh dominatortreeitem if expand state changed;r=fitzgen In DominatorTreeItem check props.expanded in shouldComponentUpdate. Added integration/sanity test to check that a node can be collapsed and expanded using mouse events.

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

Comment on attachment 8709921 [details] [diff] [review] bug1241070.v1.patch Review of attachment 8709921 [details] [diff] [review]: ----------------------------------------------------------------- The patch itself looks great, but I'd prefer to write component tests for this rather than an integration test. Please try that out, and if you can't write an effective component test, then we can land this version instead. Thanks! ::: devtools/client/memory/test/browser/browser_memory_dominator_trees_02.js @@ +1,4 @@ > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +// Integration test for mouse interaction in the dominator tree Did you try and write a test for the component directly? Were you unable to write one that failed before the shouldComponentUpdate changes and passed afterwards? I would really prefer to test this at the component level directly, rather than the whole tool's integration level. However, if you can't make the test fail without the shouldComponentUpdate changes then it won't be a very effective regression test.

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #3) > Did you try and write a test for the component directly? Were you unable to > write one that failed before the shouldComponentUpdate changes and passed > afterwards? Thanks for the feedback! Actually I had a working component test, but I felt that an integration test was better suited to this issue since it involves both DominatorTree and the DominatorTreeItem. Could we keep both? Any reason not too add integration tests here ?

Yeah, we can keep both.

Reminder: the integration test might need to be disabled on DEBUG builds :( -- do a try push to find out.

The try push looks fine on debug builds : https://treeherder.mozilla.org/#/jobs?repo=try&revision=701842ba777c (win8 seems stuck, but that's unrelated)

new try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd14df4175d2

Comment on attachment 8710149 [details] [diff] [review] bug1241070.v2.patch (added component test) Review of attachment 8710149 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Looks like my latest try push triggers frequent failures of the performance tests on Linux 32bits opt in dt5 e10s. Just rebased and did a new try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=30556e32c5e8

This commit leads to a permafail of devtools/client/performance/test/browser_perf-telemetry.js on Linux 32 opt e10s. Relaunched one last try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=00f77b82ed3c . If it fails again, I'll investigate the issue locally.

Applied the patch from jsantell in Bug 1204174 . Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1009e1408fc

Adding checkin needed, all failures on try are already investigated via Bug 1204174.

failed to apply: renamed 1241070 -> bug1241070.v2.patch applying bug1241070.v2.patch patching file devtools/client/memory/test/browser/browser.ini Hunk #1 succeeded at 9 with fuzz 2 (offset 0 lines). patching file devtools/client/memory/test/chrome/chrome.ini Hunk #1 FAILED at 0 1 out of 1 hunks FAILED -- saving rejects to file devtools/client/memory/test/chrome/chrome.ini.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh bug1241070.v2.patch

Just rebased on top of fx-team, this patch should apply without issue.

Wrong patch uploaded!

Julian, can you make sure to request uplift to 46 for this bug, once it has landed in m-c and baked for a day or so?

Comment on attachment 8713208 [details] [diff] [review] bug1241070.v4.patch Approval Request Comment [Feature/regressing bug #]: Fix for the feature developed in Bug 1229229 [User impact if declined]: Tree nodes in the new Dominator view of devtools Memory Tool cannot be toggled properly. [Describe test coverage new/current, TreeHerder]: Tests included in the patch [Risks and why]: Low risk, only impacts devtools users using the Memory Tool module. [String/UUID change made/needed]: N/A

Comment on attachment 8713208 [details] [diff] [review] bug1241070.v4.patch Fixes a recent regression, includes tests. Please uplift to aurora.

I have successfully reproduce this bug on firefox nightly 46.0a1 (2016-01-20) with windows 7(32 bit) Mozilla/5.0 (Windows NT 6.1; rv:46.0) Gecko/20100101 Firefox/46.0 I found this fix on latest beta 46.0b2 Mozilla/5.0 (Windows NT 6.1; rv:46.0) Gecko/20100101 Firefox/46.0 Build ID : 20160314144540 And I also found this fix on latest aurora 47.0a2 (2016-03-16) Mozilla/5.0 (Windows NT 6.1; rv:47.0) Gecko/20100101 Firefox/47.0 Build ID :20160316004053 [bugday-20160316]

(In reply to Md. Rahimul Islam from comment #24) > I have successfully reproduce this bug on firefox nightly 46.0a1 (2016-01-20) > with windows 7(32 bit) > Mozilla/5.0 (Windows NT 6.1; rv:46.0) Gecko/20100101 Firefox/46.0 > > I found this fix on latest beta 46.0b2 > > Mozilla/5.0 (Windows NT 6.1; rv:46.0) Gecko/20100101 Firefox/46.0 > Build ID : 20160314144540 > > And I also found this fix on latest aurora 47.0a2 (2016-03-16) > Mozilla/5.0 (Windows NT 6.1; rv:47.0) Gecko/20100101 Firefox/47.0 > Build ID :20160316004053 > > [bugday-20160316] I am changing the status of the bug as it was verified by Rahimul