Rework compositor touch hit testing

Rework compositor touch hit testing

Previously compositor thread hit-testing was done by sending a list of absolute rects to the compositor on layout (see www.chromium.org/developers/design-documents/compositor-hit-testing for details).  This is broken whenever the position of an element can change without causing layout (scrolling, transforms, etc.), and results in touch event handlers not getting invoked when they should.  To fix this we need to track the hit-test rects on a layer-by-layer basis, and this CL is the first big step in that direction.
 
I started with the per-layer approach LeviW@ first tried here: https://bugs.webkit.org/show_bug.cgi?id=103914, it was rejected from WebKit because it was too different from what iOS was doing, but is a perfectly reasonable approach for Blink.  I modified this original patch in a number of ways, including:
 - tracking rects per enclosing RenderLayer (instead of the enclosing composited layer), and then later projecting those rects onto the appropriate composited layer.
 - breaking up the rect accumulation into multiple methods to eliminate code duplication and simplify some things
 - disabling the mechanism when touch events aren't supported (doesn't affect devtools touch emulation) or when the page isn't composited
 - fixing a bunch of bugs (see new tests below)
 - short circuiting some common cases to make them faster
 - accumulate LayoutRects instead of just IntRects
 - exposing the per-layer details to the tests

This doesn't yet handle scrolling (or changing the transform) of non-composited layers, but that was already broken so this shouldn't make it any worse (and now handles the case when the layers are composited).  I'll work on this case in a follow-on CL - this CL is big enough already.

This change reworks the testing infrastructure to make it possible to test bugs with when hit test rect update happens.  Rather than freshly compute the hit test rects whenever the test asks for them, we instead register Internals to be notified by ScrollingCoordinator whenever the rects change. This allows us to verify that the rects are getting updated when they should, and also that we aren't getting unexpected updates. 

This change also adds several additional test cases, including: 
- touch handlers inside of scrolled divs (composited and non-composited)
- clipping behavior
- 2d (non-composited) transforms
- svg
- that changing style causes the rects to be updated
- global handlers on the html and body elements
- additional iframe test cases, including non-composited transforms of iframes

Also in order to make validating the test results easier, I've put a 'visualize' mode into the tests which will overlay the actual rects on top of the elements being tested. See http://goo.gl/ObBry for the current results (when running in content-shell interactively, which disables the use of the Ahem font to make the text readable). 

BUG=248522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154611

[Rick Byers, 2013-06-28 03:03:00]

