Dispatch synthetic mousemove off the refresh driver

RESOLVED FIXED in mozilla12

Status

()

Core
Layout
P1
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla12
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
In particular, we want to do this after we've done the normal refresh driver layout flushing (because mousemove flushes layout), so do it as a Flush_Display observer.

This fixes the Tdhtml regression from bug 716549.
(Assignee)

Comment 1

6 years ago
Created attachment 587241 [details] [diff] [review]
Dispatch synthetic mousemove off the refresh driver, not as fast as we can.   use Flush_Display here because mousemoves flush out layout, so we want to do the synthetic one after we've done our normal layout flushing
Attachment #587241 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Priority: -- → P1
Attachment #587241 - Flags: review?(roc) → review+
(Assignee)

Comment 2

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a35bcdd7c1e
Flags: in-testsuite-
Target Milestone: --- → mozilla12
(Assignee)

Comment 3

6 years ago
And backed out.  Some of our tests apparently actually rely on this event firing "very soon", and others are randomly failing for some reason with antialiasing differences with this patch....
Target Milestone: mozilla12 → ---
(Assignee)

Comment 4

6 years ago
Oh, and the "very soon" tests are of course known randomorange, because of course sometimes it doesn't fire soon even without the change in this bug!
(Assignee)

Comment 5

6 years ago
OK.  So comment 4 was only half right.  One of the issues that clearly need changes in the tests was a randomorange; the other was not.  I have both of those fixed now.

The remaining issue are the new random oranges on layout/reftests/bugs/478811-*.html.  For some reason, this patch causes antialiasing differences on the edges of the green rectangles....
(Assignee)

Updated

6 years ago
Blocks: 678691
(Assignee)

Updated

6 years ago
Blocks: 661099
(Assignee)

Comment 6

6 years ago
The remaining fails seem to deal with testcases that have a div inside a large button; it's possible that a difference in mouseover timing changes how many times we repaint that button or something.

I'm going to try adding a big div covering the whole rendering area to those testcases, in the hope of keeping the mouse away from that button, I guess.
sounds good
or make the button pointer-events:none, that's probably easier and less fragile
(Assignee)

Comment 9

6 years ago
Hmm.  I have a green try run with the other, actually, but I'll try the pointer-events thing.  I agree it's less fragile.
(Assignee)

Comment 10

6 years ago
Created attachment 587871 [details] [diff] [review]
With test fixes

This is the same C++ as I landed originally; all the new stuff is in the tests
Attachment #587871 - Flags: review?(roc)
Comment on attachment 587871 [details] [diff] [review]
With test fixes

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

::: widget/tests/native_mouse_mac_window.xul
@@ +166,5 @@
> +        }
> +      } else {
> +        timeoutFunc = callbackFunc;
> +      }
> +      gAfterLoopExecution = setTimeout(timeoutFunc, 0);

We're expecting the event to fire here, right? Why do we even need a timeout at all? Why not just let the test hang if the test fails due to the event not being delivered?

@@ +369,5 @@
> +                       function () {
> +                         left.mozRequestAnimationFrame(function() {
> +                           SimpleTest.executeSoon(callback);
> +                         });
> +                       });

Same here...

::: widget/tests/test_bug596600.xul
@@ +119,5 @@
> +         SimpleTest.executeSoon(callback);
> +       });
> +     }, winToFocus);
> +  }
> +

Why not just wait for a mouse event here?
(Assignee)

Comment 12

6 years ago
> We're expecting the event to fire here, right? Why do we even need a timeout at all? Why
> not just let the test hang if the test fails due to the event not being delivered?

The test seems to go to some pains to abort early on random unexpected events.  I was more or less just preserving that, but I could just skip the setTimeout part.

> Same here...

Not sure what you mean.  Every part of the quoted code is needed, I believe.

> Why not just wait for a mouse event here?

We could, but it'd actually take bigger changes to the test, I think.  Sometimes it needs to be a mouseover and sometimes a mouseout.  I might be able to get away with watching for mousemove, but I'd still need to remove event listeners and such...  I can do that if you prefer, of course.
(In reply to Boris Zbarsky (:bz) from comment #12)
> The test seems to go to some pains to abort early on random unexpected
> events.  I was more or less just preserving that, but I could just skip the
> setTimeout part.

Right, it shouldn't be a problem to remove the setTimeout but still check for unexpected events and continue the test when all expected events have been received.

> > Same here...
> 
> Not sure what you mean.  Every part of the quoted code is needed, I believe.

Instead of doing executeSoon or RAF, why not just wait for the mouseover to fire before continuing?

> > Why not just wait for a mouse event here?
> 
> We could, but it'd actually take bigger changes to the test, I think. 
> Sometimes it needs to be a mouseover and sometimes a mouseout.  I might be
> able to get away with watching for mousemove, but I'd still need to remove
> event listeners and such...  I can do that if you prefer, of course.

Maybe all this is too much work, but it does seem to me that the most logical way to have a reliable test here is to listen for the particular DOM events we're expecting and keep going only after they've been received. We don't need any extra constraints on their timing.
(Assignee)

Comment 14

6 years ago
> Instead of doing executeSoon or RAF, why not just wait for the mouseover to fire before
> continuing?

I can do that, I guess.  It'll take a bit more surgery on the test.  Will do it tomorrow.
Maybe it doesn't matter too much, but with your patch we still depend quite a bit on the exact timing of the synthetic event, which we still might want to change later.
(Assignee)

Comment 16

6 years ago
Yeah, agreed.  That's why I'll fix things up.  Just not tonight.  ;)
(Assignee)

Comment 17

6 years ago
Created attachment 588126 [details] [diff] [review]
With those changes
Attachment #588126 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Attachment #587871 - Attachment is obsolete: true
Attachment #587871 - Flags: review?(roc)
Attachment #588126 - Flags: review?(roc) → review+
(Assignee)

Comment 18

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/899a12aeff6c
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/899a12aeff6c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 762830
Depends on: 1026621
You need to log in before you can comment on or make changes to this bug.