Last Comment Bug 716793 - Dispatch synthetic mousemove off the refresh driver
: Dispatch synthetic mousemove off the refresh driver
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Mac OS X
: P1 normal (vote)
: mozilla12
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on: 1026621 762830
Blocks: 661099 678691 716549
  Show dependency treegraph
 
Reported: 2012-01-09 21:13 PST by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2014-06-29 08:54 PDT (History)
7 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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 (2.84 KB, patch)
2012-01-09 21:15 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
roc: review+
Details | Diff | Review
With test fixes (15.53 KB, patch)
2012-01-11 16:29 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
With those changes (16.52 KB, patch)
2012-01-12 11:31 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
roc: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-09 21:13:09 PST
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.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-09 21:15:14 PST
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
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-09 21:31:58 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a35bcdd7c1e
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-09 23:01:20 PST
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....
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-09 23:08:44 PST
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!
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-11 10:35:57 PST
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....
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-11 11:08:07 PST
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.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-11 15:39:16 PST
sounds good
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-11 15:39:55 PST
or make the button pointer-events:none, that's probably easier and less fragile
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-11 16:25:03 PST
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.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-11 16:29:22 PST
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
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-11 17:27:42 PST
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?
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-11 17:52:19 PST
> 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.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-11 18:14:32 PST
(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.
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-11 18:26:13 PST
> 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.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-11 18:34:41 PST
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.
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-11 18:51:26 PST
Yeah, agreed.  That's why I'll fix things up.  Just not tonight.  ;)
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-12 11:31:16 PST
Created attachment 588126 [details] [diff] [review]
With those changes
Comment 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-12 14:02:55 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/899a12aeff6c

Note You need to log in before you can comment on or make changes to this bug.