Hey Levi,
I've finally got an initial crack at per-layer hit testing done, can you please
take a look?  This is my first major foray into working with the RenderTree so
I'm sure I've made some mistakes.  I've left some TODO comments where I want
your feedback (I'll resolve them one way or another before submitting this CL). 
There are a lot of corner cases to the rect tracking (damn the render tree can
be complicated!), and I expect I'm probably missing some, but I hope that
overall this is a lot better than what we have today, and I expect to iterate in
future CLs to improve it further.  Let me know if you can think of additional
interesting cases that I should test.

I haven't yet gone too far down the path of eliminating ScrollingCoordinator
from the picture here (as trchen suggested), this CL is already pretty big.  My
next big step will be to try to handle the non-composited scroll/transform
update case, and decoupling from ScrollingCoordinator may make sense there. 
Feel free to suggest possible cleanup for this or follow-on CLs.

Also this doesn't quite seem to be working on device yet, even though all the
layout tests are looking good.  I suspect an issue on the cc side with applying
the hit testing to each layer correctly.  I'm going to start looking into that
now, but I don't think it'll change this CL substantially so I think it's worth
starting the review now.

Thanks!
   Rick

[leviw_travelin_and_unemployed, 2013-07-10 01:51:16]

I think you've got a good start. We do need to think of a good solution for
software transforms, still...

https://codereview.chromium.org/17471008/diff/15001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/17471008/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:2340: // TODO: Any tighter guarantee on
when we don't need to descend into children?
I don't think there's any tighter guarantee, but good catch on the optimization
of using isRoot().

https://codereview.chromium.org/17471008/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:2344: // TODO: levi's original patch
excluded curr->isText(), curr->isListMarker() and !curr->isBox(). Why?
I can't recall on the latter two, but text nodes aren't event targets and should
be handled by their containing elements.

https://codereview.chromium.org/17471008/diff/15001/Source/core/rendering/Ren...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/17471008/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderView.cpp:355: // A RenderView doesn't really have
any hit testing of it's own.
Why is this needed?

[Rick Byers, 2013-07-10 14:53:12]

On 2013/07/10 01:51:16, Levi wrote:
> I think you've got a good start. We do need to think of a good solution for
> software transforms, still...

Thanks.  Yes, I'm currently attempting to record all the rects relative to their
immediate enclosing RenderLayer.  Then when I go to send them to the compositor
I can project each rect into the co-ordinate space of the enclosing composited
layer and take it's bounding box.  

This is also a step towards being able to update the rects during software
scrolling/animations without having to walk the render tree again (just have to
reproject any layers that have been repainted), but I'll probably do that second
step in a follow-on CL.

Sound like a good plan to you?

>
https://codereview.chromium.org/17471008/diff/15001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderObject.cpp (right):
> 
>
https://codereview.chromium.org/17471008/diff/15001/Source/core/rendering/Ren...
> Source/core/rendering/RenderObject.cpp:2340: // TODO: Any tighter guarantee on
> when we don't need to descend into children?
> I don't think there's any tighter guarantee, but good catch on the
optimization
> of using isRoot().

Ok, thanks.

>
https://codereview.chromium.org/17471008/diff/15001/Source/core/rendering/Ren...
> Source/core/rendering/RenderObject.cpp:2344: // TODO: levi's original patch
> excluded curr->isText(), curr->isListMarker() and !curr->isBox(). Why?
> I can't recall on the latter two, but text nodes aren't event targets and
should
> be handled by their containing elements.

Ok, thanks - updated.

>
https://codereview.chromium.org/17471008/diff/15001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderView.cpp (right):
> 
>
https://codereview.chromium.org/17471008/diff/15001/Source/core/rendering/Ren...
> Source/core/rendering/RenderView.cpp:355: // A RenderView doesn't really have
> any hit testing of it's own.
> Why is this needed?

Expanded the comment (basically it just avoids getting a redundant rect for
iframes).

[Rick Byers, 2013-07-16 19:40:46]

I've got non-compostied transforms (and even non-composited transformed iframes,
which was a real pain) working now.  I've updated the images at
http://goo.gl/ObBry so you can see what the test results look like (green
rectangles indicate the hit test rects).

Levi, I think this CL is pretty much ready to land.  Can you take a look and do
a full review now please?

Thanks,
   Rick

[Rick Byers, 2013-07-18 01:36:48]

On 2013/07/10 14:53:12, Rick Byers wrote:
> On 2013/07/10 01:51:16, Levi wrote:
>
https://codereview.chromium.org/17471008/diff/15001/Source/core/rendering/Ren...
> > Source/core/rendering/RenderView.cpp:355: // A RenderView doesn't really
have
> > any hit testing of it's own.
> > Why is this needed?
> 
> Expanded the comment (basically it just avoids getting a redundant rect for
> iframes).

After doing some more testing of real-world websites on a pixel, I found this
was actually wrong.  If the only children of a <body> are position: absolute,
then the body and html element will have 0 size.  So instead of using the size
of the root element and pruning the tree walk there, I instead make RenderView
report the size of the entire contents and prune the tree walk with it.

[leviw_travelin_and_unemployed, 2013-07-18 18:08:38]

https://codereview.chromium.org/17471008/diff/45001/Source/core/page/scrollin...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/page/scrollin...
Source/core/page/scrolling/ScrollingCoordinator.cpp:325: continue;
Does this happen? When?

