Fixing flaky overscroll and touch exploration mode browser tests.

Fixing flaky overscroll and touch exploration mode browser tests

The issue is that right after the WebCOntents window is created, it gets resized. The resize message is sent to the rendeer and until the confirmation is received, all mouse moves and touch moves are "held", which affects a bunch of tests.

- For in-process browser tests, ensure the resize is completed before the main   test body is executed
- For content browser tests the solution is a bit more tricky. The WebContents   window is lazily created when the first navigation happens and for content browser tests the first navigation doesn't happen as part of the setup. So the fix for this case is to add "wait for resize" functionality to TestNavigationObserver, which is a helper class used to wait for navigations to complete.

BUG=305722, 357311
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280356

[sadrul, 2014-06-12 23:25:52]

https://codereview.chromium.org/330113002/diff/1/content/public/test/browser_...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/330113002/diff/1/content/public/test/browser_...
content/public/test/browser_test_utils.cc:248: 
Could this work with RenderWidgetHostImpl::resize_ack_pending_ instead of with
aura::WindowEventDispatcher::move_hold_count_? That way, it would be applicable
on all platforms.

I am not sure that DrawWaiterForTest::WaitForCommit() is the right thing to do
while waiting for the resize to end. Ideally, this would work like
WaitForLoadStop(), where it spins up a nested message-loop and terminates it
when an event happens (e.g. checking resize_ack_pending_ on
NOTIFICATION_RENDER_WIDGET_HOST_DID_UPDATE_BACKING_STORE might work for this
case, although it doesn't feel that clean).

[mfomitchev, 2014-06-13 15:20:57]

Done. Does this rest of it look okay in general?

https://codereview.chromium.org/330113002/diff/1/content/public/test/browser_...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/330113002/diff/1/content/public/test/browser_...
content/public/test/browser_test_utils.cc:248: 
On 2014/06/12 23:25:51, sadrul wrote:
> Could this work with RenderWidgetHostImpl::resize_ack_pending_ instead of with
> aura::WindowEventDispatcher::move_hold_count_? That way, it would be
applicable
> on all platforms.
> 
> I am not sure that DrawWaiterForTest::WaitForCommit() is the right thing to do
> while waiting for the resize to end. Ideally, this would work like
> WaitForLoadStop(), where it spins up a nested message-loop and terminates it
> when an event happens (e.g. checking resize_ack_pending_ on
> NOTIFICATION_RENDER_WIDGET_HOST_DID_UPDATE_BACKING_STORE might work for this
> case, although it doesn't feel that clean).

Done.

[mfomitchev, 2014-06-13 15:55:01]

Cleaned up the code - ready for a real review now.

[mfomitchev, 2014-06-13 15:56:21]

https://codereview.chromium.org/330113002/diff/40001/content/public/test/brow...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/330113002/diff/40001/content/public/test/brow...
content/public/test/browser_test_utils.cc:223:
WaitForResizeComplete(web_contents);
I wonder if we should make it so that this is done only for the very first
navigation...

https://codereview.chromium.org/330113002/diff/40001/content/public/test/test...
File content/public/test/test_navigation_observer.cc (right):

https://codereview.chromium.org/330113002/diff/40001/content/public/test/test...
content/public/test/test_navigation_observer.cc:102:
WaitForResizeComplete(web_contents_);
I wonder if we should make it so that this is done only for the very first
navigation...

[mfomitchev, 2014-06-16 17:20:36]

@jam - can you please review? Thank you!

[jam, 2014-06-16 19:59:53]

Almost most browser tests don't care about resize events. Why do this for all
tests?

[mfomitchev, 2014-06-16 21:34:00]

All touch and mouse moves are "held" while the resize is in progress. Most
browser tests don't care but those that test touch or mouse functionality do.
I've hit this twice with different tests (touch exploration mode and overscroll)
and spent some time debugging before I got to the root cause. I could've just
fixed those specific tests, but I wanted to save other developers from a similar
hassle. This might also fix some other flaky tests which already exist. My hope
is that it wouldn't impact tests that do not care (other than waiting a bit
longer), and it would help tests that do.

[jam, 2014-06-16 22:48:27]

On 2014/06/16 21:34:00, mfomitchev wrote:
> All touch and mouse moves are "held" while the resize is in progress. Most
> browser tests don't care but those that test touch or mouse functionality do.
> I've hit this twice with different tests (touch exploration mode and
overscroll)
> and spent some time debugging before I got to the root cause. I could've just
> fixed those specific tests, but I wanted to save other developers from a
similar
> hassle. This might also fix some other flaky tests which already exist. My
hope
> is that it wouldn't impact tests that do not care (other than waiting a bit
> longer), and it would help tests that do.

It doesn't scale that we make the browser test harness wait for all the events
that various subclasses of browser tests need to happen first. So please only do
this for tests which need it. If necessary you can create base classes for tests
that need this (i.e. something like TouchBrowserTest) to happen so that each
test gets it for free.

