Closed Bug 1241070 Opened 4 years ago Closed 4 years ago

Dominator tree nodes cannot be toggled more than once

Categories

(DevTools :: Memory, defect, P2)

defect

Tracking

(firefox45 unaffected, firefox46+ verified, firefox47+ verified)

VERIFIED FIXED
Firefox 47
Tracking Status
firefox45 --- unaffected
firefox46 + verified
firefox47 + verified

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch bug1241070.v1.patch (obsolete) — Splinter Review
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.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Attachment #8709921 - Flags: review?(nfitzgerald)
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.
Attachment #8709921 - Flags: review?(nfitzgerald)
Has STR: --- → yes
Priority: -- → P2
(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 ?
Flags: needinfo?(nfitzgerald)
Yeah, we can keep both.
Flags: needinfo?(nfitzgerald)
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
Attachment #8709921 - Attachment is obsolete: true
Attachment #8710149 - Flags: review?(nfitzgerald)
Comment on attachment 8710149 [details] [diff] [review]
bug1241070.v2.patch (added component test)

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

Thanks!
Attachment #8710149 - Flags: review?(nfitzgerald) → review+
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.
Duplicate of this bug: 1243076
Adding checkin needed, all failures on try are already investigated via Bug 1204174.
Keywords: checkin-needed
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
Flags: needinfo?(jdescottes)
Keywords: checkin-needed
Attached patch bug1241070.v3.patch (obsolete) — Splinter Review
Just rebased on top of fx-team, this patch should apply without issue.
Attachment #8710149 - Attachment is obsolete: true
Flags: needinfo?(jdescottes)
Attachment #8712728 - Flags: review+
Keywords: checkin-needed
Wrong patch uploaded!
Attachment #8712728 - Attachment is obsolete: true
Attachment #8713208 - Flags: review+
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?
Flags: needinfo?(jdescottes)
https://hg.mozilla.org/mozilla-central/rev/79a04ee0ab30
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
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
Flags: needinfo?(jdescottes)
Attachment #8713208 - Flags: approval-mozilla-aurora?
Comment on attachment 8713208 [details] [diff] [review]
bug1241070.v4.patch

Fixes a recent regression, includes tests. Please uplift to aurora.
Attachment #8713208 - Flags: approval-mozilla-aurora? → approval-mozilla-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
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20160316]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.