https://codereview.chromium.org/17471008/diff/45001/Source/core/page/scrollin...
Source/core/page/scrolling/ScrollingCoordinator.cpp:520: // Examine every target
in detail.
This comment doesn't really help.

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.h:571: virtual void
computeOwnHitTestRects(Vector<IntRect>&, const LayoutPoint& layerOffset) const
OVERRIDE;
"Own" seems redundant to me.

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:592:
rects.append(pixelSnappedIntRect(layerOffset, size()));
This won't actually get pixel snapping right since you're not using the
locationOffset. You should use enclosingIntRect (we don't mind if this is a
pixel too big, but we don't want a pixel too small).

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:1103: // Only safe to call from
RenderLayerModelObject::destroyLayer(RenderArena*)
Did you mean to have this change?

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:2299: ASSERT(currentLayer);
You could also assert this is the enclosing layer.

[Rick Byers, 2013-07-18 19:58:51]

Thanks Levi.

https://codereview.chromium.org/17471008/diff/45001/Source/core/page/scrollin...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/page/scrollin...
Source/core/page/scrolling/ScrollingCoordinator.cpp:520: // Examine every target
in detail.
On 2013/07/18 18:08:39, Levi wrote:
> This comment doesn't really help.

Removed

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.h:571: virtual void
computeOwnHitTestRects(Vector<IntRect>&, const LayoutPoint& layerOffset) const
OVERRIDE;
On 2013/07/18 18:08:39, Levi wrote:
> "Own" seems redundant to me.

I wanted to clearly differentiate it from the public entrypoint
'computeLayerHitTestRects' which computes the rects for this node and all it's
children. computeHitTestRects seems ambiguous to me as to whether it's a single
node or a tree walk.

What about: one of these pairs of names?
computeHitTestsRectsForTree / computeHitTestsRectsForRenderer
or
computeTreeHitTestRects / computeSelfHitTestRects

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:592:
rects.append(pixelSnappedIntRect(layerOffset, size()));
On 2013/07/18 18:08:39, Levi wrote:
> This won't actually get pixel snapping right since you're not using the
> locationOffset. You should use enclosingIntRect (we don't mind if this is a
> pixel too big, but we don't want a pixel too small).

Done.  Also fixed RenderBlock::computeOwnHitTestRects.

On a related note, perhaps it would be simpler (and more precise) if I just
tracked LayoutRects here and converted to enclosing IntRect only just before
sending them to the compositor?

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:1103: // Only safe to call from
RenderLayerModelObject::destroyLayer(RenderArena*)
On 2013/07/18 18:08:39, Levi wrote:
> Did you mean to have this change?

Yeah, I stumbled across this incorrect comment when examining the lifetime
semantics of RenderLayer for the state I added to Internals.  Figured I'd just
correct it here.

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:2299: ASSERT(currentLayer);
On 2013/07/18 18:08:39, Levi wrote:
> You could also assert this is the enclosing layer.

Done.

[leviw_travelin_and_unemployed, 2013-07-18 20:21:48]

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.h:571: virtual void
computeOwnHitTestRects(Vector<IntRect>&, const LayoutPoint& layerOffset) const
OVERRIDE;
On 2013/07/18 19:58:52, Rick Byers wrote:
> On 2013/07/18 18:08:39, Levi wrote:
> > "Own" seems redundant to me.
> 
> I wanted to clearly differentiate it from the public entrypoint
> 'computeLayerHitTestRects' which computes the rects for this node and all it's
> children. computeHitTestRects seems ambiguous to me as to whether it's a
single
> node or a tree walk.
> 
> What about: one of these pairs of names?
> computeHitTestsRectsForTree / computeHitTestsRectsForRenderer
> or
> computeTreeHitTestRects / computeSelfHitTestRects

I like computeSelfHitTestRects

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:592:
rects.append(pixelSnappedIntRect(layerOffset, size()));
On 2013/07/18 19:58:52, Rick Byers wrote:
> On 2013/07/18 18:08:39, Levi wrote:
> > This won't actually get pixel snapping right since you're not using the
> > locationOffset. You should use enclosingIntRect (we don't mind if this is a
> > pixel too big, but we don't want a pixel too small).
> 
> Done.  Also fixed RenderBlock::computeOwnHitTestRects.
> 
> On a related note, perhaps it would be simpler (and more precise) if I just
> tracked LayoutRects here and converted to enclosing IntRect only just before
> sending them to the compositor?

Yes.

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:1103: // Only safe to call from
RenderLayerModelObject::destroyLayer(RenderArena*)
On 2013/07/18 19:58:52, Rick Byers wrote:
> On 2013/07/18 18:08:39, Levi wrote:
> > Did you mean to have this change?
> 
> Yeah, I stumbled across this incorrect comment when examining the lifetime
> semantics of RenderLayer for the state I added to Internals.  Figured I'd just
> correct it here.

Cool :)

[leviw_travelin_and_unemployed, 2013-07-18 20:23:03]

https://codereview.chromium.org/17471008/diff/56001/Source/core/page/scrollin...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/17471008/diff/56001/Source/core/page/scrollin...
Source/core/page/scrolling/ScrollingCoordinator.cpp:324: // This page may not
have compositing enabled at all.
So I get that, but then we should return, not continue, right?

[Rick Byers, 2013-07-18 20:55:49]

On 2013/07/18 20:23:03, Levi wrote:
>
https://codereview.chromium.org/17471008/diff/56001/Source/core/page/scrollin...
> File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):
> 
>
https://codereview.chromium.org/17471008/diff/56001/Source/core/page/scrollin...
> Source/core/page/scrolling/ScrollingCoordinator.cpp:324: // This page may not
> have compositing enabled at all.
> So I get that, but then we should return, not continue, right?

I wasn't sure if there were other cases where where some layers but not others
could have an enclosing composited layer.  Perhaps I should handle the
no-accelerated-compositing case earlier - short-circuiting the whole process (as
I do now for the touch-events-disabled case).  Then I can ASSERT here that we
always find a composited layer (but leave the code in place just in case)?

[leviw_travelin_and_unemployed, 2013-07-18 20:57:44]

On 2013/07/18 20:55:49, Rick Byers wrote:
> On 2013/07/18 20:23:03, Levi wrote:
> >
>
https://codereview.chromium.org/17471008/diff/56001/Source/core/page/scrollin...
> > File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):
> > 
> >
>
https://codereview.chromium.org/17471008/diff/56001/Source/core/page/scrollin...
> > Source/core/page/scrolling/ScrollingCoordinator.cpp:324: // This page may
not
> > have compositing enabled at all.
> > So I get that, but then we should return, not continue, right?
> 
> I wasn't sure if there were other cases where where some layers but not others
> could have an enclosing composited layer.  Perhaps I should handle the
> no-accelerated-compositing case earlier - short-circuiting the whole process
(as
> I do now for the touch-events-disabled case).  Then I can ASSERT here that we
> always find a composited layer (but leave the code in place just in case)?

