Add window states docked; and docked minimized. Add wm window event to set docked and undocked.

Add window states docked; and docked minimized. Add wm window event to set docked and undocked.

BUG=344597
Committed: https://crrev.com/45f090b7b0892ebf217ce128913854261470b3f7
Cr-Commit-Position: refs/heads/master@{#298070}

[dtapuska, 2014-09-23 21:14:52]

dtapuska@chromium.org changed reviewers:
+ varkha@chromium.org

[varkha, 2014-09-23 22:35:31]

Looks like things are going in the right direction. I'll be looking some more,
that's what I have so far.

https://codereview.chromium.org/597683003/diff/1/ash/wm/default_state.cc
File ash/wm/default_state.cc (right):

https://codereview.chromium.org/597683003/diff/1/ash/wm/default_state.cc#newc...
ash/wm/default_state.cc:81: WindowStateType current =
window_state->GetStateType();
nit: current_state_type would be more expressive now that you've expanded the
scope where it is used.

https://codereview.chromium.org/597683003/diff/1/ash/wm/default_state.cc#newc...
ash/wm/default_state.cc:85: if (current == WINDOW_STATE_TYPE_DOCKED_MINIMIZED) {
nit: Ternary? Here and below.

https://codereview.chromium.org/597683003/diff/1/ash/wm/default_state.cc#newc...
ash/wm/default_state.cc:113: case WM_EVENT_UNDOCK:
Can we make it work without this event type?

https://codereview.chromium.org/597683003/diff/1/ash/wm/default_state.cc#newc...
ash/wm/default_state.cc:119: return;
Does this ever happen (if not - NOTREACHED() would be good)?

https://codereview.chromium.org/597683003/diff/1/ash/wm/dock/docked_window_la...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/1/ash/wm/dock/docked_window_la...
ash/wm/dock/docked_window_layout_manager.cc:838: } else if (old_type ==
wm::WINDOW_STATE_TYPE_DOCKED_MINIMIZED) {
Maybe add NOTREACHED for WINDOW_STATE_TYPE_MINIMIZED if it is impossible now.

https://codereview.chromium.org/597683003/diff/1/ash/wm/dock/docked_window_re...
File ash/wm/dock/docked_window_resizer.cc (right):

https://codereview.chromium.org/597683003/diff/1/ash/wm/dock/docked_window_re...
ash/wm/dock/docked_window_resizer.cc:249: wm::WM_EVENT_UNDOCK);
Would WM_EVENT_NORMAL work just as well as WM_EVENT_UNDOCK (since minimized
windows are not draggable)?

https://codereview.chromium.org/597683003/diff/1/ash/wm/dock/docked_window_re...
ash/wm/dock/docked_window_resizer.cc:250: window_state_->OnWMEvent(&event);
Is this clean in all the cases where MaybeReparentWindowOnDragCompletion does
not dock or undock? Should it be called there?

https://codereview.chromium.org/597683003/diff/1/ash/wm/window_state.cc
File ash/wm/window_state.cc (right):

https://codereview.chromium.org/597683003/diff/1/ash/wm/window_state.cc#newco...
ash/wm/window_state.cc:129: GetStateType() == WINDOW_STATE_TYPE_DOCKED;
This may make the code simpler elsewhere but may be confusing nonetheless.
Consider IsRestored or something like that (since
IsNormalOrDockedButNotDockedMinimized would be too long).

https://codereview.chromium.org/597683003/diff/1/ash/wm/window_state.cc#newco...
ash/wm/window_state.cc:134: GetStateType() == WINDOW_STATE_TYPE_DOCKED;
Is this necessary given that DOCKED is a Normal state?

[dtapuska, 2014-09-24 15:21:13]

https://codereview.chromium.org/597683003/diff/1/ash/wm/dock/docked_window_re...
File ash/wm/dock/docked_window_resizer.cc (right):

https://codereview.chromium.org/597683003/diff/1/ash/wm/dock/docked_window_re...
ash/wm/dock/docked_window_resizer.cc:249: wm::WM_EVENT_UNDOCK);
On 2014/09/23 22:35:31, varkha wrote:
> Would WM_EVENT_NORMAL work just as well as WM_EVENT_UNDOCK (since minimized
> windows are not draggable)?

This is what I originally had. I didn't have the WM_EVENT_UNDOCK but I failed a
test. The test was a drag while hitting a maximize accelerator; the test case
was failing because the drag was cancelled via a maximize but then this code
would execute and would end up setting it as restored; so the bounds were wrong.

I can change this to check the window state I guess; and remove the UNDOCK
event. So what t his would be is something like 
if (is_docked) {
   send WM_EVENT_DOCKED;
} else if (window_state_->IsDocked()) {
   send WM_EVENT_RESTORE;
}

