[Refactor] Move the logic to update bounds for show type from WorkspaceLayoutManager to DefaultState

Move the logic to update bounds for show type from WorkspaceLayoutManager to DefaultState

* Introduced WindowStateObserver::{Pre|Post}WindowShowTypeChange
 We had implicit dependency between OnWindowShowTypeChanged implementations. This clearly separate the things that should happen before and after the window's bounds is updated.

This is another step to introduce state machine.

BUG=318325
TEST=no functional change. all tests should pass.
TBR=benwells@chromium.org

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

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

[oshima, 2014-02-11 21:49:34]

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

https://codereview.chromium.org/149303003/diff/30001/ash/wm/default_state.cc#...
ash/wm/default_state.cc:237: void
DefaultState::UpdateBoundsFromShowType(wm::WindowState* window_state,
The code that updates window state in OnWindowShowTypeChanged has been merge and
moved to here.

[pkotwicz, 2014-02-12 05:10:00]

https://codereview.chromium.org/149303003/diff/170001/ash/wm/panels/panel_lay...
File ash/wm/panels/panel_layout_manager.cc (right):

https://codereview.chromium.org/149303003/diff/170001/ash/wm/panels/panel_lay...
ash/wm/panels/panel_layout_manager.cc:462: void
PanelLayoutManager::OnPreWindowShowTypeChange(
Given my suggestion about keeping WorkspaceLayoutManager::ShowTypeChanged() in
WorkspaceLayoutManager can't this be OnPostWindowShowTypeChanged() instead?

https://codereview.chromium.org/149303003/diff/170001/ash/wm/window_state.h
File ash/wm/window_state.h (right):

https://codereview.chromium.org/149303003/diff/170001/ash/wm/window_state.h#n...
ash/wm/window_state.h:292: // TODO(oshima): Remove more logic from WLM and this.
Nit: Can you rephrase the TODO? I do not understand what the TODO is for.

https://codereview.chromium.org/149303003/diff/170001/ash/wm/window_state_obs...
File ash/wm/window_state_observer.h (right):

https://codereview.chromium.org/149303003/diff/170001/ash/wm/window_state_obs...
ash/wm/window_state_observer.h:27: WindowShowType old_type) {}
I wonder whether it is possible to call OnPreWindowShowTypeChange() before the
window show type is changed. You will need to undock docked windows in
ToplevelWindowEventHandler::ScopedWindowResizerOnPreWindowShowTypeChange() which
is slightly ugly.
(In my opinion, the reason that you need to undock docked windows in
ToplevelWindowEventHandler::ScopedWindowResizerOnPreWindowShowTypeChange() is
because WindowState::is_dragged() is computed suboptimally.)

https://codereview.chromium.org/149303003/diff/170001/ash/wm/wm_types.h
File ash/wm/wm_types.h (right):

https://codereview.chromium.org/149303003/diff/170001/ash/wm/wm_types.h#newco...
ash/wm/wm_types.h:47: // restore operation with a few exceptions.
How about: "A window has requsted the normal show type. This usually involves
..."

https://codereview.chromium.org/149303003/diff/170001/ash/wm/wm_types.h#newco...
ash/wm/wm_types.h:50: // A window is requested to become a maximized window.
How about: "A window has requested to become maximized." Similar pattern for the
others below.

https://codereview.chromium.org/149303003/diff/170001/ash/wm/wm_types.h#newco...
ash/wm/wm_types.h:62: SNAP_RIGHT,
I wonder whether the compound event types are worthy of being WMEvents.
I think that they can be replaced with just
VERTICAL_MAXIMIZE
HORIZONTAL_MAXIMIZE

and that you should be able to demote TOGGLE_MAXIMIZE_CAPTION, TOGGLE_MAXIMIZE
and TOGGLE_FULLSCREEN from WMEvent-hood

https://codereview.chromium.org/149303003/diff/170001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (left):

https://codereview.chromium.org/149303003/diff/170001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:296: wm::WindowShowType
last_show_type) {
For the sake of this CL, it makes sense to keep
WorkspaceLayoutManager::ShowTypeChanged() in WorkspaceLayoutManager.

Some of the code in ShowTypeChanged() is duplicated in DockedWindowLayoutManager
and PanelLayoutManager()
e.g. the call to window->Hide()

The animations used for panels, docked windows and normal windows are different.
It is a matter of luck that calling
views::corewm::SetWindowVisibilityAnimationType(
    window_state->window(), 
    WINDOW_VISIBILITY_ANIMATION_TYPE_MINIMIZE);
for panels and docked windows does not regress the panel and docked window
animations

[pkotwicz, 2014-02-12 05:44:48]

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

https://codereview.chromium.org/149303003/diff/170001/ash/wm/default_state.cc...
ash/wm/default_state.cc:112: // TODO(oshima): Make docked window a state.
I don't think that making docked window a state will be useful. The bounds of a
docked window depend on whether any other windows are docked.

https://codereview.chromium.org/149303003/diff/170001/ash/wm/default_state.cc...
ash/wm/default_state.cc:267: gfx::Rect work_area_in_parent =
Nit: Move |work_area_in_parent| closer to where it is used (within the switch
statement)

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

https://codereview.chromium.org/149303003/diff/170001/ash/wm/window_state.cc#...
ash/wm/window_state.cc:34: // manager with proper friendship.
Can you move adding this class as part of a different CL?

https://codereview.chromium.org/149303003/diff/170001/ash/wm/wm_types.h
File ash/wm/wm_types.h (right):

https://codereview.chromium.org/149303003/diff/170001/ash/wm/wm_types.h#newco...
ash/wm/wm_types.h:79: // A user requested to toggle fullscreen state.
No one seems to use TOGGLE_FULLSCREEN yet. Is there a reason that you are adding
it in this CL?

[oshima, 2014-02-12 14:08:33]

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

https://codereview.chromium.org/149303003/diff/170001/ash/wm/default_state.cc...
ash/wm/default_state.cc:112: // TODO(oshima): Make docked window a state.
On 2014/02/12 05:44:48, pkotwicz wrote:
> I don't think that making docked window a state will be useful. The bounds of
a
> docked window depend on whether any other windows are docked.

It may not look useful right now, but it will be very useful. It'll make it very
clear what should happen in docked state. For example, docked window behaves
weird when you set bounds to docked window.

Having a state will make it easy to address all scenarios.

https://codereview.chromium.org/149303003/diff/170001/ash/wm/default_state.cc...
ash/wm/default_state.cc:267: gfx::Rect work_area_in_parent =
On 2014/02/12 05:44:48, pkotwicz wrote:
> Nit: Move |work_area_in_parent| closer to where it is used (within the switch
> statement)

Done.

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

https://codereview.chromium.org/149303003/diff/170001/ash/wm/window_state.cc#...
ash/wm/window_state.cc:34: // manager with proper friendship.
On 2014/02/12 05:44:48, pkotwicz wrote:
> Can you move adding this class as part of a different CL?

Sorry, I didn't understand. Can you elaborate?

https://codereview.chromium.org/149303003/diff/170001/ash/wm/window_state.h
File ash/wm/window_state.h (right):

https://codereview.chromium.org/149303003/diff/170001/ash/wm/window_state.h#n...
ash/wm/window_state.h:292: // TODO(oshima): Remove more logic from WLM and this.
On 2014/02/12 05:10:00, pkotwicz wrote:
> Nit: Can you rephrase the TODO? I do not understand what the TODO is for.

Done.

https://codereview.chromium.org/149303003/diff/170001/ash/wm/window_state_obs...
File ash/wm/window_state_observer.h (right):

https://codereview.chromium.org/149303003/diff/170001/ash/wm/window_state_obs...
ash/wm/window_state_observer.h:27: WindowShowType old_type) {}
On 2014/02/12 05:10:00, pkotwicz wrote:
> I wonder whether it is possible to call OnPreWindowShowTypeChange() before the
> window show type is changed. You will need to undock docked windows in
> ToplevelWindowEventHandler::ScopedWindowResizerOnPreWindowShowTypeChange()
which
> is slightly ugly.
> (In my opinion, the reason that you need to undock docked windows in
> ToplevelWindowEventHandler::ScopedWindowResizerOnPreWindowShowTypeChange() is
> because WindowState::is_dragged() is computed suboptimally.)

