New issue
Advanced search Search tips

Issue 626761 link

Starred by 3 users

Issue metadata

Status: Archived
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Some of the tab dragging tests fail on chromeos because of IsWindowPositionManaged

Project Member Reported by sky@chromium.org, Jul 8 2016

Issue description

I'm in the process of reenabling some of the tab dragging tests on chromeos. As part of reenabling them some are failing because of the wrong value from IsWindowPositionManaged(). I'm going to comment out the assertions for now until they can be reevaluated.
 
Owner: abodenha@chromium.org
Status: Assigned (was: Untriaged)
Adding abodenha@ to triage
Owner: afakhry@chromium.org
Failing tests due to this:

1 - TabDragging/DetachToBrowserTabDragControllerTest.DetachToOwnWindow/0, where GetParam() = "mouse"
2 - TabDragging/DetachToBrowserTabDragControllerTest.DetachFromFullsizeWindow/0, where GetParam() = "mouse"
3 - TabDragging/DetachToBrowserTabDragControllerTest.DetachToOwnWindowFromMaximizedWindow/0, where GetParam() = "mouse"
4 - TabDragging/DetachToBrowserTabDragControllerTest.DetachToDockedWindowFromMaximizedWindow/0, where GetParam() = "mouse"
5 - TabDragging/DetachToBrowserTabDragControllerTest.DetachToDockedWindowFromMaximizedWindow/1, where GetParam() = "touch"


Status: Started (was: Assigned)
I managed to fix all the above tests except DetachFromFullsizeWindow.

varkha@, it seems you wrote this test. This test fails on the bot which is using the openbox WM. Setting the browser window's bounds to the work area's bounds is NOT equivalent to the window being full screen in that Window Manager.

Is that a bug in the test or the code itself?
Cc: pkasting@chromium.org
I will keep the test TabDragging/DetachToBrowserTabDragControllerTest.DetachFromFullsizeWindow/0 disabled on Linux for now. It passes locally but fails on the bot. It fails locally only if I run the test with openbox. The reason is the following:

Setting the source browser window's bound to the work area bounds will go to DesktopWindowTreeHostX11::SetBounds() which will adjust the bounds by subtracting 1px from width and height. To the TabDragController, the source browser window is no longer full size, and it will not maximize the detached window on completing the drag. Hence the test fails because it expects that window to be maximized.


Comment 6 by varkha@chromium.org, Nov 16 2016

#5, could also change the expectation of the newly created window to be maximized after the drag to be different on linux since the behavior is different (and you have a system-dependent disable anyway.
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c813d7634e5c6ba6c808e326778b0cdb4f6e773c

commit c813d7634e5c6ba6c808e326778b0cdb4f6e773c
Author: afakhry <afakhry@chromium.org>
Date: Thu Dec 01 17:33:40 2016

Reenable Tabdragging tests failing because of IsWindowPositionManaged()

It used to be exiting the move loop and clearing the move_loop_widget_ before
EndDrag() had a chance to set the window position to manageable again.

This CL adds a maximized browser window waiter, and moves setting the window
position managed logic to ash.

BUG= 626761 ,  331924 
TEST=interactive_ui_tests --gtest_filter=TabDragging*