So that would cover the maximize case; because presumably the window state
wouldn't be docked anymore.

https://codereview.chromium.org/597683003/diff/1/ash/wm/window_state.cc
File ash/wm/window_state.cc (right):

https://codereview.chromium.org/597683003/diff/1/ash/wm/window_state.cc#newco...
ash/wm/window_state.cc:134: GetStateType() == WINDOW_STATE_TYPE_DOCKED;
On 2014/09/23 22:35:31, varkha wrote:
> Is this necessary given that DOCKED is a Normal state?

No it isn't needed

[dtapuska, 2014-09-25 19:51:57]

dtapuska@chromium.org changed reviewers:
+ oshima@chromium.org

[varkha, 2014-09-25 22:09:28]

https://codereview.chromium.org/597683003/diff/20001/ash/wm/default_state.cc
File ash/wm/default_state.cc (right):

https://codereview.chromium.org/597683003/diff/20001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:85: if (current_state_type ==
WINDOW_STATE_TYPE_DOCKED_MINIMIZED) {
nit: there are still some nits from the first batch of comments. Generally you
make sure that all the comments are answered in some way, this makes it easier
to review knowing that there is no need to go back and revisit old comments.

https://codereview.chromium.org/597683003/diff/20001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:439: // and remove this hack.
This comment can now be moved to above line 449.

https://codereview.chromium.org/597683003/diff/20001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:451: 
nit: no need for empty line.

https://codereview.chromium.org/597683003/diff/20001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:453: // previousbounds. However, we want to maintain the
restore bounds.
nit: space after 'previous'.

https://codereview.chromium.org/597683003/diff/20001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:526: break;
nit: I suggest merging this with case WINDOW_STATE_TYPE_MINIMIZED below.

https://codereview.chromium.org/597683003/diff/20001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:568: if (state_type_ != WINDOW_STATE_TYPE_MINIMIZED) {
Should this account for the new WINDOW_STATE_DOCKED_MINIMIZED state as well?
Here and in other places where we check for WINDOW_STATE_TYPE_MINIMIZED rather
than IsMinimized?

https://codereview.chromium.org/597683003/diff/20001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/20001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:823:
RecordUmaAction(DOCKED_ACTION_MAXIMIZE, DOCKED_ACTION_SOURCE_UNKNOWN);
I think we would need to record a new event source -
DOCKED_ACTION_SOURCE_KEYBOARD. Here and in other places where docking action may
be a result of a keyboard event. Maybe with an auto-reset that is armed when an
accelerator is dispatched.

https://codereview.chromium.org/597683003/diff/20001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer.cc (right):

https://codereview.chromium.org/597683003/diff/20001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer.cc:260: is_attached_panel);
There is still an outstanding comment here about
MaybeReparentWindowOnDragCompletion that might not be in exact match with the
window_state_->OnWMEvent() above. Is this a concern?

[dtapuska, 2014-09-26 14:17:08]

https://codereview.chromium.org/597683003/diff/1/ash/wm/default_state.cc
File ash/wm/default_state.cc (right):

https://codereview.chromium.org/597683003/diff/1/ash/wm/default_state.cc#newc...
ash/wm/default_state.cc:81: WindowStateType current =
window_state->GetStateType();
On 2014/09/23 22:35:31, varkha wrote:
> nit: current_state_type would be more expressive now that you've expanded the
> scope where it is used.

Acknowledged.

https://codereview.chromium.org/597683003/diff/1/ash/wm/default_state.cc#newc...
ash/wm/default_state.cc:113: case WM_EVENT_UNDOCK:
On 2014/09/23 22:35:31, varkha wrote:
> Can we make it work without this event type?

Done.

https://codereview.chromium.org/597683003/diff/1/ash/wm/default_state.cc#newc...
ash/wm/default_state.cc:119: return;
On 2014/09/23 22:35:31, varkha wrote:
> Does this ever happen (if not - NOTREACHED() would be good)?

Done.

https://codereview.chromium.org/597683003/diff/1/ash/wm/dock/docked_window_la...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/1/ash/wm/dock/docked_window_la...
ash/wm/dock/docked_window_layout_manager.cc:838: } else if (old_type ==
wm::WINDOW_STATE_TYPE_DOCKED_MINIMIZED) {
On 2014/09/23 22:35:31, varkha wrote:
> Maybe add NOTREACHED for WINDOW_STATE_TYPE_MINIMIZED if it is impossible now.

Done.

https://codereview.chromium.org/597683003/diff/1/ash/wm/dock/docked_window_re...
File ash/wm/dock/docked_window_resizer.cc (right):

https://codereview.chromium.org/597683003/diff/1/ash/wm/dock/docked_window_re...
ash/wm/dock/docked_window_resizer.cc:249: wm::WM_EVENT_UNDOCK);
On 2014/09/24 15:21:13, Dave Tapuska wrote:
> On 2014/09/23 22:35:31, varkha wrote:
> > Would WM_EVENT_NORMAL work just as well as WM_EVENT_UNDOCK (since minimized
> > windows are not draggable)?
> 
> This is what I originally had. I didn't have the WM_EVENT_UNDOCK but I failed
a
> test. The test was a drag while hitting a maximize accelerator; the test case
> was failing because the drag was cancelled via a maximize but then this code
> would execute and would end up setting it as restored; so the bounds were
wrong.
> 
> I can change this to check the window state I guess; and remove the UNDOCK
> event. So what t his would be is something like 
> if (is_docked) {
>    send WM_EVENT_DOCKED;
> } else if (window_state_->IsDocked()) {
>    send WM_EVENT_RESTORE;
> }
> 
> So that would cover the maximize case; because presumably the window state
> wouldn't be docked anymore.

