Report correct touch hit rects for fullscreen HTML5 video on Android

Report correct touch hit rects when
RuntimeEnabledFeatures::overlayFullscreenVideoEnabled is true and have a touch
handler on the document or body.

We had an optimization in the cc touch hit rect calculation that if there was a
handler on the document or body then we'd short circuit the computation and
report a single rect at the root layer covering the whole page. Now when making
an HTML5 video element fullscreen with this flag turned on the
RenderLayerCompositor removes the root cc::layer and reattaches the video layer
causing the compositor to think that there are no handlers registered.


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

[Zeeshan Qureshi, 2014-06-07 01:33:10]

Rick, can you do a first pass? Should we be adding a test for this edge case?

[abarth-chromium, 2014-06-07 09:05:55]

On 2014/06/07 at 01:33:10, zeeshanq wrote:
> Should we be adding a test for this edge case?

Yes, please add a test.

[abarth-chromium, 2014-06-07 09:06:03]

https://codereview.chromium.org/317373007/diff/20001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/317373007/diff/20001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:797: bool
inFullscreenOnAndroid = RuntimeEnabledFeatures::overlayFullscreenVideoEnabled()
&& RenderLayerCompositor::findFullscreenVideoRenderer(*document);
It's inefficient to call findFullscreenVideoRenderer more than once per frame. 
Can the RenderLayerCompositor and this code share the information about whether
we're inFullscreenOnAndroid instead of each computing it separately.

Also, please don't refer to Android in the variable name.  In principle, this
can happen whenever RuntimeEnabledFeatures::overlayFullscreenVideoEnabled is
true, regardless of whether we're on Android.

[Zeeshan Qureshi, 2014-06-09 23:51:54]

Thanks Adam! I updated the CL to share state between the two.

I still need to plumb the flag when running the test and clean it up a bit but
does this look like the right approach to you? This doesn't test the video drag
directly but the exception that we added to the touch hit rect computation.

[Zeeshan Qureshi, 2014-06-10 19:19:13]

The Android tests in fullscreen/ were timing out on my device so created a
separate test suite for this.

[Rick Byers, 2014-06-10 20:02:24]

I still need to review the tests, but this general approach seems OK to me. 
Nice and simple.  I'm a little worried about the perf impact of potentially
computing rects for all nodes in a big document when full screen video is up,
but presumably this should happen exactly once when we go full screen (i.e. we
won't be getting layouts etc. while the video is playing), right?

https://codereview.chromium.org/317373007/diff/100001/Source/core/page/scroll...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/317373007/diff/100001/Source/core/page/scroll...
Source/core/page/scrolling/ScrollingCoordinator.cpp:797: for
(TouchEventTargetSet::const_iterator iter = targets->begin();
!document->renderView()->compositor()->inOverlayFullscreenVideo() && iter !=
targets->end(); ++iter) {
rather than check inOverlayFullscreenVideo on each iteration of the loop, I'd
find it clearer to just have this whole section inside of it's own if statement.

https://codereview.chromium.org/317373007/diff/100001/Source/core/rendering/c...
File Source/core/rendering/compositing/RenderLayerCompositor.h (right):

https://codereview.chromium.org/317373007/diff/100001/Source/core/rendering/c...
Source/core/rendering/compositing/RenderLayerCompositor.h:289: void
applyOverlayFullscreenVideoAdjustment();
we usually list all the methods before the fields, so this should move back up
to where it was.

[Rick Byers, 2014-06-10 21:39:57]

https://codereview.chromium.org/317373007/diff/100001/LayoutTests/fast/events...
File
LayoutTests/fast/events/touch/overlayfullscreenvideo/compositor-touch-hit-rects-fullscreen-video-controls-expected.txt
(right):

https://codereview.chromium.org/317373007/diff/100001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/overlayfullscreenvideo/compositor-touch-hit-rects-fullscreen-video-controls-expected.txt:1:
This test makes sure that touch hit rects are reported for fullscreen HTML5
video control elements when
RuntimeEnabledFeatures::overlayFullscreenVideoEnabled() is true.
Nit: tweak the test description for the fact that it also runs with this flag
disabled (in which case we expect the document rects).

https://codereview.chromium.org/317373007/diff/100001/LayoutTests/fast/events...
File
LayoutTests/fast/events/touch/overlayfullscreenvideo/compositor-touch-hit-rects-fullscreen-video-controls.html
(right):

https://codereview.chromium.org/317373007/diff/100001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/overlayfullscreenvideo/compositor-touch-hit-rects-fullscreen-video-controls.html:22:
function logRects(fullscreen) {
rather than copy/paste code from the touch hit testing tests, can you just
invoke the functions you need (refactoring there as necessary)?

https://codereview.chromium.org/317373007/diff/100001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):

https://codereview.chromium.org/317373007/diff/100001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/base.py:1751:
VirtualTestSuite('hittesting',
I think you can use the existing 'android' virtual test suite for this,
shouldn't have to create your own.

[Zeeshan Qureshi, 2014-06-11 23:31:40]

Rick, tried to address most of your comments.

[Rick Byers, 2014-06-12 15:47:20]

Thanks, LGTM.

+trchen from the overlay fullscreen video perspective.

https://codereview.chromium.org/317373007/diff/180001/LayoutTests/fullscreen/...
File
LayoutTests/fullscreen/compositor-touch-hit-rects-fullscreen-video-controls.html
(right):

https://codereview.chromium.org/317373007/diff/180001/LayoutTests/fullscreen/...
LayoutTests/fullscreen/compositor-touch-hit-rects-fullscreen-video-controls.html:9:
video control elements when
I'm not sure what "document handler is false" means, and since this test runs
both with and without overlayFullscreenVideoEnabled I'd simplify the
description.  Eg. "... video control elements even when there is a document
handler".

[trchen, 2014-06-12 21:35:04]

lgtm too. Thanks you!

[Zeeshan Qureshi, 2014-06-12 21:43:21]

The CQ bit was checked by zeeshanq@chromium.org

[Zeeshan Qureshi, 2014-06-12 21:43:35]

Thanks!

[commit-bot: I haz the power, 2014-06-12 21:43:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zeeshanq@chromium.org/317373007/200001

[commit-bot: I haz the power, 2014-06-12 22:54:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[commit-bot: I haz the power, 2014-06-13 00:04:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-13 00:04:04]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[Zeeshan Qureshi, 2014-06-13 00:05:19]

The CQ bit was checked by zeeshanq@chromium.org

[commit-bot: I haz the power, 2014-06-13 00:05:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zeeshanq@chromium.org/317373007/200001

[commit-bot: I haz the power, 2014-06-13 01:08:33]

Change committed as 176071