[mfomitchev, 2014-06-18 02:07:40]

Ok, fair enough. I've removed the changes to the base test classes. 
I've made the exploration mode test wait for resize.
The overscroll tests actually work as is now with Sadrul's recent fix, so I've
just enabled them.

[jam, 2014-06-19 16:25:31]

https://codereview.chromium.org/330113002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/330113002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:734: bool
RenderWidgetHostImpl::IsResizeAckPendingForTesting() {
nit: inline this in the header as resize_ack_pending_for_testing()

https://codereview.chromium.org/330113002/diff/80001/content/public/test/brow...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/330113002/diff/80001/content/public/test/brow...
content/public/test/browser_test_utils.cc:37: #include "ui/aura/window.h"
this isn't going to compile on platforms where we don't use aura

seems like you want to wrap the new method with an USE_AURA ifdef since it's
only used in aura tests anyways?

(nit: in the future, sending tryjobs before review would be helpful in catching
these things before a reviewer looks at it)

[mfomitchev, 2014-06-19 20:04:57]

https://codereview.chromium.org/330113002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/330113002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:734: bool
RenderWidgetHostImpl::IsResizeAckPendingForTesting() {
On 2014/06/19 16:25:31, jam wrote:
> nit: inline this in the header as resize_ack_pending_for_testing()

Done.

https://codereview.chromium.org/330113002/diff/80001/content/public/test/brow...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/330113002/diff/80001/content/public/test/brow...
content/public/test/browser_test_utils.cc:37: #include "ui/aura/window.h"
Done. Good point.

[jam, 2014-06-23 07:12:05]

lgtm

https://codereview.chromium.org/330113002/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/330113002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:259: inline bool
resize_ack_pending_for_testing() {return resize_ack_pending_;}
nit: { return resize_ack_pending_; } per style guide

no need for "inline"

https://codereview.chromium.org/330113002/diff/120001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/330113002/diff/120001/content/public/test/bro...
content/public/test/browser_test_utils.cc:240: if (content) {
nit: in general, early return if possible to reduce indentation. i.e.

if (!content)
  return;

[sadrul, 2014-06-24 17:22:11]

https://codereview.chromium.org/330113002/diff/120001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.h (right):

https://codereview.chromium.org/330113002/diff/120001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.h:102: 
Do not add this here. Use a WindowEventDispatcherTestApi instead (see
ui/aura/test/window_test_api, for example)

[mfomitchev, 2014-06-24 19:54:25]

https://codereview.chromium.org/330113002/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/330113002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:259: inline bool
resize_ack_pending_for_testing() {return resize_ack_pending_;}
On 2014/06/23 07:12:05, jam wrote:
> nit: { return resize_ack_pending_; } per style guide
> 
> no need for "inline"

Done.

https://codereview.chromium.org/330113002/diff/120001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/330113002/diff/120001/content/public/test/bro...
content/public/test/browser_test_utils.cc:240: if (content) {
On 2014/06/23 07:12:05, jam wrote:
> nit: in general, early return if possible to reduce indentation. i.e.
> 
> if (!content)
>   return;

Done.

https://codereview.chromium.org/330113002/diff/120001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.h (right):

https://codereview.chromium.org/330113002/diff/120001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.h:102: 
On 2014/06/24 17:22:11, sadrul wrote:
> Do not add this here. Use a WindowEventDispatcherTestApi instead (see
> ui/aura/test/window_test_api, for example)

Done.

[sadrul, 2014-06-25 22:28:05]

lgtm

[mfomitchev, 2014-06-25 22:29:22]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-06-25 22:31:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/330113002/140001

[commit-bot: I haz the power, 2014-06-26 01:43:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/21703)

[commit-bot: I haz the power, 2014-06-26 01:47:44]

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

[commit-bot: I haz the power, 2014-06-26 01:47:45]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[mfomitchev, 2014-06-26 20:52:32]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-06-26 20:53:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/330113002/140001

[commit-bot: I haz the power, 2014-06-26 21:58:14]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/22148)

[commit-bot: I haz the power, 2014-06-26 22:00:20]

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

[commit-bot: I haz the power, 2014-06-26 22:00:21]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[mfomitchev, 2014-06-26 22:08:16]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-06-26 22:09:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/330113002/140001

[commit-bot: I haz the power, 2014-06-26 23:07:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/22186)

[commit-bot: I haz the power, 2014-06-26 23:10:36]

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

[commit-bot: I haz the power, 2014-06-26 23:10:37]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[mfomitchev, 2014-06-26 23:25:30]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-06-26 23:27:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/330113002/140001

[commit-bot: I haz the power, 2014-06-27 00:22:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/22237)

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

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

[commit-bot: I haz the power, 2014-06-27 00:26:14]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[mfomitchev, 2014-06-27 12:50:08]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-06-27 12:50:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/330113002/180001

[commit-bot: I haz the power, 2014-06-27 16:17:29]

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

[commit-bot: I haz the power, 2014-06-27 17:37:13]

Change committed as 280356