Done.

https://codereview.chromium.org/597683003/diff/1/ash/wm/dock/docked_window_re...
ash/wm/dock/docked_window_resizer.cc:249: wm::WM_EVENT_UNDOCK);
On 2014/09/23 22:35:31, varkha wrote:
> Would WM_EVENT_NORMAL work just as well as WM_EVENT_UNDOCK (since minimized
> windows are not draggable)?

Done.

https://codereview.chromium.org/597683003/diff/1/ash/wm/window_state.cc
File ash/wm/window_state.cc (right):

https://codereview.chromium.org/597683003/diff/1/ash/wm/window_state.cc#newco...
ash/wm/window_state.cc:129: GetStateType() == WINDOW_STATE_TYPE_DOCKED;
On 2014/09/23 22:35:31, varkha wrote:
> This may make the code simpler elsewhere but may be confusing nonetheless.
> Consider IsRestored or something like that (since
> IsNormalOrDockedButNotDockedMinimized would be too long).

Done.

https://codereview.chromium.org/597683003/diff/1/ash/wm/window_state.cc#newco...
ash/wm/window_state.cc:134: GetStateType() == WINDOW_STATE_TYPE_DOCKED;
On 2014/09/24 15:21:13, Dave Tapuska wrote:
> On 2014/09/23 22:35:31, varkha wrote:
> > Is this necessary given that DOCKED is a Normal state?
> 
> No it isn't needed

Done.

https://codereview.chromium.org/597683003/diff/1/ash/wm/window_state.cc#newco...
ash/wm/window_state.cc:134: GetStateType() == WINDOW_STATE_TYPE_DOCKED;
On 2014/09/23 22:35:31, varkha wrote:
> Is this necessary given that DOCKED is a Normal state?

Done.

https://codereview.chromium.org/597683003/diff/20001/ash/wm/default_state.cc
File ash/wm/default_state.cc (right):