Review-Url: https://codereview.chromium.org/2494713003
Cr-Commit-Position: refs/heads/master@{#435638}

[modify] https://crrev.com/c813d7634e5c6ba6c808e326778b0cdb4f6e773c/ash/wm/toplevel_window_event_handler.cc
[modify] https://crrev.com/c813d7634e5c6ba6c808e326778b0cdb4f6e773c/ash/wm/toplevel_window_event_handler_unittest.cc
[modify] https://crrev.com/c813d7634e5c6ba6c808e326778b0cdb4f6e773c/chrome/browser/ui/views/tabs/tab_drag_controller.cc
[modify] https://crrev.com/c813d7634e5c6ba6c808e326778b0cdb4f6e773c/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3a9a59b140a23a82da474ede5c5abb311bb088c1

commit 3a9a59b140a23a82da474ede5c5abb311bb088c1
Author: rockot <rockot@chromium.org>
Date: Thu Dec 01 22:39:55 2016

Revert of Reenable Tabdragging tests failing because of IsWindowPositionManaged() (patchset #10 id:200001 of https://codereview.chromium.org/2494713003/ )

Reason for revert:
Causing failures in ash_unittests. See CL comments for details. If you have a fix, please reland with the fix applied.

Original issue's description:
> Reenable Tabdragging tests failing because of IsWindowPositionManaged()
>
> It used to be exiting the move loop and clearing the move_loop_widget_ before
> EndDrag() had a chance to set the window position to manageable again.
>
> This CL adds a maximized browser window waiter, and moves setting the window
> position managed logic to ash.
>
> BUG= 626761 ,  331924 
> TEST=interactive_ui_tests --gtest_filter=TabDragging*
>
> Committed: https://crrev.com/c813d7634e5c6ba6c808e326778b0cdb4f6e773c
> Cr-Commit-Position: refs/heads/master@{#435638}

TBR=pkasting@chromium.org,sky@chromium.org,skuhne@chromium.org,erg@chromium.org,afakhry@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 626761 ,  331924 

Review-Url: https://codereview.chromium.org/2550533002
Cr-Commit-Position: refs/heads/master@{#435742}

[modify] https://crrev.com/3a9a59b140a23a82da474ede5c5abb311bb088c1/ash/wm/toplevel_window_event_handler.cc
[modify] https://crrev.com/3a9a59b140a23a82da474ede5c5abb311bb088c1/ash/wm/toplevel_window_event_handler_unittest.cc
[modify] https://crrev.com/3a9a59b140a23a82da474ede5c5abb311bb088c1/chrome/browser/ui/views/tabs/tab_drag_controller.cc
[modify] https://crrev.com/3a9a59b140a23a82da474ede5c5abb311bb088c1/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/83e8abcce5801546b4bb2ff03fdaf5cb4590f0e5

commit 83e8abcce5801546b4bb2ff03fdaf5cb4590f0e5
Author: afakhry <afakhry@chromium.org>
Date: Thu Dec 01 23:54:52 2016

Reland of Reenable Tabdragging tests failing because of IsWindowPositionManaged()

It was reverted previously as it caused a crash in
WorkspaceEventHandlerTest.DeleteWhileInRunLoop.

It used to be exiting the move loop and clearing the move_loop_widget_ before
EndDrag() had a chance to set the window position to manageable again.

This CL adds a maximized browser window waiter, and moves setting the window
position managed logic to ash.

TBR=pkasting@chromium.org,sky@chromium.org,skuhne@chromium.org,oshima@chromium.org,erg@chromium.org
BUG= 626761 ,  331924 
TEST=interactive_ui_tests --gtest_filter=TabDragging*

Review-Url: https://codereview.chromium.org/2540013005
Cr-Commit-Position: refs/heads/master@{#435775}

[modify] https://crrev.com/83e8abcce5801546b4bb2ff03fdaf5cb4590f0e5/ash/wm/toplevel_window_event_handler.cc
[modify] https://crrev.com/83e8abcce5801546b4bb2ff03fdaf5cb4590f0e5/ash/wm/toplevel_window_event_handler_unittest.cc
[modify] https://crrev.com/83e8abcce5801546b4bb2ff03fdaf5cb4590f0e5/chrome/browser/ui/views/tabs/tab_drag_controller.cc
[modify] https://crrev.com/83e8abcce5801546b4bb2ff03fdaf5cb4590f0e5/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc

Status: Fixed (was: Started)

Comment 11 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58

Comment 12 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 13 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Labels: VerifyIn-61

Comment 15 by dchan@chromium.org, Oct 14 2017

Status: Archived (was: Fixed)
Components: Tests>Disabled
Labels: Test-Disabled
Project Member

Comment 17 by bugdroid1@chromium.org, Jan 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9de4762785e0e8a03ae83f1c834b002cfbc0a868

commit 9de4762785e0e8a03ae83f1c834b002cfbc0a868
Author: Peter Kasting <pkasting@chromium.org>
Date: Thu Jan 03 20:22:59 2019

Enable disabled tests in c/b/ui/views/tabs/.

Bug:  331924 , 603562,  626761 ,  837219 
Change-Id: If2695993d98dc00168c367c3c171b99a96eb7bc5
Reviewed-on: https://chromium-review.googlesource.com/c/1351809
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619736}
[modify] https://crrev.com/9de4762785e0e8a03ae83f1c834b002cfbc0a868/chrome/browser/ui/views/frame/browser_frame.cc
[modify] https://crrev.com/9de4762785e0e8a03ae83f1c834b002cfbc0a868/chrome/browser/ui/views/frame/browser_frame.h
[modify] https://crrev.com/9de4762785e0e8a03ae83f1c834b002cfbc0a868/chrome/browser/ui/views/frame/browser_non_client_frame_view.h
[modify] https://crrev.com/9de4762785e0e8a03ae83f1c834b002cfbc0a868/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc
[modify] https://crrev.com/9de4762785e0e8a03ae83f1c834b002cfbc0a868/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h
[modify] https://crrev.com/9de4762785e0e8a03ae83f1c834b002cfbc0a868/chrome/browser/ui/views/frame/browser_non_client_frame_view_mac.h
[modify] https://crrev.com/9de4762785e0e8a03ae83f1c834b002cfbc0a868/chrome/browser/ui/views/frame/browser_non_client_frame_view_mac.mm
[modify] https://crrev.com/9de4762785e0e8a03ae83f1c834b002cfbc0a868/chrome/browser/ui/views/frame/browser_view.h
[modify] https://crrev.com/9de4762785e0e8a03ae83f1c834b002cfbc0a868/chrome/browser/ui/views/frame/glass_browser_frame_view.cc
[modify] https://crrev.com/9de4762785e0e8a03ae83f1c834b002cfbc0a868/chrome/browser/ui/views/frame/glass_browser_frame_view.h
[modify] https://crrev.com/9de4762785e0e8a03ae83f1c834b002cfbc0a868/chrome/browser/ui/views/frame/opaque_browser_frame_view.cc
[modify] https://crrev.com/9de4762785e0e8a03ae83f1c834b002cfbc0a868/chrome/browser/ui/views/frame/opaque_browser_frame_view.h
[modify] https://crrev.com/9de4762785e0e8a03ae83f1c834b002cfbc0a868/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
[modify] https://crrev.com/9de4762785e0e8a03ae83f1c834b002cfbc0a868/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.h

Sign in to add a comment