I thought about it, but I figured that's requires more changes, and decided no
to change this behavior right now. We can revisit this after refactoring.

https://codereview.chromium.org/149303003/diff/170001/ash/wm/wm_types.h
File ash/wm/wm_types.h (right):

https://codereview.chromium.org/149303003/diff/170001/ash/wm/wm_types.h#newco...
ash/wm/wm_types.h:50: // A window is requested to become a maximized window.
On 2014/02/12 05:10:00, pkotwicz wrote:
> How about: "A window has requested to become maximized." Similar pattern for
the
> others below.

Did you mean "A window has been requested" ? A window itself does not request to
be a minimized window.

https://codereview.chromium.org/149303003/diff/170001/ash/wm/wm_types.h#newco...
ash/wm/wm_types.h:62: SNAP_RIGHT,
On 2014/02/12 05:10:00, pkotwicz wrote:
> I wonder whether the compound event types are worthy of being WMEvents.
> I think that they can be replaced with just
> VERTICAL_MAXIMIZE
> HORIZONTAL_MAXIMIZE
> 
> and that you should be able to demote TOGGLE_MAXIMIZE_CAPTION, TOGGLE_MAXIMIZE
> and TOGGLE_FULLSCREEN from WMEvent-hood

This is necessary to implement state machine, where the condition will be
handled in each state.