https://codereview.chromium.org/597683003/diff/20001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:85: if (current_state_type ==
WINDOW_STATE_TYPE_DOCKED_MINIMIZED) {
On 2014/09/25 22:09:28, varkha wrote:
> nit: there are still some nits from the first batch of comments. Generally you
> make sure that all the comments are answered in some way, this makes it easier
> to review knowing that there is no need to go back and revisit old comments.
Sorry I thought I covered them all; but I should Ack mark them as Done; point
taken.

https://codereview.chromium.org/597683003/diff/20001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:439: // and remove this hack.
On 2014/09/25 22:09:28, varkha wrote:
> This comment can now be moved to above line 449.

Done.

https://codereview.chromium.org/597683003/diff/20001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:451: 
On 2014/09/25 22:09:28, varkha wrote:
> nit: no need for empty line.

Done.

https://codereview.chromium.org/597683003/diff/20001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:453: // previousbounds. However, we want to maintain the
restore bounds.
On 2014/09/25 22:09:28, varkha wrote:
> nit: space after 'previous'.

Done.

https://codereview.chromium.org/597683003/diff/20001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:526: break;
On 2014/09/25 22:09:28, varkha wrote:
> nit: I suggest merging this with case WINDOW_STATE_TYPE_MINIMIZED below.

Slightly modified; but adjusted.

https://codereview.chromium.org/597683003/diff/20001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:568: if (state_type_ != WINDOW_STATE_TYPE_MINIMIZED) {
On 2014/09/25 22:09:28, varkha wrote:
> Should this account for the new WINDOW_STATE_DOCKED_MINIMIZED state as well?
> Here and in other places where we check for WINDOW_STATE_TYPE_MINIMIZED rather
> than IsMinimized?

Done.

https://codereview.chromium.org/597683003/diff/20001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/20001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:823:
RecordUmaAction(DOCKED_ACTION_MAXIMIZE, DOCKED_ACTION_SOURCE_UNKNOWN);
On 2014/09/25 22:09:28, varkha wrote:
> I think we would need to record a new event source -
> DOCKED_ACTION_SOURCE_KEYBOARD. Here and in other places where docking action
may
> be a result of a keyboard event. Maybe with an auto-reset that is armed when
an
> accelerator is dispatched.

I've adjusted the logic entry condition since the first state would imply the
next two always being true. And only record the uma action for what it was
originally intended for; the maximize. I presume the snap condition was added
when snap left/snap right was done. And those accelerators actions already are
logged via uma.

https://codereview.chromium.org/597683003/diff/20001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer.cc (right):

https://codereview.chromium.org/597683003/diff/20001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer.cc:260: is_attached_panel);
On 2014/09/25 22:09:28, varkha wrote:
> There is still an outstanding comment here about
> MaybeReparentWindowOnDragCompletion that might not be in exact match with the
> window_state_->OnWMEvent() above. Is this a concern?

I moved the code around; so the event sending is inside the reparenting method.

[varkha, 2014-09-29 18:59:29]

Another batch of comments. The behavior seems much better now.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/default_state.cc
File ash/wm/default_state.cc (right):

https://codereview.chromium.org/597683003/diff/60001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:87: WINDOW_STATE_TYPE_DOCKED : WINDOW_STATE_TYPE_NORMAL;
nit: indent 4.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:95: WINDOW_STATE_TYPE_DOCKED_MINIMIZED :
WINDOW_STATE_TYPE_MINIMIZED;
nit: indent 4.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:449: // and remove this hack.
Maybe rephrase a bit now that the docked state is introduced.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:569: previous_state_type ==
WINDOW_STATE_TYPE_DOCKED_MINIMIZED ||
nit: consider a variable or function to compare to both minimized states now
that this logic is repeated a few times.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:589: aura::Window*
window(dock_container_->children()[i]);
nit: s/window/child and s/query_window/window.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:643: }
Looks like this logic is repeated in SetDockedAlignment. Could you wrap it into
a method?

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:652: alignment_ ==
DOCKED_ALIGNMENT_NONE)
nit: curly braces are recommended and seem to be a common style in Chromium for
multi-line condition
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals).

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_layout_manager.h (right):

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.h:110: void
SetDockedAlignment(DockedAlignment desired_alignment);
nit: Not sure if Set is the right name here since this is not a simple setter.
Also needs a comment describing the parameter(s).

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.h:188: DockedAlignment
CalculateAlignment(const aura::Window* query_window) const;
CalculateAlignment name is already used and is public. I would make the name
explicit, maybe CalculateAlignmentOfWindow(window) and change the parameter to
just |window|. Also needs a comment explaining what it returns and the
parameter(s).

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer.cc (right):

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer.cc:226: if (!window_state_->IsMinimized() &&
!window_state_->IsDocked() &&
Can you comment why this change is necessary?

[dtapuska, 2014-09-29 20:59:57]

https://codereview.chromium.org/597683003/diff/60001/ash/wm/default_state.cc
File ash/wm/default_state.cc (right):

https://codereview.chromium.org/597683003/diff/60001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:87: WINDOW_STATE_TYPE_DOCKED : WINDOW_STATE_TYPE_NORMAL;
On 2014/09/29 18:59:28, varkha wrote:
> nit: indent 4.

Done.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:95: WINDOW_STATE_TYPE_DOCKED_MINIMIZED :
WINDOW_STATE_TYPE_MINIMIZED;
On 2014/09/29 18:59:28, varkha wrote:
> nit: indent 4.

Done.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:449: // and remove this hack.
On 2014/09/29 18:59:28, varkha wrote:
> Maybe rephrase a bit now that the docked state is introduced.

Done.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:569: previous_state_type ==
WINDOW_STATE_TYPE_DOCKED_MINIMIZED ||
On 2014/09/29 18:59:28, varkha wrote:
> nit: consider a variable or function to compare to both minimized states now
> that this logic is repeated a few times.

Done.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:589: aura::Window*
window(dock_container_->children()[i]);
On 2014/09/29 18:59:28, varkha wrote:
> nit: s/window/child and s/query_window/window.

Done.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:643: }
On 2014/09/29 18:59:28, varkha wrote:
> Looks like this logic is repeated in SetDockedAlignment. Could you wrap it
into
> a method?