I think that's the right behavior. It doesn't make sense to go through this
process w/o compositing, and if we have compositing, we should always find a
compositing layer.

[Rick Byers, 2013-07-18 22:20:32]

Thanks Levi.  CL updated (you can just review the diff from the previous
patchset).

>
https://codereview.chromium.org/17471008/diff/56001/Source/core/page/scrollin...
> > > Source/core/page/scrolling/ScrollingCoordinator.cpp:324: // This page may
> not
> > > have compositing enabled at all.
> > > So I get that, but then we should return, not continue, right?
> > 
> > I wasn't sure if there were other cases where where some layers but not
others
> > could have an enclosing composited layer.  Perhaps I should handle the
> > no-accelerated-compositing case earlier - short-circuiting the whole process
> (as
> > I do now for the touch-events-disabled case).  Then I can ASSERT here that
we
> > always find a composited layer (but leave the code in place just in case)?
> 
> I think that's the right behavior. It doesn't make sense to go through this
> process w/o compositing, and if we have compositing, we should always find a
> compositing layer.

Done.

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.h:571: virtual void
computeOwnHitTestRects(Vector<IntRect>&, const LayoutPoint& layerOffset) const
OVERRIDE;
On 2013/07/18 20:21:48, Levi wrote:
> On 2013/07/18 19:58:52, Rick Byers wrote:
> > On 2013/07/18 18:08:39, Levi wrote:
> > > "Own" seems redundant to me.
> > 
> > I wanted to clearly differentiate it from the public entrypoint
> > 'computeLayerHitTestRects' which computes the rects for this node and all
it's
> > children. computeHitTestRects seems ambiguous to me as to whether it's a
> single
> > node or a tree walk.
> > 
> > What about: one of these pairs of names?
> > computeHitTestsRectsForTree / computeHitTestsRectsForRenderer
> > or
> > computeTreeHitTestRects / computeSelfHitTestRects
> 
> I like computeSelfHitTestRects

