We can trigger layout extra times off synth mouse events

NEW
Assigned to

Status

()

Core
Layout
6 years ago
6 years ago

People

(Reporter: bz, Assigned: tnikkel)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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...
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.
(Assignee)

Comment 2

6 years ago
(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.
The downside being confusion about which synthentic mouse move position is the correct one.
(Assignee)

Comment 4

6 years ago
Created attachment 593719 [details] [diff] [review]
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.
Assignee: nobody → tnikkel
Attachment #593719 - Flags: review?(roc)
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?
Attachment #593719 - Flags: review?(roc) → review+
(Assignee)

Comment 6

6 years ago
(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)) {
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
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.
We can skip posting synth mouse moves from background tabs altogether, no?
(Assignee)

Comment 10

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

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
You need to log in before you can comment on or make changes to this bug.