Automated test for dragging between same-page, cross-site frames.

Automated test for dragging between same-page, cross-site frames.

BUG=666858
Committed: https://crrev.com/d4d9b103a38ebe19ef10fd894920081009f086a9
Cr-Commit-Position: refs/heads/master@{#437739}

[Łukasz Anforowicz, 2016-12-06 01:28:09]

lukasza@chromium.org changed reviewers:
+ nick@chromium.org

[Łukasz Anforowicz, 2016-12-06 01:28:09]

nick@, can you PTAL?

Also - feel free to comment on https://crbug.com/671445 that I've just opened. 
The new test doesn't do anything different from the other tests, so I think it
doesn't hurt to review it (and separately investigate and discuss the flakiness
I've discovered).

[ncarter (slow), 2016-12-08 22:45:43]

What's up with the red trybots?

[Łukasz Anforowicz, 2016-12-09 17:16:25]

On 2016/12/08 22:45:43, ncarter wrote:
> What's up with the red trybots?

Sorry about that.  linux_site_isolation was my fault (the new test expectedly
fails with --site-per-process but I screwed up the wildcards in
testing/buildbot/filters/site-per-process.interactive_ui_tests.filter).  Other
bot failures don't seem like my fault (and in fact didn't repro again in the
latest patchset) - I see that there were some PolicyUITest.SendPolicyValues
failures on mac_chromium_rel_ng and linux_chromium_chromeos_rel_ng (seems like
https://crbug.com/671454) and also telemetry failures on win_chromium_rel_ng and
linux_chromium_rel_ng and some layout test failures on
linux_chromium_asan_rel_ng.  Layout tests shouldn't be impacted by the CL.  I
don't know about telemetry tests, but I've been conditioned in the past to treat
them as flakes and ignore :-P.

[ncarter (slow), 2016-12-09 18:28:40]

lgtm

https://codereview.chromium.org/2549023003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc (right):

https://codereview.chromium.org/2549023003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:971: "dragover"));
Is this value just inconsistent across platforms, or does it vary from run to
run? (if it varies from run to run, do we have to worry about flakiness?)

https://codereview.chromium.org/2549023003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:1100:
right_frame()->ExecuteJavaScriptWithUserGestureForTests(base::UTF8ToUTF16(
It's unfortunate that we can't use ExecuteScript in this case, because of the
possibility of crosstalk. We could probably make ExecuteScript a whole lot
nicer...

[Łukasz Anforowicz, 2016-12-09 22:33:27]

Thanks Nick!

https://codereview.chromium.org/2549023003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc (right):

https://codereview.chromium.org/2549023003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:971: "dragover"));
On 2016/12/09 18:28:39, ncarter wrote:
> Is this value just inconsistent across platforms, or does it vary from run to
> run? (if it varies from run to run, do we have to worry about flakiness?)

It is inconsistent across platforms.  For example - on Ash, a single mouse move
event is translated into a single dragenter OR dragover event.  With Aura/X11 a
single mouse mouse event is translated into a dragenter PLUS dragover event.

Additionally, it felt right to have relaxed verification of dragover, because
when testing manually one also gets SOME number of dragover events (but always
only a SINGLE dragstart, drop, dragend and a SINGLE dragenter/dragleave per
frame/element transition) - this depends on how fast the mouse is moved.  In
other words - it felt right to do verification that would still succeed even if
SimulateMouseMove... methods started to simulate mouse events for intermediate
points between starting and final mouse positions.

https://codereview.chromium.org/2549023003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:1100:
right_frame()->ExecuteJavaScriptWithUserGestureForTests(base::UTF8ToUTF16(
On 2016/12/09 18:28:39, ncarter wrote:
> It's unfortunate that we can't use ExecuteScript in this case, because of the
> possibility of crosstalk. We could probably make ExecuteScript a whole lot
> nicer...

We could either relax ExecuteScriptAndExtractFoo to keep waiting until a
response of type Foo is received (right now this is considerer a failure
condition - detected when ExtractScriptAndExtractFoo calls value->GetAsFoo).  I
think I dislike this idea.

I also considered adding an ExecuteScriptAndExtractDictionary - this would avoid
hitting the invalid-type condition, while still requiring spinning below (i.e.
lines 1108-1110) until *our* dictionary is received.  This felt wrong
(dictionaries are difficult to work with + adding a dictionary support for this
one test is weird).

I also thought about modifying
content/test/data/drag_and_drop/event_monitoring.js to send strings, rather than
dictionaries/objects.  This felt unnecessary.


So - I think this is fine as-is, although it does make me frown a little bit...
:-/

[Łukasz Anforowicz, 2016-12-09 22:33:37]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-12-09 22:34:54]

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, 2016-12-10 03:38:18]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481322817719600,
"parent_rev": "ee81de330c8bcea707db8262a7a44843a2c79773", "commit_rev":
"faabeb214019067f05b67672c29e67673c217370"}

[commit-bot: I haz the power, 2016-12-10 03:38:46]

Description was changed from

==========
Automated test for dragging between same-page, cross-site frames.

BUG=666858
==========

to

==========
Automated test for dragging between same-page, cross-site frames.

BUG=666858

Review-Url: https://codereview.chromium.org/2549023003
==========

[commit-bot: I haz the power, 2016-12-10 03:38:47]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-12 15:05:56]

Description was changed from

==========
Automated test for dragging between same-page, cross-site frames.

BUG=666858

Review-Url: https://codereview.chromium.org/2549023003
==========

to

==========
Automated test for dragging between same-page, cross-site frames.

BUG=666858

Committed: https://crrev.com/d4d9b103a38ebe19ef10fd894920081009f086a9
Cr-Commit-Position: refs/heads/master@{#437739}
==========

[commit-bot: I haz the power, 2016-12-12 15:05:57]

Patchset 2 (id:??) landed as
https://crrev.com/d4d9b103a38ebe19ef10fd894920081009f086a9
Cr-Commit-Position: refs/heads/master@{#437739}