https://codereview.chromium.org/149303003/diff/170001/ash/wm/wm_types.h#newco...
ash/wm/wm_types.h:79: // A user requested to toggle fullscreen state.
On 2014/02/12 05:44:48, pkotwicz wrote:
> No one seems to use TOGGLE_FULLSCREEN yet. Is there a reason that you are
adding
> it in this CL?

Thank you for the catch. It's an error when I split the CL.
WindowState::ToggleFullscreen() now uses this.

https://codereview.chromium.org/149303003/diff/170001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (left):

https://codereview.chromium.org/149303003/diff/170001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:296: wm::WindowShowType
last_show_type) {
On 2014/02/12 05:10:00, pkotwicz wrote:
> For the sake of this CL, it makes sense to keep
> WorkspaceLayoutManager::ShowTypeChanged() in WorkspaceLayoutManager.
> 
> Some of the code in ShowTypeChanged() is duplicated in
DockedWindowLayoutManager
> and PanelLayoutManager()
> e.g. the call to window->Hide()
> 
> The animations used for panels, docked windows and normal windows are
different.
> It is a matter of luck that calling
> views::corewm::SetWindowVisibilityAnimationType(
>     window_state->window(), 
>     WINDOW_VISIBILITY_ANIMATION_TYPE_MINIMIZE);
> for panels and docked windows does not regress the panel and docked window
> animations

Thank you for the catch. I added panel check in DefaultState which should
address your concern.
Docked window is already excluded.

[pkotwicz, 2014-02-12 18:20:17]

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

https://codereview.chromium.org/149303003/diff/170001/ash/wm/window_state.cc#...
ash/wm/window_state.cc:34: // manager with proper friendship.
Can you remove this class from this CL?

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

https://codereview.chromium.org/149303003/diff/280001/ash/wm/default_state.cc...
ash/wm/default_state.cc:112: // TODO(oshima): Make docked window a state.
Is there a plan to make panel windows and docked windows use more of
UpdateBoundsFromShowType()?

[oshima, 2014-02-12 19:56:16]

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

https://codereview.chromium.org/149303003/diff/170001/ash/wm/window_state.cc#...
ash/wm/window_state.cc:34: // manager with proper friendship.
On 2014/02/12 18:20:18, pkotwicz wrote:
> Can you remove this class from this CL?

Sorry I still didn't understand. This is used in 402 below.
If you want me to just a new method to WLM and expose it, let me know. (I was
uncomfortable to expose WLM to this class, but if you do not like this, I'm
fine)

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

https://codereview.chromium.org/149303003/diff/280001/ash/wm/default_state.cc...
ash/wm/default_state.cc:112: // TODO(oshima): Make docked window a state.
On 2014/02/12 18:20:18, pkotwicz wrote:
> Is there a plan to make panel windows and docked windows use more of
> UpdateBoundsFromShowType()?

The plan is to replace DefaultState with state machine.

There will be a class for each state, and implement the behavior for WM events.
The goal is to eliminate IsSnapped, IsFullscreen (and other ifs) and makes it
clear what should happen in which state.

[pkotwicz, 2014-02-12 22:27:27]

LGTM with nits

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

https://codereview.chromium.org/149303003/diff/170001/ash/wm/window_state.cc#...
ash/wm/window_state.cc:34: // manager with proper friendship.
I see now

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

https://codereview.chromium.org/149303003/diff/360001/ash/wm/default_state.cc...
ash/wm/default_state.cc:113: if (!window_state->IsDocked() &&
!window_state->IsPanel())
Can you move the implementation of IsPanel() to the anonymous namespace?

I do not want WindowState::IsPanel() to be used by mistake

https://codereview.chromium.org/149303003/diff/360001/ash/wm/default_state.cc...
ash/wm/default_state.cc:237: void
DefaultState::UpdateBoundsFromShowType(wm::WindowState* window_state,
Nit: You can remove the wm:: namespace prefixes

https://codereview.chromium.org/149303003/diff/360001/ash/wm/default_state.cc...
ash/wm/default_state.cc:326: if (old_show_type == wm::SHOW_TYPE_MINIMIZED)
Lines 326 and 327 should be gone

https://codereview.chromium.org/149303003/diff/360001/ash/wm/default_state.h
File ash/wm/default_state.h (right):

https://codereview.chromium.org/149303003/diff/360001/ash/wm/default_state.h#...
ash/wm/default_state.h:23: // Process a stete dependent events, such as
TOGGLE_MAXIMZIED,
"a stete" -> "state"
TOGGLE_MAXIMZIED -> TOGGLE_MAXIMIZED

https://codereview.chromium.org/149303003/diff/360001/ash/wm/default_state.h#...
ash/wm/default_state.h:27: // Update the window bounds according to the current
and previous show type.
How about: "Animate to new window bounds based on the current and previous show
type."

https://codereview.chromium.org/149303003/diff/360001/ash/wm/panels/panel_lay...
File ash/wm/panels/panel_layout_manager.cc (right):

https://codereview.chromium.org/149303003/diff/360001/ash/wm/panels/panel_lay...
ash/wm/panels/panel_layout_manager.cc:462: void
PanelLayoutManager::OnPreWindowShowTypeChange(
Can't this be OnPostWindowShowTypeChange()

https://codereview.chromium.org/149303003/diff/360001/ash/wm/toplevel_window_...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/149303003/diff/360001/ash/wm/toplevel_window_...
ash/wm/toplevel_window_event_handler.cc:108: wm::WindowShowType type) OVERRIDE;
Nit: extra space

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

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state.cc#...
ash/wm/window_state.cc:33: // TODO(oshima): Once all logic are cleaned up, move
this to the real layout
Nit: all logic are cleaned up -> all logic is cleaned up

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state.cc#...
ash/wm/window_state.cc:321: if (is_dragged() || !IsSnapped())
AdjustSnappedBounds() is suboptimal but is ok for the sake of this CL

WindowState should have a method to get the snapped bounds for a window of a
given width. It should not have AdjustSnappedBounds().

(Similar to the distinction between
WorkspaceLayoutManager::SetMaximizedOrFullscreenBounds() and
ScreenUtil::GetMaximizedWindowBoundsInParent() )

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state.cc#...
ash/wm/window_state.cc:377: base::AutoReset<bool>
resetter(&ignore_property_change_, true);
Can you move |resetter| within the if statement?

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state.h
File ash/wm/window_state.h (right):

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state.h#n...
ash/wm/window_state.h:338: // True to ignore a property change event to avoid
reentrance.
How about: "True to ignore a property change event to avoid reentrance in
UpdateWindowShowType()."

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state_obs...
File ash/wm/window_state_observer.h (right):

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state_obs...
ash/wm/window_state_observer.h:18: // property change as this will be invoked
when the window
this -> they

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state_obs...
ash/wm/window_state_observer.h:25: // that the window can use the correct
environment to update its bounds.
Can you please add a TODO to remove this method once docked windows have their
own state

I am ok with the code as is if and only if the next CL you write puts docked
windows into their own state

https://codereview.chromium.org/149303003/diff/360001/ash/wm/wm_types.h
File ash/wm/wm_types.h (right):

https://codereview.chromium.org/149303003/diff/360001/ash/wm/wm_types.h#newco...
ash/wm/wm_types.h:47: // restore operation with a few exceptions.
I do not think that the comment:  "A window is requested to become a normal
state" is a useful. Can you remove the sentence. (I think the second sentence is
still useful)

Can you also remove the comments for MAXIMIZE, MINIMIZE, FULLSCREEN, SNAP_LEFT,
SNAP_RIGHT events too (Keep the TODO)?

[oshima, 2014-02-13 14:52:33]

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

https://codereview.chromium.org/149303003/diff/360001/ash/wm/default_state.cc...
ash/wm/default_state.cc:113: if (!window_state->IsDocked() &&
!window_state->IsPanel())
On 2014/02/12 22:27:27, pkotwicz wrote:
> Can you move the implementation of IsPanel() to the anonymous namespace?
> 
> I do not want WindowState::IsPanel() to be used by mistake

sure done. Note that IsDocked is also wrong, and I'll address it in later.

https://codereview.chromium.org/149303003/diff/360001/ash/wm/default_state.cc...
ash/wm/default_state.cc:237: void
DefaultState::UpdateBoundsFromShowType(wm::WindowState* window_state,
On 2014/02/12 22:27:27, pkotwicz wrote:
> Nit: You can remove the wm:: namespace prefixes

removed all wm::. Thanks

https://codereview.chromium.org/149303003/diff/360001/ash/wm/default_state.cc...
ash/wm/default_state.cc:326: if (old_show_type == wm::SHOW_TYPE_MINIMIZED)
On 2014/02/12 22:27:27, pkotwicz wrote:
> Lines 326 and 327 should be gone

Done.

https://codereview.chromium.org/149303003/diff/360001/ash/wm/default_state.h
File ash/wm/default_state.h (right):

https://codereview.chromium.org/149303003/diff/360001/ash/wm/default_state.h#...
ash/wm/default_state.h:23: // Process a stete dependent events, such as
TOGGLE_MAXIMZIED,
On 2014/02/12 22:27:27, pkotwicz wrote:
> "a stete" -> "state"
> TOGGLE_MAXIMZIED -> TOGGLE_MAXIMIZED

Done.

https://codereview.chromium.org/149303003/diff/360001/ash/wm/default_state.h#...
ash/wm/default_state.h:27: // Update the window bounds according to the current
and previous show type.
On 2014/02/12 22:27:27, pkotwicz wrote:
> How about: "Animate to new window bounds based on the current and previous
show
> type."

Done.

https://codereview.chromium.org/149303003/diff/360001/ash/wm/panels/panel_lay...
File ash/wm/panels/panel_layout_manager.cc (right):

https://codereview.chromium.org/149303003/diff/360001/ash/wm/panels/panel_lay...
ash/wm/panels/panel_layout_manager.cc:462: void
PanelLayoutManager::OnPreWindowShowTypeChange(
On 2014/02/12 22:27:27, pkotwicz wrote:
> Can't this be OnPostWindowShowTypeChange()

Done.

https://codereview.chromium.org/149303003/diff/360001/ash/wm/toplevel_window_...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/149303003/diff/360001/ash/wm/toplevel_window_...
ash/wm/toplevel_window_event_handler.cc:108: wm::WindowShowType type) OVERRIDE;
On 2014/02/12 22:27:27, pkotwicz wrote:
> Nit: extra space

Done.

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

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state.cc#...
ash/wm/window_state.cc:33: // TODO(oshima): Once all logic are cleaned up, move
this to the real layout
On 2014/02/12 22:27:27, pkotwicz wrote:
> Nit: all logic are cleaned up -> all logic is cleaned up

Done.

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state.cc#...
ash/wm/window_state.cc:321: if (is_dragged() || !IsSnapped())
On 2014/02/12 22:27:27, pkotwicz wrote:
> AdjustSnappedBounds() is suboptimal but is ok for the sake of this CL
> 
> WindowState should have a method to get the snapped bounds for a window of a
> given width. It should not have AdjustSnappedBounds().
> 
> (Similar to the distinction between
> WorkspaceLayoutManager::SetMaximizedOrFullscreenBounds() and
> ScreenUtil::GetMaximizedWindowBoundsInParent() )

yes this is temporal. This method should not be exposed. I'll make it
internal/private once i remove all calls outside.

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state.cc#...
ash/wm/window_state.cc:377: base::AutoReset<bool>
resetter(&ignore_property_change_, true);
On 2014/02/12 22:27:27, pkotwicz wrote:
> Can you move |resetter| within the if statement?

Done.

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state.h
File ash/wm/window_state.h (right):

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state.h#n...
ash/wm/window_state.h:338: // True to ignore a property change event to avoid
reentrance.
On 2014/02/12 22:27:27, pkotwicz wrote:
> How about: "True to ignore a property change event to avoid reentrance in
> UpdateWindowShowType()."

Done.

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state_obs...
File ash/wm/window_state_observer.h (right):

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state_obs...
ash/wm/window_state_observer.h:18: // property change as this will be invoked
when the window
On 2014/02/12 22:27:27, pkotwicz wrote:
> this -> they

Done.

https://codereview.chromium.org/149303003/diff/360001/ash/wm/window_state_obs...
ash/wm/window_state_observer.h:25: // that the window can use the correct
environment to update its bounds.
On 2014/02/12 22:27:27, pkotwicz wrote:
> Can you please add a TODO to remove this method once docked windows have their
> own state
> 
> I am ok with the code as is if and only if the next CL you write puts docked
> windows into their own state

Added TODO. It requires a couple of more refactoring CL but will add it as soon
as possible.

https://codereview.chromium.org/149303003/diff/360001/ash/wm/wm_types.h
File ash/wm/wm_types.h (right):

https://codereview.chromium.org/149303003/diff/360001/ash/wm/wm_types.h#newco...
ash/wm/wm_types.h:47: // restore operation with a few exceptions.
On 2014/02/12 22:27:27, pkotwicz wrote:
> I do not think that the comment:  "A window is requested to become a normal
> state" is a useful. Can you remove the sentence. (I think the second sentence
is
> still useful)
> 
> Can you also remove the comments for MAXIMIZE, MINIMIZE, FULLSCREEN,
SNAP_LEFT,
> SNAP_RIGHT events too (Keep the TODO)?

Removed individual comments, and added the overview of these events.

[oshima, 2014-02-13 14:53:48]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-02-13 14:54:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/149303003/570001

[commit-bot: I haz the power, 2014-02-13 15:35:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 15:35:57]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[oshima, 2014-02-13 16:02:42]

tbr'ing benwells@chromium.org for trivial renaming in
chrome/browser/ui/views/apps

[oshima, 2014-02-13 16:02:50]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-02-13 16:03:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/149303003/570001

[commit-bot: I haz the power, 2014-02-13 16:42:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 16:42:33]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[oshima, 2014-02-13 17:07:03]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-02-13 17:07:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/149303003/780003

[commit-bot: I haz the power, 2014-02-13 18:17:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 18:17:38]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, check_deps2git, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, events_unittests, google_apis_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[oshima, 2014-02-13 19:17:45]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-02-13 19:21:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/149303003/990002

[commit-bot: I haz the power, 2014-02-13 20:03:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/149303003/990002

[commit-bot: I haz the power, 2014-02-13 21:38:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 21:38:55]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[oshima, 2014-02-13 22:06:56]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-02-13 22:08:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/149303003/990002

[commit-bot: I haz the power, 2014-02-13 23:50:58]

Change committed as 251191

[oshima, 2014-02-14 20:10:36]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-02-14 20:11:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/149303003/1490001

[commit-bot: I haz the power, 2014-02-14 20:50:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 20:50:52]

Try jobs failed on following builders: linux_chromium_rel

[oshima, 2014-02-14 22:44:03]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-02-14 22:52:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/149303003/1490001

[commit-bot: I haz the power, 2014-02-14 22:59:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 22:59:57]

Try jobs failed on following builders: linux_chromium_rel

[oshima, 2014-02-15 00:19:36]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-02-15 00:36:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/149303003/1490001

[commit-bot: I haz the power, 2014-02-15 00:56:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-15 00:56:49]

Try jobs failed on following builders: linux_chromium_rel

[oshima, 2014-02-15 03:42:07]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-02-15 03:42:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/149303003/1490001

[commit-bot: I haz the power, 2014-02-15 05:31:13]

Change committed as 251539