Not moving a docked window when restoring a previously maximized window

Not moving a docked window when restoring a previously maximized window. When a maximized window gets docked (possible with a secondary monitor - see bug steps to repro) it now gets restored first. This prevents weird animated behavior or hiding it when it gets undocked or restored.
Additionally a tab dragged out of a maximized browser and docked is not maximized anymore at the end of the drag (this used to be causing it to get undocked immediately).

BUG=309954
BUG=305276
TEST=interactive_ui_tests --gtest_filter=*TabDragControllerTest*DetachToDockedWindowFromMaximizedWindow*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231698

[varkha, 2013-10-23 23:54:18]

oshima@, please take a look. The CL prevents a maximized window from staying
maximized when a tab is dragged out and docked (whether it is a single tab being
dragged or just one out of many tabs being dragged). When a tab gets docked it
is no longer "restored" if it is already restored (not minimized). Thanks!

[oshima, 2013-10-24 15:54:50]

can you add test for this?

[varkha, 2013-10-24 21:58:43]

PTAL. I have added a test that docks a maximized tab and verifies that it is no
longer maximized.

[oshima, 2013-10-25 16:12:31]

lgtm with nits

https://codereview.chromium.org/38073004/diff/190001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/38073004/diff/190001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1899: if (host_desktop_type_
== chrome::HOST_DESKTOP_TYPE_ASH &&
I think this check isn't necessary as the paren'ts ID won't match in desktop
environment. Maybe this should be DCHECK in if block.

https://codereview.chromium.org/38073004/diff/190001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/38073004/diff/190001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2043:
DetachToDockedTabDragControllerTest() {}
virtual dtor.

[varkha, 2013-10-25 17:05:03]

+sky@ for OWNERS review. This is a case when dragging a tab from a maximized
browser (or dragging a maximized browser on a dual screen setup by its only tab)
should not result in maximizing the target browser - if it gets docked as a
result of the drag. PTAL.

https://codereview.chromium.org/38073004/diff/190001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/38073004/diff/190001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1899: if (host_desktop_type_
== chrome::HOST_DESKTOP_TYPE_ASH &&
On 2013/10/25 16:12:32, oshima wrote:
> I think this check isn't necessary as the paren'ts ID won't match in desktop
> environment. Maybe this should be DCHECK in if block.

Done.

https://codereview.chromium.org/38073004/diff/190001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/38073004/diff/190001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2043:
DetachToDockedTabDragControllerTest() {}
On 2013/10/25 16:12:32, oshima wrote:
> virtual dtor. 

Done. Here and for other derivatives of DetachToBrowserTabDragControllerTest.

[sky, 2013-10-28 14:30:08]

https://codereview.chromium.org/38073004/diff/260001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/38073004/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1878: if
(attached_tabstrip_->GetWidget()->GetNativeWindow()->parent()->id() ==
Is there a way for these ifdefs to be moved closed to where they are used. What
if RunMoveLoop took a boolean indicating if it wants maximized when released?

[varkha, 2013-10-29 03:08:25]

https://codereview.chromium.org/38073004/diff/260001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/38073004/diff/260001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1878: if
(attached_tabstrip_->GetWidget()->GetNativeWindow()->parent()->id() ==
On 2013/10/28 14:30:08, sky wrote:
> Is there a way for these ifdefs to be moved closer to where they are used.
What
> if RunMoveLoop took a boolean indicating if it wants maximized when released?

I looked into what it would take to move the ash-specific code into
ToplevelWindowEventHandler. I don't think it is practical since EndEventLoop
cannot be used as a reliable marker that the move loop was cancelled. Paths that
cause RunMoveLoop to exit are multiple and triggered both from inside TLWEH and
outside. See how tlweh->EndEventLoop is called in TDC - it is not always in sync
with how TDC::CompleteDrag is called. I also prefer leaving the maximize /
restore of browser tabs in TabDragController.
Instead I have refactored the code so that IsDocked can be called on a Widget
along the lines how IsMaximized and such are implemented. I hope this is OK.
PTAL.

[sky, 2013-10-29 16:36:06]

https://codereview.chromium.org/38073004/diff/400001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/38073004/diff/400001/ui/views/widget/widget.h...
ui/views/widget/widget.h:482: bool IsDocked() const;
Ben is trying to keep ash specific state out of views. Can you move this where
you need it? Same comment on all the changes you're adding to ui/views.

[varkha, 2013-10-29 17:06:17]

https://codereview.chromium.org/38073004/diff/400001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/38073004/diff/400001/ui/views/widget/widget.h...
ui/views/widget/widget.h:482: bool IsDocked() const;
On 2013/10/29 16:36:06, sky wrote:
> Ben is trying to keep ash specific state out of views. Can you move this where
> you need it? Same comment on all the changes you're adding to ui/views.

Not sure I follow. How can I not introduce something to ui/views and at the same
time eliminate ash-specific ifdefs in TabDragController?
I could introduce docked as a property (there is GetNativeWindowProperty) but I
thought you didn't like that approach either.
Or do you mean just moving that chain of invocations to a method in
TabDragController (although then it would need to be ifdefed with USE_ASH)?

[sky, 2013-10-29 17:28:34]

On Tue, Oct 29, 2013 at 10:06 AM,  <varkha@chromium.org> wrote:
>
> https://codereview.chromium.org/38073004/diff/400001/ui/views/widget/widget.h
> File ui/views/widget/widget.h (right):
>
>
https://codereview.chromium.org/38073004/diff/400001/ui/views/widget/widget.h...
> ui/views/widget/widget.h:482: bool IsDocked() const;
> On 2013/10/29 16:36:06, sky wrote:
>>
>> Ben is trying to keep ash specific state out of views. Can you move
>
> this where
>>
>> you need it? Same comment on all the changes you're adding to
>
> ui/views.
>
> Not sure I follow. How can I not introduce something to ui/views and at
> the same time eliminate ash-specific ifdefs in TabDragController?
> I could introduce docked as a property (there is
> GetNativeWindowProperty) but I thought you didn't like that approach
> either.
> Or do you mean just moving that chain of invocations to a method in
> TabDragController (although then it would need to be ifdefed with
> USE_ASH)?

Ya, sorry, the two are a bit conflicting. But yes, I meant the later
(more ifdefs in TDC):(

>
> https://codereview.chromium.org/38073004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[varkha, 2013-10-29 18:28:02]

https://codereview.chromium.org/38073004/diff/400001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/38073004/diff/400001/ui/views/widget/widget.h...
ui/views/widget/widget.h:482: bool IsDocked() const;
On 2013/10/29 16:36:06, sky wrote:
> Ben is trying to keep ash specific state out of views. Can you move this where
> you need it? Same comment on all the changes you're adding to ui/views.

Done.

[sky, 2013-10-29 20:46:41]

LGTM

https://codereview.chromium.org/38073004/diff/500001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/38073004/diff/500001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:212: // Returns true if
|bounds| contains the y-coordinate |y|. The y-coordinate
Move IsDocked here please.

[varkha, 2013-10-29 21:08:31]

https://codereview.chromium.org/38073004/diff/500001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/38073004/diff/500001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:212: // Returns true if
|bounds| contains the y-coordinate |y|. The y-coordinate
On 2013/10/29 20:46:42, sky wrote:
> Move IsDocked here please.

Done.

[commit-bot: I haz the power, 2013-10-29 21:16:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/38073004/560001

[commit-bot: I haz the power, 2013-10-30 01:05:44]

Change committed as 231698