Done.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:652: alignment_ ==
DOCKED_ALIGNMENT_NONE)
On 2014/09/29 18:59:28, varkha wrote:
> nit: curly braces are recommended and seem to be a common style in Chromium
for
> multi-line condition
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals).

Done.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_layout_manager.h (right):

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.h:110: void
SetDockedAlignment(DockedAlignment desired_alignment);
On 2014/09/29 18:59:28, varkha wrote:
> nit: Not sure if Set is the right name here since this is not a simple setter.
> Also needs a comment describing the parameter(s).

Done.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.h:188: DockedAlignment
CalculateAlignment(const aura::Window* query_window) const;
On 2014/09/29 18:59:28, varkha wrote:
> CalculateAlignment name is already used and is public. I would make the name
> explicit, maybe CalculateAlignmentOfWindow(window) and change the parameter to
> just |window|. Also needs a comment explaining what it returns and the
> parameter(s).

Done.

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer.cc (right):

https://codereview.chromium.org/597683003/diff/60001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer.cc:226: if (!window_state_->IsMinimized() &&
!window_state_->IsDocked() &&
On 2014/09/29 18:59:28, varkha wrote:
> Can you comment why this change is necessary?

Previously docked windows were normal state type windows. What happens here; is
that a window is is docked; it will still end up with these conditions met (not
minimized & not normal state); previously it would have been false because it
would have been true on normal state type.

[varkha, 2014-09-30 20:15:21]

Mostly nits at this point. I think you can ask oshima to review the next patch,
especially from window state model point of view.

https://codereview.chromium.org/597683003/diff/120001/ash/wm/default_state.cc
File ash/wm/default_state.cc (right):

https://codereview.chromium.org/597683003/diff/120001/ash/wm/default_state.cc...
ash/wm/default_state.cc:92: WINDOW_STATE_TYPE_DOCKED : WINDOW_STATE_TYPE_NORMAL;
nit: The line after ? should also be indented +4.

