1261048 - Selected area in timeline goes insane when I toggle Devtools docking mode

Status: VERIFIED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect, P1)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 48, 32bit, ID 20160330030326
STR:
0. Open devtools, dock it to the right side of window, dock it to the bottom side of window, close devtools.
1. Open Devtools -> Performance
2. Press button "Start Recording Performance". Press it again to end recording.
3. Click  "JS Flame Chart"  tab to switch to it.
4. Press Ctrl+Shift+D
5. Click  "Waterfall"  tab to switch to it.
6. Press Ctrl+Shift+D
7. Click  "JS Flame Chart"  tab to switch to it.

Result:        After Step 7 selected area in timeline quickly collapses at the end of timeline
Expectations:  Selected area in timeline should stay still

Comment 1

[arni2033]

This is regression from bug 1120623. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=e417e32d9cb698fe0b2e376d9cdeac0c2cffb43f&tochange=7a0ebe3fccacc655c38e7f32948626af4d3d3269

Comment 2

[Greg Tatum [:gregtatum]]

Woo! That one is fun.

Comment 3

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Platform triage decision: This is not something that we will block Fx47 release on.

Comment 4

[Jet Villegas (inactive)]

[Triage] Patrick: can you comment on severity here? Thx.

Comment 5

[Patrick Brosset <:pbro>]

I'll let Greg comment on this one as he knows more about the performance tool (or who to talk to about it), than me.

Comment 6

[Greg Tatum [:gregtatum]]

This only happens when you hit the cmd+d shortcut on that specific view, so the severity seems medium to me. Although I believe it could be a quick fix since you just need to account for the modifiers being used on the shortcuts. It might be worth tackling.

Comment 7

[Greg Tatum [:gregtatum]]

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

Comment 8

[Greg Tatum [:gregtatum]]

Attachment: Fix shortcut modifier bug with Flame Graph.

I made the Flame Graph ignore keys with any modifiers and it cleared up the issue.

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8771980 [details] [diff] [review]
Fix shortcut modifier bug with Flame Graph.

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

LGTM!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/46d964768cf6
Fix shortcut modifier bug with Flame Graph. r=jsantell

Comment 11

[Patrick Brosset <:pbro>]

Looks like a simple enough change that could be uplifted to aurora and beta in time for the 48 release.
Once this lands on m-c, could you please request uplift approvals?

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46d964768cf6

Comment 13

[Greg Tatum [:gregtatum]]

Comment on attachment 8771980 [details] [diff] [review]
Fix shortcut modifier bug with Flame Graph.

Approval Request Comment
[Feature/regressing bug #]: 1261048
[User impact if declined]: The performance tool can break and the graph will begin to act erratic on hitting certain shortcut combinations.

[Describe test coverage new/current, TreeHerder]: The flame graph has mochitest coverage for the individual features of the graph here: https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools%2Fclient%2Fshared%2Ftest+path%3Aflame&redirect=false and overall integration coverage here: https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools%2Fclient%2Fperformance+path%3Aflame&redirect=false

[Risks and why]: This is low risk as it adds a small modifier button check to a handler.

[String/UUID change made/needed]: None.

Comment 14

[Gerry Chang [:gchang]]

Comment on attachment 8771980 [details] [diff] [review]
Fix shortcut modifier bug with Flame Graph.

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

We'll have 48 beta 10 to mitigate the potential regression. This patch fixes a shortcut regression with Flame Graph. Let's take it in 48 beta 10 and aurora. This also needs to be verified.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/84020ba5861c

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/07eb639d4583

Comment 17

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce this issue on Firefox 48.0a1 (2016-03-30) and on Windows 10 x64, using STR from Comment 0.
The issue is no longer reproducible on Firefox 48.0b10, Firefox 49.0a2 (2016-07-22), Firefox 50.0a1 (2016-07-21).
The tests were performed on Windows 10 x64, Ubuntu 16.04 x64 and on Mac OS X 10.11.1