Fix flakiness in DragAndDropBrowserTests

Fix flakiness in DragAndDropBrowserTests

Multiple drag and drop tests in interactive_ui_tests have been flaky
with OOPIFs present. One cause was addressed in r460177, and this CL
fixes another, which is that UI interaction can happen before surface-
based hit testing is ready in the browser process.

This CL includes a small refactor of SurfaceHitTestReadyNotifier to
unify code paths for tests inside and outside of content/.

Most of this patch was written by lukasza@.

BUG=671445, 704603
Review-Url: https://codereview.chromium.org/2786223002
Cr-Commit-Position: refs/heads/master@{#461490}
Committed: https://chromium.googlesource.com/chromium/src/+/66c46290f9774951731d1d38ad75d7aa2d1ac23d

[kenrb, 2017-03-30 19:15:53]

The CQ bit was checked by kenrb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 19:16:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 20:40:52]

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

[commit-bot: I haz the power, 2017-03-30 20:40:52]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-03-30 22:16:07]

kenrb@chromium.org changed reviewers:
+ alexmos@chromium.org

[kenrb, 2017-03-30 22:16:08]

alexmos@: PTAL content/?

I'm not certain this fixes the flakiness in the tests, but it is a plausible
cause, and is a necessary cleanup.

[alexmos, 2017-03-31 21:57:55]

LGTM with nits.  Thanks for the refactor, the tests look much nicer with this.

https://codereview.chromium.org/2786223002/diff/1/chrome/browser/ui/views/dra...
File chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc (left):

https://codereview.chromium.org/2786223002/diff/1/chrome/browser/ui/views/dra...
chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:841: // Flaky on
Linux. crbug.com/704603
Since you're reenabling this, also reference this bug in the description?

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

https://codereview.chromium.org/2786223002/diff/1/content/public/test/browser...
content/public/test/browser_test_utils.cc:1177: // should probably merge these.
Update this comment?

https://codereview.chromium.org/2786223002/diff/1/content/public/test/browser...
content/public/test/browser_test_utils.cc:1191: cc::SurfaceId root_surface_id_;
This appears to be unused now.

https://codereview.chromium.org/2786223002/diff/1/content/public/test/browser...
content/public/test/browser_test_utils.cc:1211: // frequent in tests in this
file.
"in this file" is outdated, since the tests are now elsewhere

https://codereview.chromium.org/2786223002/diff/1/content/public/test/browser...
content/public/test/browser_test_utils.cc:1261: if
(!child_view->IsRenderWidgetHostViewChildFrame())
Do we care to handle the case where child_view is null? (e.g., if the frame
crashed?)

[kenrb, 2017-04-03 17:17:57]

Description was changed from

==========
Fix flakiness in DragAndDropBrowserTests

Multiple drag and drop tests in interactive_ui_tests have been flaky
with OOPIFs present. One cause was addressed in r460177, and this CL
fixes another, which is that UI interaction can happen before surface-
based hit testing is ready in the browser process.

This CL includes a small refactor of SurfaceHitTestReadyNotifier to
unify code paths for tests inside and outside of content/.

Most of this patch was written by lukasza@.

BUG=671445
==========

to

==========
Fix flakiness in DragAndDropBrowserTests

Multiple drag and drop tests in interactive_ui_tests have been flaky
with OOPIFs present. One cause was addressed in r460177, and this CL
fixes another, which is that UI interaction can happen before surface-
based hit testing is ready in the browser process.

This CL includes a small refactor of SurfaceHitTestReadyNotifier to
unify code paths for tests inside and outside of content/.

Most of this patch was written by lukasza@.

BUG=671445,704603
==========

[kenrb, 2017-04-03 17:28:08]

The CQ bit was checked by kenrb@chromium.org to run a CQ dry run

[kenrb, 2017-04-03 17:28:29]

kenrb@chromium.org changed reviewers:
+ paulmeyer@chromium.org