https://codereview.chromium.org/597683003/diff/120001/ash/wm/default_state.cc...
ash/wm/default_state.cc:100: WINDOW_STATE_TYPE_DOCKED_MINIMIZED :
WINDOW_STATE_TYPE_MINIMIZED;
nit: The line after ? should also be indented +4.

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:629: if (desired_alignment !=
DOCKED_ALIGNMENT_NONE &&
Can this part be also checked inside IsDockedAlignmentValid?

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.h (right):

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.h:113: void
RequestDesiredDockedAlignment(DockedAlignment desired_alignment);
From the comment sounds like MaybeSetDesiredDockedAlignment. The comments are
usually written describing what the method does ("Sets...", "Allocates...",
"Calculates...", etc). So in this case it could be "Checks the rules and
possibly updates the docked layout to conform to |desired_alignment|. May not
apply the |desired_alignment| when current shelf alignment conflicts with it.
Never clears the |alignment_|."

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.h:194: DockedAlignment
CalculateDockedAlignmentExcept(const aura::Window* child)
nit: How about CalculateAlignmentExcept?
nit: Calculates...
nit: Avoid passive (e.g. "Returns alignment of docked windows other than the
|child|.")

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.h:197: // Determine if the
|desired_alignment| is applicable taking into account
I think this is IsDockedAlignmentValid.
nits:
s/desired_alignment/alignment
s/Determine/Determines.
s/location of the shelf/shelf alignment.

[dtapuska, 2014-10-01 15:01:34]

https://codereview.chromium.org/597683003/diff/120001/ash/wm/default_state.cc
File ash/wm/default_state.cc (right):

https://codereview.chromium.org/597683003/diff/120001/ash/wm/default_state.cc...
ash/wm/default_state.cc:92: WINDOW_STATE_TYPE_DOCKED : WINDOW_STATE_TYPE_NORMAL;
On 2014/09/30 20:15:19, varkha wrote:
> nit: The line after ? should also be indented +4.

Done.

https://codereview.chromium.org/597683003/diff/120001/ash/wm/default_state.cc...
ash/wm/default_state.cc:100: WINDOW_STATE_TYPE_DOCKED_MINIMIZED :
WINDOW_STATE_TYPE_MINIMIZED;
On 2014/09/30 20:15:19, varkha wrote:
> nit: The line after ? should also be indented +4.

Done.

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:629: if (desired_alignment !=
DOCKED_ALIGNMENT_NONE &&
On 2014/09/30 20:15:19, varkha wrote:
> Can this part be also checked inside IsDockedAlignmentValid?

No it can't be. If it look at the conditionals they are different.

if (desired_alignment == alignment_ ||
    desired_alignment == DOCKED_ALIGNMENT_NONE ||
    alignment_ == DOCKED_ALIGNMENT_NONE) 

isn't the same as

if (desired_alignment != DOCKED_ALIGNMENT_NONE &&
    alignment != DOCKED_ALIGNMENT_NONE &&
    desired_alignment != alignment)

In the first case if the alignment is LEFT, and the desired alignment is RIGHT;
the result is true; in the second it is false. DeMorgan's law doesn't apply
here.

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.h (right):

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.h:113: void
RequestDesiredDockedAlignment(DockedAlignment desired_alignment);
On 2014/09/30 20:15:21, varkha wrote:
> From the comment sounds like MaybeSetDesiredDockedAlignment. The comments are
> usually written describing what the method does ("Sets...", "Allocates...",
> "Calculates...", etc). So in this case it could be "Checks the rules and
> possibly updates the docked layout to conform to |desired_alignment|. May not
> apply the |desired_alignment| when current shelf alignment conflicts with it.
> Never clears the |alignment_|."

Done.

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.h:194: DockedAlignment
CalculateDockedAlignmentExcept(const aura::Window* child)
On 2014/09/30 20:15:19, varkha wrote:
> nit: How about CalculateAlignmentExcept?
> nit: Calculates...
> nit: Avoid passive (e.g. "Returns alignment of docked windows other than the
> |child|.")

Done.

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.h:197: // Determine if the
|desired_alignment| is applicable taking into account
On 2014/09/30 20:15:21, varkha wrote:
> I think this is IsDockedAlignmentValid.
> nits:
> s/desired_alignment/alignment
> s/Determine/Determines.
> s/location of the shelf/shelf alignment.

Done.

[varkha, 2014-10-01 18:16:34]

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:629: if (desired_alignment !=
DOCKED_ALIGNMENT_NONE &&
On 2014/10/01 15:01:33, Dave Tapuska wrote:
> On 2014/09/30 20:15:19, varkha wrote:
> > Can this part be also checked inside IsDockedAlignmentValid?
> 
> No it can't be. If it look at the conditionals they are different.
> 
> if (desired_alignment == alignment_ ||
>     desired_alignment == DOCKED_ALIGNMENT_NONE ||
>     alignment_ == DOCKED_ALIGNMENT_NONE) 
> 
> isn't the same as
> 
> if (desired_alignment != DOCKED_ALIGNMENT_NONE &&
>     alignment != DOCKED_ALIGNMENT_NONE &&
>     desired_alignment != alignment)
> 
> In the first case if the alignment is LEFT, and the desired alignment is
RIGHT;
> the result is true; in the second it is false. DeMorgan's law doesn't apply
> here.
> 
> 
> 

I see. Couldn't you pass pass a second argument (current alignment) into
IsDockedAlignmentValid? You could pass alignment_ or desired_alignment. The
logic is serving the same purpose and so I think putting it in one place will
make mistakes less likely if it needs to be tuned later.

https://codereview.chromium.org/597683003/diff/140001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.h (right):

https://codereview.chromium.org/597683003/diff/140001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.h:191: // Return the alignment of the
docked windows other than the |child|.
nit: Returns

https://codereview.chromium.org/597683003/diff/140001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.h:195: // the shelf location.
nit: s/location/alignment

[varkha, 2014-10-01 18:43:02]

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:629: if (desired_alignment !=
DOCKED_ALIGNMENT_NONE &&
On 2014/10/01 18:16:34, varkha wrote:
> On 2014/10/01 15:01:33, Dave Tapuska wrote:
> > On 2014/09/30 20:15:19, varkha wrote:
> > > Can this part be also checked inside IsDockedAlignmentValid?
> > 
> > No it can't be. If it look at the conditionals they are different.
> > 
> > if (desired_alignment == alignment_ ||
> >     desired_alignment == DOCKED_ALIGNMENT_NONE ||
> >     alignment_ == DOCKED_ALIGNMENT_NONE) 
> > 
> > isn't the same as
> > 
> > if (desired_alignment != DOCKED_ALIGNMENT_NONE &&
> >     alignment != DOCKED_ALIGNMENT_NONE &&
> >     desired_alignment != alignment)
> > 
> > In the first case if the alignment is LEFT, and the desired alignment is
> RIGHT;
> > the result is true; in the second it is false. DeMorgan's law doesn't apply
> > here.
> > 
> > 
> > 
> 
> I see. Couldn't you pass pass a second argument (current alignment) into
> IsDockedAlignmentValid? You could pass alignment_ or desired_alignment. The
> logic is serving the same purpose and so I think putting it in one place will
> make mistakes less likely if it needs to be tuned later.

Right. I see my mistake now. Maybe just add a comment in
MaybeSetDesiredAlignment to explain what we are trying to prevent there.

[dtapuska, 2014-10-01 20:05:08]

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/120001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:629: if (desired_alignment !=
DOCKED_ALIGNMENT_NONE &&
On 2014/10/01 18:43:02, varkha wrote:
> On 2014/10/01 18:16:34, varkha wrote:
> > On 2014/10/01 15:01:33, Dave Tapuska wrote:
> > > On 2014/09/30 20:15:19, varkha wrote:
> > > > Can this part be also checked inside IsDockedAlignmentValid?
> > > 
> > > No it can't be. If it look at the conditionals they are different.
> > > 
> > > if (desired_alignment == alignment_ ||
> > >     desired_alignment == DOCKED_ALIGNMENT_NONE ||
> > >     alignment_ == DOCKED_ALIGNMENT_NONE) 
> > > 
> > > isn't the same as
> > > 
> > > if (desired_alignment != DOCKED_ALIGNMENT_NONE &&
> > >     alignment != DOCKED_ALIGNMENT_NONE &&
> > >     desired_alignment != alignment)
> > > 
> > > In the first case if the alignment is LEFT, and the desired alignment is
> > RIGHT;
> > > the result is true; in the second it is false. DeMorgan's law doesn't
apply
> > > here.
> > > 
> > > 
> > > 
> > 
> > I see. Couldn't you pass pass a second argument (current alignment) into
> > IsDockedAlignmentValid? You could pass alignment_ or desired_alignment. The
> > logic is serving the same purpose and so I think putting it in one place
will
> > make mistakes less likely if it needs to be tuned later.
> 
> Right. I see my mistake now. Maybe just add a comment in
> MaybeSetDesiredAlignment to explain what we are trying to prevent there.

Done.

https://codereview.chromium.org/597683003/diff/140001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.h (right):

https://codereview.chromium.org/597683003/diff/140001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.h:191: // Return the alignment of the
docked windows other than the |child|.
On 2014/10/01 18:16:34, varkha wrote:
> nit: Returns

Done.

https://codereview.chromium.org/597683003/diff/140001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.h:195: // the shelf location.
On 2014/10/01 18:16:34, varkha wrote:
> nit: s/location/alignment

Done.

[varkha, 2014-10-01 20:33:14]

One comment. Let me know if you want to address it here or in
https://codereview.chromium.org/594383002/.

https://codereview.chromium.org/597683003/diff/160001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/160001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:852:
RecordUmaAction(DOCKED_ACTION_MAXIMIZE, DOCKED_ACTION_SOURCE_UNKNOWN);
Should probably record some other action in UMA (probably DOCKED_ACTION_UNDOCK)
when it is just undock and not DOCKED_ACTION_MAXIMIZE. Note that docked->snapped
state transition was recorded in UMA before as DOCKED_ACTION_MAXIMIZE action.
I am not sure if this belongs in this CL or the one that adds accelerators - up
to you.

[oshima, 2014-10-01 23:48:57]

lgtm once you get approvael from varkha, with a nit and q.

https://codereview.chromium.org/597683003/diff/160001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/160001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:640: ShelfAlignment shelf_alignment
= SHELF_ALIGNMENT_BOTTOM;
nit:

could you please change to
shelf_alignment = shelf_ ? shelf_->slignment() : XXX_BOTTOM?

https://codereview.chromium.org/597683003/diff/160001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:733: if (visible &&
window_state->IsMinimized())
in which state (other than minimized) the window can be re-shown?

[dtapuska, 2014-10-02 21:05:29]

https://codereview.chromium.org/597683003/diff/160001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/160001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:640: ShelfAlignment shelf_alignment
= SHELF_ALIGNMENT_BOTTOM;
On 2014/10/01 23:48:57, oshima wrote:
> nit:
> 
> could you please change to
> shelf_alignment = shelf_ ? shelf_->slignment() : XXX_BOTTOM?

Done.

https://codereview.chromium.org/597683003/diff/160001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:733: if (visible &&
window_state->IsMinimized())
On 2014/10/01 23:48:57, oshima wrote:
> in which state (other than minimized) the window can be re-shown?

When restoring a app/panel (not a browser window); the window is restored first
by some other code; and if this code executes (ie; Restore()) it will undocks
the window; so you end up seeing that restoring panels doesn't work. So the
safeguard is that we only want to restore minimized windows here.

[oshima, 2014-10-02 21:36:50]

lgtm

https://codereview.chromium.org/597683003/diff/180001/ash/wm/default_state.cc
File ash/wm/default_state.cc (right):

https://codereview.chromium.org/597683003/diff/180001/ash/wm/default_state.cc...
ash/wm/default_state.cc:521: }
can you add comment why this should return instead of pass through to the reset?

[dtapuska, 2014-10-02 21:43:41]

https://codereview.chromium.org/597683003/diff/180001/ash/wm/default_state.cc
File ash/wm/default_state.cc (right):

https://codereview.chromium.org/597683003/diff/180001/ash/wm/default_state.cc...
ash/wm/default_state.cc:521: }
On 2014/10/02 21:36:49, oshima wrote:
> can you add comment why this should return instead of pass through to the
reset?

Done.

[varkha, 2014-10-02 22:03:12]

A nit and a question about UMA in the previous patch
(https://codereview.chromium.org/597683003/diff/160001/ash/wm/dock/docked_wind...).

https://codereview.chromium.org/597683003/diff/200001/ash/wm/default_state.cc
File ash/wm/default_state.cc (right):

https://codereview.chromium.org/597683003/diff/200001/ash/wm/default_state.cc...
ash/wm/default_state.cc:35: state_type == WINDOW_STATE_TYPE_DOCKED_MINIMIZED);
nit: drop brackets and align with state_type above.

[dtapuska, 2014-10-03 15:45:17]

https://codereview.chromium.org/597683003/diff/160001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/597683003/diff/160001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:852:
RecordUmaAction(DOCKED_ACTION_MAXIMIZE, DOCKED_ACTION_SOURCE_UNKNOWN);
On 2014/10/01 20:33:14, varkha wrote:
> Should probably record some other action in UMA (probably
DOCKED_ACTION_UNDOCK)
> when it is just undock and not DOCKED_ACTION_MAXIMIZE. Note that
docked->snapped
> state transition was recorded in UMA before as DOCKED_ACTION_MAXIMIZE action.
> I am not sure if this belongs in this CL or the one that adds accelerators -
up
> to you.

Done.

https://codereview.chromium.org/597683003/diff/200001/ash/wm/default_state.cc
File ash/wm/default_state.cc (right):

https://codereview.chromium.org/597683003/diff/200001/ash/wm/default_state.cc...
ash/wm/default_state.cc:35: state_type == WINDOW_STATE_TYPE_DOCKED_MINIMIZED);
On 2014/10/02 22:03:12, varkha wrote:
> nit: drop brackets and align with state_type above.

clang requires brackets for a function body; but indent is done.

The indent rules are unclear to me.. Seems generally indent 4 for 
compound statements; but why would that rule not be applied here? 

ie; is it to indent at least 4 with the caveat of making the code more readable?
I've done both indent 4, and indent all the way and be told the converse of what
I've done each time.

[varkha, 2014-10-03 16:14:05]

lgtm with a nit about indentation / parenthesis.

https://codereview.chromium.org/597683003/diff/200001/ash/wm/default_state.cc
File ash/wm/default_state.cc (right):

https://codereview.chromium.org/597683003/diff/200001/ash/wm/default_state.cc...
ash/wm/default_state.cc:35: state_type == WINDOW_STATE_TYPE_DOCKED_MINIMIZED);
On 2014/10/03 15:45:17, Dave Tapuska wrote:
> On 2014/10/02 22:03:12, varkha wrote:
> > nit: drop brackets and align with state_type above.
> 
> clang requires brackets for a function body; but indent is done.
> 
> The indent rules are unclear to me.. Seems generally indent 4 for 
> compound statements; but why would that rule not be applied here? 
> 
> ie; is it to indent at least 4 with the caveat of making the code more
readable?
> I've done both indent 4, and indent all the way and be told the converse of
what
> I've done each time.

+4 indent would probably be technically correct as well but more code prefers
aligning conditionals one under the other. Not sure about the parentheses for
return - I meant like in line 104 of window_state.cc in this CL.
https://codereview.chromium.org/597683003/diff/220001/ash/wm/window_state.cc

This is probably borderline and you are correct that there are example both
ways.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Return_Values.

I still think that:
return a_long_but_simple_expression ||
       another_long_but_simple_expression;
looks better than
return (a_long_but_simple_expression ||
        another_long_but_simple_expression);
or
return a_long_but_simple_expression ||
    another_long_but_simple_expression;

And yes, style guide leaves a lot of room for free will ;)

[dtapuska, 2014-10-03 18:53:55]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2014-10-03 18:54:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/597683003/260001

[commit-bot: I haz the power, 2014-10-03 18:58:43]

Committed patchset #14 (id:260001) as 691d5a882c09d79f9553a9d21643e094f1286bb8

[commit-bot: I haz the power, 2014-10-03 18:59:30]

Patchset 14 (id:??) landed as
https://crrev.com/45f090b7b0892ebf217ce128913854261470b3f7
Cr-Commit-Position: refs/heads/master@{#298070}