Boris Zbarsky [:bzbarsky] Assignee
	Description • 13 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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a35bcdd7c1e

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 13 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....

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 13 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!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 13 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....

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 13 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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 7 • 13 years ago

sounds good

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 13 years ago

or make the button pointer-events:none, that's probably easier and less fragile

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 13 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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 13 years ago

This is the same C++ as I landed originally; all the new stuff is in the tests

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 11 • 13 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 13 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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 13 • 13 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 13 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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 15 • 13 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 13 years ago

Yeah, agreed.  That's why I'll fix things up.  Just not tonight.  ;)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 18 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/899a12aeff6c

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 19 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/899a12aeff6c