[kenrb, 2017-04-03 17:28:30]

Thanks alexmos.

paulmeyer@: PTAL drag_and_drop_interactive_uitest.cc?

https://codereview.chromium.org/2786223002/diff/1/chrome/browser/ui/views/dra...
File chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc (left):

https://codereview.chromium.org/2786223002/diff/1/chrome/browser/ui/views/dra...
chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:841: // Flaky on
Linux. crbug.com/704603
On 2017/03/31 21:57:54, alexmos wrote:
> Since you're reenabling this, also reference this bug in the description?

Done.

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

https://codereview.chromium.org/2786223002/diff/1/content/public/test/browser...
content/public/test/browser_test_utils.cc:1177: // should probably merge these.
On 2017/03/31 21:57:55, alexmos wrote:
> Update this comment?

Removed instead, it becomes obsolete with this change, since the paths are
basically merged.

https://codereview.chromium.org/2786223002/diff/1/content/public/test/browser...
content/public/test/browser_test_utils.cc:1191: cc::SurfaceId root_surface_id_;
On 2017/03/31 21:57:55, alexmos wrote:
> This appears to be unused now.

Done.

https://codereview.chromium.org/2786223002/diff/1/content/public/test/browser...
content/public/test/browser_test_utils.cc:1211: // frequent in tests in this
file.
On 2017/03/31 21:57:55, alexmos wrote:
> "in this file" is outdated, since the tests are now elsewhere

Done. Also reworked the comment a bit.

https://codereview.chromium.org/2786223002/diff/1/content/public/test/browser...
content/public/test/browser_test_utils.cc:1261: if
(!child_view->IsRenderWidgetHostViewChildFrame())
On 2017/03/31 21:57:54, alexmos wrote:
> Do we care to handle the case where child_view is null? (e.g., if the frame
> crashed?)

Added.

[commit-bot: I haz the power, 2017-04-03 17:28:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[paulmeyer, 2017-04-03 17:53:15]

LGTM

[commit-bot: I haz the power, 2017-04-03 18:30:09]

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

[commit-bot: I haz the power, 2017-04-03 18:30:10]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-04-03 18:34:03]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2017-04-03 18:34:03]

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2786223002/#ps20001
(title: "Review comments addressed")

[commit-bot: I haz the power, 2017-04-03 18:35:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 18:47:40]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491244443334950,
"parent_rev": "bc4ace68733437fde610ae43ff62a97719a6d816", "commit_rev":
"66c46290f9774951731d1d38ad75d7aa2d1ac23d"}

[commit-bot: I haz the power, 2017-04-03 18:48:31]

Description was changed from

==========
Fix flakiness in DragAndDropBrowserTests

Multiple drag and drop tests in interactive_ui_tests have been flaky
with OOPIFs present. One cause was addressed in r460177, and this CL
fixes another, which is that UI interaction can happen before surface-
based hit testing is ready in the browser process.

This CL includes a small refactor of SurfaceHitTestReadyNotifier to
unify code paths for tests inside and outside of content/.

Most of this patch was written by lukasza@.

BUG=671445,704603
==========

to

==========
Fix flakiness in DragAndDropBrowserTests

Multiple drag and drop tests in interactive_ui_tests have been flaky
with OOPIFs present. One cause was addressed in r460177, and this CL
fixes another, which is that UI interaction can happen before surface-
based hit testing is ready in the browser process.

This CL includes a small refactor of SurfaceHitTestReadyNotifier to
unify code paths for tests inside and outside of content/.

Most of this patch was written by lukasza@.

BUG=671445,704603

Review-Url: https://codereview.chromium.org/2786223002
Cr-Commit-Position: refs/heads/master@{#461490}
Committed:
https://chromium.googlesource.com/chromium/src/+/66c46290f9774951731d1d38ad75...
==========

[commit-bot: I haz the power, 2017-04-03 18:48:33]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/66c46290f9774951731d1d38ad75...