Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2507223003:
Tests for dragging between two frames (potentially cross-site from main frame). (Closed)
|
Created:
4 years, 1 month ago by Łukasz Anforowicz Modified:
4 years ago Reviewers:
ncarter (slow) CC:
chromium-reviews, tfarina, dcheng, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTests for dragging between two frames (potentially cross-site from main frame).
BUG=647249
Committed: https://crrev.com/953dc658ce1d28603ee9f04795f6b9253adcdcec
Cr-Commit-Position: refs/heads/master@{#435796}
Patch Set 1 #Patch Set 2 : Rebasing... #Patch Set 3 : Need 2 events from 1 mouse event (for consistency between ash and x11). #Patch Set 4 : Maybe this is ready for landing? #
Total comments: 5
Patch Set 5 : Remove accidental Blink changes from the CL. #
Total comments: 10
Patch Set 6 : Added SuppressPassingStartDragFurther method + moved constant definitions. #
Messages
Total messages: 30 (22 generated)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
lukasza@chromium.org changed reviewers: + nick@chromium.org
Nick, can you take a look please? https://codereview.chromium.org/2507223003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc (right): https://codereview.chromium.org/2507223003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:672: // dragover, dragend and/or drop events in DragImageBetweenFrames test). This somewhat surprising difference is present with and without OOPIFs and is also present before paulmeyer@'s changes (e.g. in 54.0.2840.100), so I haven't opened a bug at this point + I think it is okay to land the CL as-is. https://codereview.chromium.org/2507223003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:735: // the left and the right frame. See also https://crbug.com/59081. A separate test will be needed to verify no regressions of https://crbug.com/59081. I plan to do this in a separate CL (which will also need some test helper to verify that unexpected events don't fire - current test verification only checks that expected events do fire). https://codereview.chromium.org/2507223003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:863: state->expected_dom_event_data.set_expected_mime_types(""); Same as above - this difference is present with and without OOPIFs and is also present before paulmeyer@'s changes (e.g. in 54.0.2840.100), so I haven't opened a bug at this point + I think it is okay to land the CL as-is. I think this difference is slightly less surprising than the other TODO above - one could say that "drop" event "consumes" the data and so there really are no mime types afterwards. Still, one day, I probably should try to understand why this happens (after adding security-regression automated tests + *after* making the tests work on at least one non-aura platform :-P). https://codereview.chromium.org/2507223003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebFrameWidgetBase.h (right): https://codereview.chromium.org/2507223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebFrameWidgetBase.h:93: Ooops. Not sure how it got here. I'll remove in the next patchset.
This test is quite an accomplishment; it looks like it was a lot of work to get it working properly. Just some questions about structure of these tests. https://codereview.chromium.org/2507223003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc (right): https://codereview.chromium.org/2507223003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:672: // dragover, dragend and/or drop events in DragImageBetweenFrames test). On 2016/11/30 16:47:38, Łukasz Anforowicz wrote: > This somewhat surprising difference is present with and without OOPIFs and is > also present before paulmeyer@'s changes (e.g. in 54.0.2840.100), so I haven't > opened a bug at this point + I think it is okay to land the CL as-is. Thanks for the info. I agree this is a weird inconsistency and we should strive tofigure it out. https://codereview.chromium.org/2507223003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc (right): https://codereview.chromium.org/2507223003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:174: // Will prevent a real, OS-level drag-and-drop from starting for this Now that we're an interactive uitest, is suppressing the OS-level d&d actually important? https://codereview.chromium.org/2507223003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:205: // without starting a nested drag-and-drop message loop. The two modes of operation of this Waiter make it kind of hard to follow. It's not clear why the "posttask" and "run message loop" behaviors are coupled like this. Maybe there's a clear reason for it that I'm not following? You might consider adding a bool "suppress_start_drag_", and a setter for it, decoupling the suppression behavior from the run-callback behavior. Then the callback and the runloop suppression could be orthogonal? https://codereview.chromium.org/2507223003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:611: const gfx::Point kMiddleOfLeftFrame = gfx::Point(155, 150); Constants go at the top of the section. https://codereview.chromium.org/2507223003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:754: base::Bind(&DragAndDropBrowserTest::DragImageBetweenFrames_Step2, This can't be inline, right, because it needs to occur inside the nested message loop? I wonder if it would could make this read more linearly by: - Making the members of TestState be variables. - Moving Step2 to a lambda defined here, capturing the TestState variables. - Making Step3 run inline. https://codereview.chromium.org/2507223003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:765: DragImageBetweenFrames_Step3(&state); This could be inlined -- but I'm guessing you did it this way so that the test statements would be declared roughly in execution order? I think that makes sense.
https://codereview.chromium.org/2507223003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc (right): https://codereview.chromium.org/2507223003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:174: // Will prevent a real, OS-level drag-and-drop from starting for this On 2016/11/30 18:32:34, ncarter wrote: > Now that we're an interactive uitest, is suppressing the OS-level d&d actually > important? It is important on Windows. On Windows, I see no way to run test tasks once we tell the OS that the drag-and-drop started (i.e. the call to old_client_->StartDragAndDrop(...) will eventuall call (in ui/views/widget/desktop_aura/desktop_drag_drop_client_win.cc) DoDragDrop in win32 API and this call doesn't return until drag and drop ends (and since it doesn't pump any tasks/messages from Chromium's message loop, the test doesn't get a chance to say: "please simulate a mouse up [and end the drag-and-drop]"). https://codereview.chromium.org/2507223003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:205: // without starting a nested drag-and-drop message loop. On 2016/11/30 18:32:33, ncarter wrote: > The two modes of operation of this Waiter make it kind of hard to follow. It's > not clear why the "posttask" and "run message loop" behaviors are coupled like > this. Maybe there's a clear reason for it that I'm not following? > > You might consider adding a bool "suppress_start_drag_", and a setter for it, > decoupling the suppression behavior from the run-callback behavior. Then the > callback and the runloop suppression could be orthogonal? Done. https://codereview.chromium.org/2507223003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:611: const gfx::Point kMiddleOfLeftFrame = gfx::Point(155, 150); On 2016/11/30 18:32:34, ncarter wrote: > Constants go at the top of the section. Done. https://codereview.chromium.org/2507223003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:754: base::Bind(&DragAndDropBrowserTest::DragImageBetweenFrames_Step2, On 2016/11/30 18:32:34, ncarter wrote: > This can't be inline, right, because it needs to occur inside the nested message > loop? Correct - DragImageBetweenFrames_Step2 occurs inside the nested message loop. > I wonder if it would could make this read more linearly by: > - Making the members of TestState be variables. > - Moving Step2 to a lambda defined here, capturing the TestState variables. > - Making Step3 run inline. Interesting... after actually trying this, I am not so sure I still like this idea: - One cannot pass *capturing* lambdas to base::Bind. This can be worked around by binding lambda args via base::Bind, but then I would have to bind/pass "self". Yuck! - The lambda is really long (70+ lines). - Still no linear order: teststep1; drag_start_waiter.PostTaskWhenDragStarts(base::Bind( [](DragAndDropBrowserTest* self, DragImageBetweenFrames_TestState* state) { teststep2; }); EXPECT_TRUE(SimulateMouseDownAndDragStartInLeftFrame()); // teststep2 really happens *here*, not *above* drag_start_waiter.WaitUntilDragStart(...); teststep3; https://codereview.chromium.org/2507223003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:765: DragImageBetweenFrames_Step3(&state); On 2016/11/30 18:32:34, ncarter wrote: > This could be inlined -- but I'm guessing you did it this way so that the test > statements would be declared roughly in execution order? I think that makes > sense. Yes - I did that so the test steps are written down in execution order.
lgtm sorry the lambda suggestion was a wild goose chase
On 2016/12/01 23:25:30, ncarter wrote: > lgtm Thanks. > sorry the lambda suggestion was a wild goose chase No worries. It initially seemed like a good idea to me as well :-)
The CQ bit was checked by lukasza@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480635813013380,
"parent_rev": "b1659fb12b80b24f0ed7298f5aa742d0eb5f4fdc", "commit_rev":
"b7de5856afaba7ad2bc0b78b399ed755d291ecdc"}
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Tests for dragging between two frames (potentially cross-site from main frame). BUG=647249 ========== to ========== Tests for dragging between two frames (potentially cross-site from main frame). BUG=647249 Committed: https://crrev.com/953dc658ce1d28603ee9f04795f6b9253adcdcec Cr-Commit-Position: refs/heads/master@{#435796} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/953dc658ce1d28603ee9f04795f6b9253adcdcec Cr-Commit-Position: refs/heads/master@{#435796} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
