723378 - We can trigger layout extra times off synth mouse events

Status: NEW

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We now run synth mouse events off the refresh driver.  However we run them off the refresh driver of the _root_ prescontext; in desktop Firefox that means the toplevel XUL window.

So now say you have a tab with changes going on inside that can trigger reflows.  We'll reflow off the tab's refresh driver, and post a synth mouse event to the root refresh driver.  When that fires, we'll end up dispatching the synth mouse event, which will trigger a layout flush on the target presshell (probably the very tab we started with).

In the worst case, the chrome and tab refresh drivers are exactly out of phase, so we end up reflowing the tab every 8ms, not every 16ms.

What would be ideal, I think, is if we only triggered a synth mouse event off the refresh driver of whatever presshell's reflow caused us to want a synth mouse event in the first place.  roc had some concerns from just hanging synth mouse move events off every presshell, though...

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess it's OK to trigger the synth mouse move off the particular refresh driver, as long as the mouse move state is only stored on the root presshell.

Comment 2

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> I guess it's OK to trigger the synth mouse move off the particular refresh
> driver, as long as the mouse move state is only stored on the root presshell.

The downside being duplicated effort and storage?

If we did track it on each presshell we could avoid the synth mouse move altogether when the presshell that generates it doesn't have the mouse over it.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The downside being confusion about which synthentic mouse move position is the correct one.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: patch

I thought about getting more synth mouse moves overall in the case of more than one presshell generating synth mouse moves at the same time, but then you could have a pending synth mouse move in a background tab with refresh ticks at 1Hz blocking synth mouse moves of foreground tabs, so I didn't pursue that.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 593719 [details] [diff] [review]
patch

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

Nice.

::: layout/base/nsPresShell.cpp
@@ +5244,5 @@
>    }
>  
> +  PresShell* rootPresShell = GetRootPresShell();
> +  if (!rootPresShell || rootPresShell->mMouseLocation ==
> +                          nsPoint(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE)) {

fix indent?

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> fix indent?

I changed it to:

  if (!rootPresShell ||
      rootPresShell->mMouseLocation == nsPoint(NS_UNCONSTRAINEDSIZE,
                                               NS_UNCONSTRAINEDSIZE)) {

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tn) from comment #4)
> ... but then
> you could have a pending synth mouse move in a background tab with refresh
> ticks at 1Hz blocking synth mouse moves of foreground tabs...

Background tabs can't post synth mouse moves, so that isn't true. But we can revisit this if needed.

Comment 8

[Timothy Nikkel (:tnikkel)]

We get a handful more assertions on a few crashtest and reftests because we do get more synth mouse moves. We can probably just bump the counts for those.

We also fail test_hover.html, that will need to be investigated.

Comment 9

[Boris Zbarsky [:bzbarsky]]

We can skip posting synth mouse moves from background tabs altogether, no?

Comment 10

[Timothy Nikkel (:tnikkel)]

Yes, we do already, I corrected myself in a later comment.