1145784 - Graph mouse movement doesn't work when entering another graph

Status: RESOLVED FIXED

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

Description

[Brian Grinstead [:bgrins]]

Going to handle this separately from Bug 1066504, since it's proven to be a different issue.

There's some code to sync the selections.  https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/Graphs.jsm?from=graphs.jsm#2079, but it doesn't work, because there is no selection event firing on graph2 during the move (since it's not currently selecting).

I think the right solution is to bind mousemove / mouseup to the top level window instead of the frame window instead.

Comment 1

[Brian Grinstead [:bgrins]]

I'll take a look at this

Comment 2

[Victor Porof [:vporof][:vp]]

mvp brian

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: timeline-range-across-graphs.patch

v1.  Playing with this, I think it fixes all the nits I have with the mouse selection on the main graph currently.  It will allow you to continue mouse movement across the entire browser (including other graphs), and reliably handle mouseup outside of the toolbox / browser window.

I'm not sure how you want to handle tests.  The tests pass locally with the patch applied because right now I am accepting an undefined `screenX` as an indication that we should just use clientX (which is what the tests pass in).  We could alternatively actually simulate the mouse events on relevant DOM nodes so the other properties are available.

Comment 4

[Victor Porof [:vporof][:vp]]

Comment on attachment 8600124 [details] [diff] [review]
timeline-range-across-graphs.patch

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

Looks good and feels good!

The only issue is that the cursors can become misaligned between graphs. If mousing over each graph, the | doesn't stay in the same place or get hidden as before.

Comment 5

[Victor Porof [:vporof][:vp]]

Attachment: Screen Shot 2015-05-01 at 3.42.55 PM.png

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: timeline-range-across-graphs-1.patch

Restores the disappearing cursor behavior.  Test patch incoming

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: timeline-range-across-graphs-2.patch

Renames clientX -> testX and clientY -> testY throughout the tests.  Also adds a new test that doesn't use the shortcut of calling event callbacks directly and instead uses dispatchEvent with a MouseEvent, setting screenX and screenY to make sure this part doesn't regress.

Comment 8

[Brian Grinstead [:bgrins]]

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

Comment 9

[Victor Porof [:vporof][:vp]]

Comment on attachment 8601189 [details] [diff] [review]
timeline-range-across-graphs-1.patch

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

::: browser/devtools/shared/widgets/Graphs.jsm
@@ +945,5 @@
>     */
> +  _getRelativeEventCoordinates: function(e) {
> +    // For ease of testing, testX and testY can be passed in as the event
> +    // object.  If so, just return this.
> +    if (e.testX !== undefined) {

Nit: check for both testX and testY and via `"foo" in bar` to avoid calling getters (unlikely to ever be an issue, but not a bad idea in general).

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: timeline-range-across-graphs-1.patch

Folded the new test for this into part 1.  Going to upload the change from clientX/clientY to testX/testY in part 2.

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: timeline-range-across-graphs-2.patch

clientX -> testX, clientY -> testY.  Also updates FlameGraph.js to accept these properties just to remove any weirdness of the two graph types accepting different parameters in tests.

Comment 12

[Brian Grinstead [:bgrins]]

Pending try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f1ad7599e4b

Comment 13

[Brian Grinstead [:bgrins]]

Try push looks good

Comment 14

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/abc82c178273
remote:   https://hg.mozilla.org/integration/fx-team/rev/164c7af2cc5d

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/abc82c178273
https://hg.mozilla.org/mozilla-central/rev/164c7af2cc5d

Comment 16

[Victor Porof [:vporof][:vp]]

\o/