Done.

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/17471008/diff/45001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:592:
rects.append(pixelSnappedIntRect(layerOffset, size()));
On 2013/07/18 20:21:48, Levi wrote:
> On 2013/07/18 19:58:52, Rick Byers wrote:
> > On 2013/07/18 18:08:39, Levi wrote:
> > > This won't actually get pixel snapping right since you're not using the
> > > locationOffset. You should use enclosingIntRect (we don't mind if this is
a
> > > pixel too big, but we don't want a pixel too small).
> > 
> > Done.  Also fixed RenderBlock::computeOwnHitTestRects.
> > 
> > On a related note, perhaps it would be simpler (and more precise) if I just
> > tracked LayoutRects here and converted to enclosing IntRect only just before
> > sending them to the compositor?
> 
> Yes.

Done.  The only complication was RenderInline::computeSelfHitTestRects - the
AbsoluteRectsGeneratorContext was designed just for IntRects (and none of the
other existing GeneratorContext objects seemed suitable), so I added a new one -
AbsoluteLayoutRectsGeneratorContext.

[leviw_travelin_and_unemployed, 2013-07-19 17:02:49]

Now with unnamed namespaces! if you add a goto I think we've nearly covered
everything :)

LGTM. Great work. I do wish we had a perf test that covered this...

[Rick Byers, 2013-07-19 17:37:48]

On 2013/07/19 17:02:49, Levi wrote:
> Now with unnamed namespaces! if you add a goto I think we've nearly covered
> everything :)
> 
> LGTM. Great work. I do wish we had a perf test that covered this...

Heh, thanks!  

I'm doing some more detailed perf testing now.  I think the telemetry page_load
measurement is really the right high-level perf test (the storm of layout that
happens on page load is the stress test really).  I'm not sure if there's much
value in adding a micro benchmark to our automated suites, but I'm certainly
doing some myself.  In particular, I created a telemetry measurement that
records how many touch handlers, hit test rects, and rect updates a page load
has - last night I ran that over the top_25, key_mobile_sites and
key_dekstop_sites pagesets.  There are certainly some bad cases that should be
improved (eg. newegg.com has ~2500 touch hit rects and so a little over half of
the time in layout is spent inside the hit testing code) - I'll upload separate
CLs to improve these cases shortly.

[commit-bot: I haz the power, 2013-07-19 18:05:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/17471008/43108

[commit-bot: I haz the power, 2013-07-20 00:54:13]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-07-20 00:59:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/17471008/43108

[commit-bot: I haz the power, 2013-07-20 09:07:19]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-07-20 16:58:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/17471008/43108

[commit-bot: I haz the power, 2013-07-20 19:37:58]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-07-21 12:37:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/17471008/43108

[commit-bot: I haz the power, 2013-07-21 13:14:04]

Change committed as 154611

[cbiesinger, 2013-07-22 17:53:48]

On 2013/07/21 13:14:04, I haz the power (commit-bot) wrote:
> Change committed as 154611

Is it possible that this made compositing/iframes/iframe-content-flipping.html
consistently fail on Mac dbg?
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=com...

[Rick Byers, 2013-07-22 19:28:18]

On 2013/07/22 17:53:48, cbiesinger wrote:
> On 2013/07/21 13:14:04, I haz the power (commit-bot) wrote:
> > Change committed as 154611
> 
> Is it possible that this made compositing/iframes/iframe-content-flipping.html
> consistently fail on Mac dbg?
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=com...

I suppose it's possible (although I didn't see such failures in my CQ / try job
runs).  I'm looking into the details of this failure now.

[Rick Byers, 2013-07-22 20:06:09]

On 2013/07/22 19:28:18, Rick Byers wrote:
> On 2013/07/22 17:53:48, cbiesinger wrote:
> > On 2013/07/21 13:14:04, I haz the power (commit-bot) wrote:
> > > Change committed as 154611
> > 
> > Is it possible that this made
compositing/iframes/iframe-content-flipping.html
> > consistently fail on Mac dbg?
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=com...
> 
> I suppose it's possible (although I didn't see such failures in my CQ / try
job
> runs).  I'm looking into the details of this failure now.

It looks like this test is flaky in the same way (border being the wrong color
or something) on linux and windows too - just not as consistently (so there's
definitely some sort of non-determinism issue here).  I can't see any way my CL
could have caused that (since this page has no touch event handlers, my change
_should_ have been almost entirely a no-op.  EXCEPT that it could have affected
timing slightly (basically on each layout I'd quickly see there are no touch
handlers and tell Internals that there are no rects, for which it'll allocate an
empty list in case the test asks for it).

I'll try running the test in a loop on my Linux desktop to see if I can get it
to fail with and without my change.