Stick windows to sides of workspaces

Stick windows to side of workspaces

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

[varkha, 2013-05-01 15:49:17]

Looping in flackr@.

[flackr, 2013-05-01 17:34:04]

https://codereview.chromium.org/13896026/diff/2001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/13896026/diff/2001/ash/ash_switches.cc#newcode81
ash/ash_switches.cc:81: // Enable the gutter area on a desktop.
We might want a nicer name for this. Enable docked window?

https://codereview.chromium.org/13896026/diff/2001/ash/wm/window_properties.h
File ash/wm/window_properties.h (right):

https://codereview.chromium.org/13896026/diff/2001/ash/wm/window_properties.h...
ash/wm/window_properties.h:101: // A bitmask containing the workspace edges that
the window is stuck to.
I don't think we'll have any useful behaviors for windows being "stuck" to more
than one edge. We should probably only remember the right or left and perhaps
neither if the window is touching both.

https://codereview.chromium.org/13896026/diff/2001/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/2001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager.cc:366: stuck_edges_mask =
CalculateStuckEdges(window);
We probably only want to "stick" windows when they're being moved in
workspace_window_resizer.cc. Doing so for all bounds updates would allow for
confusing behaviors like a restored window creating a gutter or creating a
gutter after a resize.

https://codereview.chromium.org/13896026/diff/2001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager.cc:427: if ((0 !=
(GetStuckToEdge(window) & EDGE_RIGHT) &&
why the 0 !=? This should be fine as just if ((GetStuckToEdge(window) &
EDGE_RIGHT) && ...

https://codereview.chromium.org/13896026/diff/2001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager.cc:428: bounds.right() !=
work_area_.right()) ||
These seem like they should be handled separately as they don't share much code.
i.e. rather than
if (A || B) {
  if (A) {

  } else { // B

}
just:
if (A) {

} else if (B) {

}

https://codereview.chromium.org/13896026/diff/2001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager.cc:437: } else {
} else if (X) {
  ..
}
instead of:
} else {
  if (X) {
    ..
  }
}

https://codereview.chromium.org/13896026/diff/2001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager.cc:439: bounds.x() != work_area_.x())
This seems the same as the case above.

https://codereview.chromium.org/13896026/diff/2001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager.cc:456: }
I don't think we'll support sticking to the bottom or top for now.

[varkha, 2013-05-18 03:06:37]

PTAL

https://codereview.chromium.org/13896026/diff/2001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/13896026/diff/2001/ash/ash_switches.cc#newcode81
ash/ash_switches.cc:81: // Enable the gutter area on a desktop.
On 2013/05/01 17:34:04, flackr wrote:
> We might want a nicer name for this. Enable docked window?

Done.

https://codereview.chromium.org/13896026/diff/2001/ash/wm/window_properties.h
File ash/wm/window_properties.h (right):

https://codereview.chromium.org/13896026/diff/2001/ash/wm/window_properties.h...
ash/wm/window_properties.h:101: // A bitmask containing the workspace edges that
the window is stuck to.
I am not sure. When the gutter appears, say on the right with the non-maximized
window moving the gutter boundary left would eventually squeeze the window to
the left of it to be stuck to both edges. In this state dragging the gutter
boundary back to the right I think should preserve the window touching both the
left edge (screen) and the right edge (gutter edge). This can now be done by
resizing the screen.
Also resizing to both right and left or docking to max-left or max-right creates
multiple edges situations that we probably want to maintain when gutter and / or
launcher size and / or visibility change.

https://codereview.chromium.org/13896026/diff/2001/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/2001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager.cc:366: stuck_edges_mask =
CalculateStuckEdges(window);
On 2013/05/01 17:34:04, flackr wrote:
> We probably only want to "stick" windows when they're being moved in
> workspace_window_resizer.cc. Doing so for all bounds updates would allow for
> confusing behaviors like a restored window creating a gutter or creating a
> gutter after a resize.

Done.

https://codereview.chromium.org/13896026/diff/2001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager.cc:427: if ((0 !=
(GetStuckToEdge(window) & EDGE_RIGHT) &&
On 2013/05/01 17:34:04, flackr wrote:
> why the 0 !=? This should be fine as just if ((GetStuckToEdge(window) &
> EDGE_RIGHT) && ...

Done.

https://codereview.chromium.org/13896026/diff/2001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager.cc:428: bounds.right() !=
work_area_.right()) ||
On 2013/05/01 17:34:04, flackr wrote:
> These seem like they should be handled separately as they don't share much
code.
> i.e. rather than
> if (A || B) {
>   if (A) {
> 
>   } else { // B
> 
> }
> just:
> if (A) {
> 
> } else if (B) {
> 
> }

Done.

https://codereview.chromium.org/13896026/diff/2001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager.cc:437: } else {
The code got broken when I was chasing launcher auto-hide. Simplified.

https://codereview.chromium.org/13896026/diff/2001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager.cc:439: bounds.x() != work_area_.x())
On 2013/05/01 17:34:04, flackr wrote:
> This seems the same as the case above.

Done.

https://codereview.chromium.org/13896026/diff/2001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager.cc:456: }
The top and bottom are useful when launcher auto-hide changes. This way a window
that is right- or left-maximized maintains its docked state nicely through
transitions.

[flackr, 2013-05-22 02:00:56]

I'm worried that the idea of a window being stuck to an edge is unrelated to
whether or not it should create a gutter. While having windows stick to edges
when the display is resized may be desirable, this is orthogonal to docking
windows which have been placed at the right / left edge of the display.

https://codereview.chromium.org/13896026/diff/2001/ash/wm/window_properties.h
File ash/wm/window_properties.h (right):

https://codereview.chromium.org/13896026/diff/2001/ash/wm/window_properties.h...
ash/wm/window_properties.h:101: // A bitmask containing the workspace edges that
the window is stuck to.
On 2013/05/18 03:06:37, varkha wrote:
> I am not sure. When the gutter appears, say on the right with the
non-maximized
> window moving the gutter boundary left would eventually squeeze the window to
> the left of it to be stuck to both edges. In this state dragging the gutter
> boundary back to the right I think should preserve the window touching both
the
> left edge (screen) and the right edge (gutter edge). This can now be done by
> resizing the screen.
> Also resizing to both right and left or docking to max-left or max-right
creates
> multiple edges situations that we probably want to maintain when gutter and /
or
> launcher size and / or visibility change.

For now, I think a window touching a gutter should not be considered "stuck". We
only want to allow one gutter at a time.

https://codereview.chromium.org/13896026/diff/43001/ash/wm/property_util.cc
File ash/wm/property_util.cc (right):

https://codereview.chromium.org/13896026/diff/43001/ash/wm/property_util.cc#n...
ash/wm/property_util.cc:47: LOG(INFO) << "SetStuckToEdge " << value;
Probably shouldn't land with this.

https://codereview.chromium.org/13896026/diff/43001/ash/wm/workspace/frame_ma...
File ash/wm/workspace/frame_maximize_button.cc (right):

https://codereview.chromium.org/13896026/diff/43001/ash/wm/workspace/frame_ma...
ash/wm/workspace/frame_maximize_button.cc:577:
switches::kAshEnableDockedWindows))
nit: multiline if condition should use braces.

https://codereview.chromium.org/13896026/diff/43001/ash/wm/workspace/frame_ma...
ash/wm/workspace/frame_maximize_button.cc:578: SetStuckToEdge(window,
stuck_edges_mask);
Why check if the condition is not stuck_edges_mask only to set it to
stuck_edges_mask? I'd suggest just having a function which updates the stuck
edge(s) of a window.

https://codereview.chromium.org/13896026/diff/43001/ash/wm/workspace/frame_ma...
ash/wm/workspace/frame_maximize_button.cc:583: // Remember the widow's bounds
for restoration.
s/widow/window

https://codereview.chromium.org/13896026/diff/43001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/43001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager.cc:404: const gfx::Rect& bounds) {
We should do all docked window specific layout changes in a new layout manager
(call it docked_layout_manager). This layout manager would be similar to
panel_layout_manager except that it would only affect docked windows.

However, it seems like the change you're making is more general than docked
windows. You're keeping windows on the edge they were placed on. If this was
desirable it could belong here, I just think we should get approval for such a
feature before landing it. This is tangential to being able to dock windows
though.

[varkha, 2013-05-22 02:42:53]

Thanks for taking the time to review this. Some of it is obviously scaffolding
that won't get submitted (like a check before set that allows easy breakpoints
on state changes). More importantly, do you think if we were to keep the
currently implemented here stuck-to-edges behaviour would we need two separate
state variables for those largely orthogonal behaviours?
This could definitely benefit from a more detailed spec. I will need to invest
some time in it.

[flackr, 2013-05-23 15:45:15]

On 2013/05/22 02:42:53, varkha wrote:
> Thanks for taking the time to review this. Some of it is obviously scaffolding
> that won't get submitted (like a check before set that allows easy breakpoints
> on state changes). More importantly, do you think if we were to keep the
> currently implemented here stuck-to-edges behaviour would we need two separate
> state variables for those largely orthogonal behaviours?
> This could definitely benefit from a more detailed spec. I will need to invest
> some time in it.

I think we need some state to say that a window is in the dock (and which side)
or not. This can be used when laying out the windows / determining which
container to parent windows in (GetDefaultParent). I don't think this state is
the same as which edges a window is adjacent to. i.e. we may decide to only
apply this for "small" windows or only certain types of windows.

[varkha, 2013-05-30 21:59:11]

flackr@, please take a look. Some cleanup still needs to happen (most
importantly Relayout contains some unnecessary code from panels, LOGs that I
will remove before commit, probably more). Still I wanted to get it looked at -
I think there is enough functionality for the first behind-the-flag mock.
Dragging panels and small windows should open the dock on left or right and
dragging additional windows on the same side should dock them as well. Panels
need to be undocked from the launcher first (it seems to be an unrelated bug).
There is also response to launcher alignment change (all docked windows travel
to the other side if necessary). You can minimize and horizontally resize
windows in the dock and that will update the workspace width. There is no
vertical fanning of docked windows - that will come later. For now vertical
offset is entirely user set. The patch is rebased.

[flackr, 2013-05-31 14:23:45]

Can we break this up into smaller CLs?

i.e.
1) Managing some "docked" property of windows when they're dragged to edges and
reparenting to dock container (would effectively move docked windows above other
windows although I'm not sure if this is desirable).
2) Initial dock_layout_manager implementation which sets all docked windows to
snap to docked screen edge.
3) Automatic layout of docked windows.

There's a lot of code copied over from panel layout manager that isn't relevant
to docked windows. I can go through and point out all of it if that would be
useful but I assume this is still a work in progress?

https://codereview.chromium.org/13896026/diff/90008/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/13896026/diff/90008/ash/ash_switches.cc#newco...
ash/ash_switches.cc:75: // Enable the gutter area on a desktop.
s/gutter/dock.

https://codereview.chromium.org/13896026/diff/90008/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/90008/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:738: // Also push in the work area inset for
the dock if it is visible.
It seems odd that the shelf layout manager is responsible for managing the work
area insets based on itself, the keyboard and the dock. Long term this
management should probably be moved to a more generic place where something
observes changes to each component which can affect the work space. Can you
leave a TODO to this effect?

https://codereview.chromium.org/13896026/diff/90008/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:742: target_bounds->work_area_insets.Set(
the +=(Insets) operator would probably look nicer.

https://codereview.chromium.org/13896026/diff/90008/ash/wm/dock/dock_layout_m...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/90008/ash/wm/dock/dock_layout_m...
ash/wm/dock/dock_layout_manager.cc:83: void
FanOutWindows(std::vector<VisibleWindowPositionInfo>::iterator first,
The code here (from panel_layout_manager.cc I assume?) had the constraint that
windows had to be positioned over their icons, which was why there was a min,
max and actual position. For docked windows, the window itself and the container
should define all of the necessary parameters.

https://codereview.chromium.org/13896026/diff/90008/ash/wm/dock/dock_layout_m...
ash/wm/dock/dock_layout_manager.cc:517: void DockLayoutManager::Relayout() {
I think you'll want to rewrite this function to lay out the windows with the
constraints of docked windows.

https://codereview.chromium.org/13896026/diff/90008/ash/wm/dock/dock_layout_m...
ash/wm/dock/dock_layout_manager.cc:690: // TODO: Implement restacking if ever
necessary
This will most likely be necessary. Given that we'll try to fan out windows
within the space, similar to panels, stacking them from the active window
outwards will make all docked windows partially visible (and selectable).

https://codereview.chromium.org/13896026/diff/90008/ash/wm/dock/dock_layout_m...
File ash/wm/dock/dock_layout_manager.h (right):

https://codereview.chromium.org/13896026/diff/90008/ash/wm/dock/dock_layout_m...
ash/wm/dock/dock_layout_manager.h:156: DockList dock_windows_;
I don't think you need to maintain a separate list of docked windows. All
children of the managed container are docked windows.

https://codereview.chromium.org/13896026/diff/90008/ash/wm/window_properties.h
File ash/wm/window_properties.h (right):

https://codereview.chromium.org/13896026/diff/90008/ash/wm/window_properties....
ash/wm/window_properties.h:100: // A bitmask containing the workspace edges that
the window is docked to.
Assuming we only want one dock, we may just want a bool for if the window is
docked. Assuming multiple we could have a single edge for which side it's docked
on. A window shouldn't be docked on multiple edges.

[varkha, 2013-05-31 15:14:33]

My goal is to have 1 and 2 in this CL and remove as much of 3 as I can while
keeping the behavior meaningful. I struggled with making 1 stand on its own
without breaking too many basics. One thing that has to be taken care of if we
allow adding items to the dock is dragging items out of the dock. Do you think
this is a feasible approach? Other than that it all makes sense and is in line
with what I was going to do.

>>There's a lot of code copied over from panel layout manager that isn't
relevant to docked windows...
Absolutely - this was in my opening comment. I am in process of removing that,
just wanted early comments on the rest.

[varkha, 2013-05-31 23:31:04]

Can you please take a look - this is step 1 plus docking on a side (but no
insetting of the workspace for now).

[varkha, 2013-05-31 23:33:41]

PTAL

https://codereview.chromium.org/13896026/diff/90008/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/13896026/diff/90008/ash/ash_switches.cc#newco...
ash/ash_switches.cc:75: // Enable the gutter area on a desktop.
On 2013/05/31 14:23:45, flackr wrote:
> s/gutter/dock.

Done.

https://codereview.chromium.org/13896026/diff/90008/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/90008/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:738: // Also push in the work area inset for
the dock if it is visible.
On 2013/05/31 14:23:45, flackr wrote:
> It seems odd that the shelf layout manager is responsible for managing the
work
> area insets based on itself, the keyboard and the dock. Long term this
> management should probably be moved to a more generic place where something
> observes changes to each component which can affect the work space. Can you
> leave a TODO to this effect?

Done.

https://codereview.chromium.org/13896026/diff/90008/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:742: target_bounds->work_area_insets.Set(
On 2013/05/31 14:23:45, flackr wrote:
> the +=(Insets) operator would probably look nicer.

Done.

https://codereview.chromium.org/13896026/diff/90008/ash/wm/dock/dock_layout_m...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/90008/ash/wm/dock/dock_layout_m...
ash/wm/dock/dock_layout_manager.cc:83: void
FanOutWindows(std::vector<VisibleWindowPositionInfo>::iterator first,
I will address this in a separate CL. Removed for now.

https://codereview.chromium.org/13896026/diff/90008/ash/wm/dock/dock_layout_m...
ash/wm/dock/dock_layout_manager.cc:517: void DockLayoutManager::Relayout() {
That is the plan. For now I kept just the code that keeps the windows anchored
at the edge.

https://codereview.chromium.org/13896026/diff/90008/ash/wm/dock/dock_layout_m...
ash/wm/dock/dock_layout_manager.cc:690: // TODO: Implement restacking if ever
necessary
This is for a separate CL. I have updated TODO comment for now.

https://codereview.chromium.org/13896026/diff/90008/ash/wm/dock/dock_layout_m...
File ash/wm/dock/dock_layout_manager.h (right):

https://codereview.chromium.org/13896026/diff/90008/ash/wm/dock/dock_layout_m...
ash/wm/dock/dock_layout_manager.h:156: DockList dock_windows_;
On 2013/05/31 14:23:45, flackr wrote:
> I don't think you need to maintain a separate list of docked windows. All
> children of the managed container are docked windows.

Done.

https://codereview.chromium.org/13896026/diff/90008/ash/wm/window_properties.h
File ash/wm/window_properties.h (right):

https://codereview.chromium.org/13896026/diff/90008/ash/wm/window_properties....
ash/wm/window_properties.h:100: // A bitmask containing the workspace edges that
the window is docked to.
On 2013/05/31 14:23:45, flackr wrote:
> Assuming we only want one dock, we may just want a bool for if the window is
> docked. Assuming multiple we could have a single edge for which side it's
docked
> on. A window shouldn't be docked on multiple edges.

Done.

[stevenjb, 2013-06-03 18:19:03]

Mostly nits. Once addressed, I think sky@ should also take a look since he's
much more familiar with the layout manager architecture.

https://codereview.chromium.org/13896026/diff/122001/ash/shell_window_ids.h
File ash/shell_window_ids.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/shell_window_ids.h#n...
ash/shell_window_ids.h:102: // The container for windows docked to the sides.
nit: s/the sides/either side of a desktop/

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:33: namespace {
nit: blank line

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:106: dock_edge == DOCK_EDGE_RIGHT))
nit: alignment

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:166: const gfx::Rect& requested_bounds) {
nit: align

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:177: if (dock_container_->GetRootWindow() ==
root_window) {
nit: Early exit instead

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:189: shelf_alignment ==
SHELF_ALIGNMENT_RIGHT) {
nit: align

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:204: intptr_t old) {
nit: align

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:236: aura::Window* window, bool visible) {
One parameter per line in function declaration or definition

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:248: gained_active->parent() ==
dock_container_) {
One line? In which case no {}

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:262: // TODO:
ShelfLayoutManager::UpdateVisibilityState sets state to
TODO(varkha)

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:319: DOCK_ALIGNMENT_LEFT :
DOCK_ALIGNMENT_RIGHT;
nit: align

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:371: // TODO: Implement restacking to ensure
that all docked windows are at least
TODO(varkha)

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.h:101: };
nit: blank line

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
File ash/wm/dock/dock_window_resizer.cc (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.cc:31: namespace {
blank line

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.cc:49: int window_component) {
align

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
File ash/wm/dock/dock_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.h:19: // moving or resizing of a window while in
dock.
nit: 'in a dock'

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.h:32: // WindowResizer overides:
nit: There is a slight preference for override comments to just look like:
  // WindowResizer
I prefer the simplicity / brevity of this style myself, and so encourage others
to use it :) In practice however, as long as a class is consistent the exact
comment style doesn't mater.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.h:43: const Details& details);
nit: align

https://codereview.chromium.org/13896026/diff/122001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer.cc:74: window->type() ==
aura::client::WINDOW_TYPE_PANEL) {
align

https://codereview.chromium.org/13896026/diff/122001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer.cc:782: const gfx::Rect& bounds) const
{
This seems like it should be either a static in DockLayoutManager, or a function
in window_util.cc, instead of here.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer.h:198: // Last updated touched edge.
Can you expand this comment? It's not entirely clear to me why this needs to be
here, instead of in DockWindowResizer.

[varkha, 2013-06-03 21:03:17]

With comments addressed. flakr@, stevenjb@, do you think this is OK for owner's
review?

https://codereview.chromium.org/13896026/diff/122001/ash/shell_window_ids.h
File ash/shell_window_ids.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/shell_window_ids.h#n...
ash/shell_window_ids.h:102: // The container for windows docked to the sides.
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> nit: s/the sides/either side of a desktop/

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:33: namespace {
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> nit: blank line

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:106: dock_edge == DOCK_EDGE_RIGHT))
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> nit: alignment

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:166: const gfx::Rect& requested_bounds) {
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> nit: align

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:177: if (dock_container_->GetRootWindow() ==
root_window) {
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> nit: Early exit instead

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:189: shelf_alignment ==
SHELF_ALIGNMENT_RIGHT) {
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> nit: align

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:204: intptr_t old) {
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> nit: align

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:236: aura::Window* window, bool visible) {
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> One parameter per line in function declaration or definition

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:248: gained_active->parent() ==
dock_container_) {
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> One line? In which case no {}

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:262: // TODO:
ShelfLayoutManager::UpdateVisibilityState sets state to
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> TODO(varkha)

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:319: DOCK_ALIGNMENT_LEFT :
DOCK_ALIGNMENT_RIGHT;
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> nit: align

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:371: // TODO: Implement restacking to ensure
that all docked windows are at least
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> TODO(varkha)

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.h:101: };
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> nit: blank line

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
File ash/wm/dock/dock_window_resizer.cc (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.cc:31: namespace {
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> blank line

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.cc:49: int window_component) {
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> align

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
File ash/wm/dock/dock_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.h:19: // moving or resizing of a window while in
dock.
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> nit: 'in a dock'

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.h:32: // WindowResizer overides:
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> nit: There is a slight preference for override comments to just look like:
>   // WindowResizer
> I prefer the simplicity / brevity of this style myself, and so encourage
others
> to use it :) In practice however, as long as a class is consistent the exact
> comment style doesn't mater.

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.h:43: const Details& details);
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> nit: align

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer.cc:74: window->type() ==
aura::client::WINDOW_TYPE_PANEL) {
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> align

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer.cc:782: const gfx::Rect& bounds) const
{
On 2013/06/03 18:19:03, stevenjb (chromium) wrote:
> This seems like it should be either a static in DockLayoutManager, or a
function
> in window_util.cc, instead of here. 

Done (as a static in DockLayoutManager). Also moved GetDockLayoutManager there
to improve encapsulation).

https://codereview.chromium.org/13896026/diff/122001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer.h:198: // Last updated touched edge.
This is how I track a window that is still part of workspace but has been
dragged to the side and so it is not yet part of dock and does not have
DockWindowResizer attached to it. Is this OK or do you think we should always
chain in DockWindowResizer for all windows and handle all potential drags there?
I have expanded the comment as well.

[flackr, 2013-06-03 22:03:43]

I know the tests are coming so I won't mention those :-). This is looking much
better and easier to review, thanks for splitting it up!

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:81: // this will reparent the window back to
workspace
Doesn't the reparenting happen immediately? I'm not sure you need the FORCE_MOVE
/ DO_NOT_FORCE_MOVE enum?

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:84: force_move = FORCE_MOVE;
nit: both if and else should either have or not have curlies { }. In this case
they're both one line although i'm not sure if we count comments.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:98: dock_edge = DOCK_EDGE_RIGHT;
nit: curlies for multi-line if condition.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:141: if (GetDockEdge(child) ==
DOCK_EDGE_NONE)
Do we expect windows to get added to the dock container which aren't actually
docked? This seems like an extra complication which isn't necessary.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:221: if (key == aura::client::kShowStateKey)
{
nit: else if

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:292:
base::TimeDelta::FromMilliseconds(kWindowSlideDurationMilliseconds));
It looks like there's nothing immediately here to animate (at least for now),
can you remove the animation settings?

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:324: // touching the launcher.
This comment seems obsolete.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:345: visible_windows.push_back(window);
Building this list is unnecessary. Merge the body from the loop below to
position the window.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:352: if (force_move == FORCE_MOVE ||
As mentioned earlier, I think any windows in this container should be forced to
the edge (except if Relayout is called when dragging but in that case you can
check against dragged_window_).

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
File ash/wm/dock/dock_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.h:67: }  // namespace aura
s/aura/ash

This was probably wrong in panel_window_resizer.h too :-).

https://codereview.chromium.org/13896026/diff/122001/ash/wm/property_util.h
File ash/wm/property_util.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/property_util.h#n...
ash/wm/property_util.h:71: };
I'm not sure if we need to track which edge each window is docked too
individually. The dock should have an alignment and all windows would either be
docked or not docked. When the first window is added to the dock we can use its
bounds to determine the alignment for the dock.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/property_util.h#n...
ash/wm/property_util.h:75: // when the dock alignment is changed.
You mean when the shelf alignment is changed right?

https://codereview.chromium.org/13896026/diff/122001/ash/wm/window_properties.h
File ash/wm/window_properties.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/window_properties...
ash/wm/window_properties.h:100: // A bitmask containing the workspace edges that
the window is docked to.
Not a bitmask right?

[varkha, 2013-06-04 03:13:49]

PTAL with most comments addressed and some explanations.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:81: // this will reparent the window back to
workspace
On 2013/06/03 22:03:43, flackr wrote:
> Doesn't the reparenting happen immediately? I'm not sure you need the
FORCE_MOVE
> / DO_NOT_FORCE_MOVE enum?

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:84: force_move = FORCE_MOVE;
On 2013/06/03 22:03:43, flackr wrote:
> nit: both if and else should either have or not have curlies { }. In this case
> they're both one line although i'm not sure if we count comments.

Done (no need for else anymore).

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:98: dock_edge = DOCK_EDGE_RIGHT;
On 2013/06/03 22:03:43, flackr wrote:
> nit: curlies for multi-line if condition.

Done. Here and in a few more places. Thanks, I missed this rule.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:141: if (GetDockEdge(child) ==
DOCK_EDGE_NONE)
On 2013/06/03 22:03:43, flackr wrote:
> Do we expect windows to get added to the dock container which aren't actually
> docked? This seems like an extra complication which isn't necessary.

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:221: if (key == aura::client::kShowStateKey)
{
On 2013/06/03 22:03:43, flackr wrote:
> nit: else if

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:292:
base::TimeDelta::FromMilliseconds(kWindowSlideDurationMilliseconds));
On 2013/06/03 22:03:43, flackr wrote:
> It looks like there's nothing immediately here to animate (at least for now),
> can you remove the animation settings?

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:324: // touching the launcher.
On 2013/06/03 22:03:43, flackr wrote:
> This comment seems obsolete.

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:345: visible_windows.push_back(window);
On 2013/06/03 22:03:43, flackr wrote:
> Building this list is unnecessary. Merge the body from the loop below to
> position the window.

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:352: if (force_move == FORCE_MOVE ||
On 2013/06/03 22:03:43, flackr wrote:
> As mentioned earlier, I think any windows in this container should be forced
to
> the edge (except if Relayout is called when dragging but in that case you can
> check against dragged_window_).

Done. Convinced ;)

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
File ash/wm/dock/dock_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.h:67: }  // namespace aura
On 2013/06/03 22:03:43, flackr wrote:
> s/aura/ash
> 
> This was probably wrong in panel_window_resizer.h too :-).

Done (both here and in panel_window_resizer.h).

https://codereview.chromium.org/13896026/diff/122001/ash/wm/property_util.h
File ash/wm/property_util.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/property_util.h#n...
ash/wm/property_util.h:71: };
I thought this allows to keep the logic of comparing bounds in only one place.
If we use one boolean property we would need to recalculate what we already did.
The performance hit is of course negligible but there would be some code
duplication that I was trying to avoid.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/property_util.h#n...
ash/wm/property_util.h:75: // when the dock alignment is changed.
On 2013/06/03 22:03:43, flackr wrote:
> You mean when the shelf alignment is changed right?

Done.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/window_properties.h
File ash/wm/window_properties.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/window_properties...
ash/wm/window_properties.h:100: // A bitmask containing the workspace edges that
the window is docked to.
On 2013/06/03 22:03:43, flackr wrote:
> Not a bitmask right?

Done.

[stevenjb, 2013-06-04 16:23:53]

lgtm

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:230: else if (key ==
aura::client::kShowStateKey) {
nit: else on line 228, after }

[flackr, 2013-06-04 19:58:10]

I'm not sure I see the code for how windows can only be docked on one edge. I'm
sure it's there and perhaps I'm just skimming over it. In any event, you could
call something on the dock layout manager to add the window and give the initial
dock alignment from the workspace window resizer.

Overall, looking pretty good.

https://codereview.chromium.org/13896026/diff/122001/ash/wm/property_util.h
File ash/wm/property_util.h (right):

https://codereview.chromium.org/13896026/diff/122001/ash/wm/property_util.h#n...
ash/wm/property_util.h:71: };
On 2013/06/04 03:13:50, varkha wrote:
> I thought this allows to keep the logic of comparing bounds in only one place.
> If we use one boolean property we would need to recalculate what we already
did.
> The performance hit is of course negligible but there would be some code
> duplication that I was trying to avoid.

At the point when you determine the dock edge, you could call something on the
dock layout manager to set the side the dock is on and then you wouldn't have to
recalculate the side. But I understand the reasoning and leave it up to you.

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:116: dock_edge == DOCK_EDGE_RIGHT)) {
this should be aligned with "shelf_alignment" above to show the block it
corresponds to. Same above.

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:229: 
nit: move else if up to brace } else if (...) {

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:357: case DOCK_ALIGNMENT_NONE:
Assert that this never happens. If there are any windows in the container there
should be an alignment. Alternately, NONE could be removed from the enum and use
no children to imply no alignment but that's up to you.

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_window_...
File ash/wm/dock/dock_window_resizer.cc (right):

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.cc:87: internal::kShellWindowId_DockContainer);
Since the dock container is never directly used, just the dock container's
layout manager, just save the dock_layout_manager_ instead. You can inline
GetDockLayoutManager here since it will only be needed in the one place :-).

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_window_...
File ash/wm/dock/dock_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.h:27: static DockWindowResizer*
Create(WindowResizer* next_window_resizer,
This is the third resizer class to wrap another resizer afaik. We should
consider creating a base class for a window resizer that wraps and adds behavior
to another resizer. Maybe add a TODO?

Actually, it seems the only thing the resizer needs to do is notify the layout
manager when a docked window is being dragged. We should have a better way of
doing this, perhaps by having a way of observing drags or having a generic drag
window wrapper which informs a layout manager that a drag has started or
stopped.

https://codereview.chromium.org/13896026/diff/138001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/13896026/diff/138001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer.cc:74: window->type() ==
aura::client::WINDOW_TYPE_PANEL) {
Should this really be an else if? Wouldn't this prevent dragging from the dock
to the launcher?

https://codereview.chromium.org/13896026/diff/138001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer.cc:318: SetDockEdge(window(),
dock_edge_);
This should probably be in CompleteDrag. Also we should handle undocking windows
here too, in the same place as docking them.

[flackr, 2013-06-04 20:58:03]

https://codereview.chromium.org/13896026/diff/138001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/13896026/diff/138001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer.cc:700: dock_edge_ =
DockLayoutManager::FindDockEdge(window());
As discussed, you shouldn't need to track this since you can check from the drag
Details object whether it was a resize or not. See
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/panels/pane...
for an example.

[varkha, 2013-06-07 19:01:28]

PTAL. I will remove LOG statements and probably oak changes before owner review.

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:116: dock_edge == DOCK_EDGE_RIGHT)) {
On 2013/06/04 19:58:10, flackr wrote:
> this should be aligned with "shelf_alignment" above to show the block it
> corresponds to. Same above.

Done.

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:229: 
On 2013/06/04 19:58:10, flackr wrote:
> nit: move else if up to brace } else if (...) {

Done.

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:230: else if (key ==
aura::client::kShowStateKey) {
On 2013/06/04 16:23:54, stevenjb (chromium) wrote:
> nit: else on line 228, after }

Done.

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:357: case DOCK_ALIGNMENT_NONE:
On 2013/06/04 19:58:10, flackr wrote:
> Assert that this never happens. If there are any windows in the container
there
> should be an alignment. Alternately, NONE could be removed from the enum and
use
> no children to imply no alignment but that's up to you.

Done.

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_window_...
File ash/wm/dock/dock_window_resizer.cc (right):

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.cc:87: internal::kShellWindowId_DockContainer);
On 2013/06/04 19:58:10, flackr wrote:
> Since the dock container is never directly used, just the dock container's
> layout manager, just save the dock_layout_manager_ instead. You can inline
> GetDockLayoutManager here since it will only be needed in the one place :-).

Done.

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_window_...
File ash/wm/dock/dock_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/138001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.h:27: static DockWindowResizer*
Create(WindowResizer* next_window_resizer,
Done (see TODO in workspace_window_resizer.cc).

https://codereview.chromium.org/13896026/diff/138001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/13896026/diff/138001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer.cc:74: window->type() ==
aura::client::WINDOW_TYPE_PANEL) {
It would (for the panels). This is a symmetrical situation to what is described
in http://crbug.com/247085 and I have left a TODO about it. It looks like it
does not introduce anything unexpected so I have made this change.

https://codereview.chromium.org/13896026/diff/138001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer.cc:318: SetDockEdge(window(),
dock_edge_);
Done (the first part). The second part may get addressed if we refactor chained
resizers, it is difficult to do it now since the WorkspaceWindowResizer is not
used when moving docked windows (or attached panels for that matter).

https://codereview.chromium.org/13896026/diff/138001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer.cc:700: dock_edge_ =
DockLayoutManager::FindDockEdge(window());
On 2013/06/04 20:58:03, flackr wrote:
> As discussed, you shouldn't need to track this since you can check from the
drag
> Details object whether it was a resize or not. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/panels/pane...
> for an example.

Done.

[varkha, 2013-06-10 14:24:15]

+sky@ for OWNERS review.

[sky, 2013-06-10 15:46:41]

+mukai
It's my understanding we're moving away from a worskpace per maximized window
(see https://codereview.chromium.org/16093036/ and corresponding bug
https://code.google.com/p/chromium/issues/detail?id=245345 ). Given this, do we
need any of this code?

[flackr, 2013-06-10 16:15:53]

On 2013/06/10 15:46:41, sky wrote:
> +mukai
> It's my understanding we're moving away from a worskpace per maximized window
> (see https://codereview.chromium.org/16093036/ and corresponding bug
> https://code.google.com/p/chromium/issues/detail?id=245345 ). Given this, do
we
> need any of this code?

+kuscher, I had thought we still wanted to experiment with this docked window
behavior even in a single workspace to see if we could have some smart
management of small windows.

[kuscher2, 2013-06-10 18:10:57]

+Alcor

Yes, we still want to experiment with clever positioning for panels in this
world.


On Mon, Jun 10, 2013 at 9:15 AM, <flackr@chromium.org> wrote:

> On 2013/06/10 15:46:41, sky wrote:
>
>> +mukai
>> It's my understanding we're moving away from a worskpace per maximized
>> window
>> (see
https://codereview.chromium.**org/16093036/<https://codereview.chromium.org/1...
corresponding bug
>>
https://code.google.com/p/**chromium/issues/detail?id=**245345<https://code.g...).
Given this, do
>>
> we
>
>> need any of this code?
>>
>
> +kuscher, I had thought we still wanted to experiment with this docked
> window
> behavior even in a single workspace to see if we could have some smart
> management of small windows.
>
>
https://codereview.chromium.**org/13896026/<https://codereview.chromium.org/1...
>

[Jun Mukai, 2013-06-10 19:30:06]

quick update: talked with alex, and the UI does not sound like a conflict with
my CL actually, since that would happen with normal desktop workspace too.
I'll take a look at the CL too just in case. Please wait a bit.

On 2013/06/10 18:10:57, kuscher_google.com wrote:
> +Alcor
> 
> Yes, we still want to experiment with clever positioning for panels in this
> world.
> 
> 
> On Mon, Jun 10, 2013 at 9:15 AM, <mailto:flackr@chromium.org> wrote:
> 
> > On 2013/06/10 15:46:41, sky wrote:
> >
> >> +mukai
> >> It's my understanding we're moving away from a worskpace per maximized
> >> window
> >> (see
>
https://codereview.chromium.**org/16093036/%3Chttps://codereview.chromium.org...
> corresponding bug
> >>
>
https://code.google.com/p/**chromium/issues/detail?id=**245345%3Chttps://code...>).
> Given this, do
> >>
> > we
> >
> >> need any of this code?
> >>
> >
> > +kuscher, I had thought we still wanted to experiment with this docked
> > window
> > behavior even in a single workspace to see if we could have some smart
> > management of small windows.
> >
> >
>
https://codereview.chromium.**org/13896026/%3Chttps://codereview.chromium.org...>
> >

[Jun Mukai, 2013-06-10 20:40:56]

seems not conflict with my CL. a few comments.

https://codereview.chromium.org/13896026/diff/192002/ash/shell_window_ids.h
File ash/shell_window_ids.h (right):

https://codereview.chromium.org/13896026/diff/192002/ash/shell_window_ids.h#n...
ash/shell_window_ids.h:103: const int kShellWindowId_DockContainer = 23;
this looks too high.  IIRC the order of id here corresponds to the order of the
container layers (someone in ash/wm uses that assumption).  Does it really need
to appear above of phantom
windows/partial-screenshot-UI/power-button-animation??
If not, probably good to move aside of PanelContainer and increment all other
containers above.

https://codereview.chromium.org/13896026/diff/192002/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/192002/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:20: #include "ash/wm/workspace_controller.h"
unnecessary include?

https://codereview.chromium.org/13896026/diff/192002/ash/wm/dock/dock_window_...
File ash/wm/dock/dock_window_resizer.cc (right):

https://codereview.chromium.org/13896026/diff/192002/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.cc:116:
workspace_controller->SetActiveWorkspaceByWindow(window);
This SetActiveworkspaceByWindow may not be necessary after my CL, since
maximized window doesn't have its own workspace.  Still fullscreen/immersive
windows have, but I heard that this dock doesn't appear with those windows.

[flackr, 2013-06-10 20:58:06]

https://codereview.chromium.org/13896026/diff/192002/ash/shell_window_ids.h
File ash/shell_window_ids.h (right):

https://codereview.chromium.org/13896026/diff/192002/ash/shell_window_ids.h#n...
ash/shell_window_ids.h:103: const int kShellWindowId_DockContainer = 23;
On 2013/06/10 20:40:57, Jun Mukai wrote:
> this looks too high.  IIRC the order of id here corresponds to the order of
the
> container layers (someone in ash/wm uses that assumption).  Does it really
need
> to appear above of phantom
> windows/partial-screenshot-UI/power-button-animation??
> If not, probably good to move aside of PanelContainer and increment all other
> containers above.

Just looked into this, stacking_controller.cc uses this assumption to determine
if windows were created from containers visible on the lock screen. So yes, the
id's should be renumbered to make room for this as a lower id.

https://codereview.chromium.org/13896026/diff/192002/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/192002/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:100: if (bounds.right() ==
dock_bounds.right() &&
Move if on previous line with else, i.e. "} else if (..."

https://codereview.chromium.org/13896026/diff/192002/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:148: if (!IsWindowDocked(child))
We add undocked windows to the dock container now? Wasn't the code change in the
drag resizer to make sure we never enter this case?

[varkha, 2013-06-10 21:09:47]

Thanks and PTAL.

https://codereview.chromium.org/13896026/diff/192002/ash/shell_window_ids.h
File ash/shell_window_ids.h (right):

https://codereview.chromium.org/13896026/diff/192002/ash/shell_window_ids.h#n...
ash/shell_window_ids.h:103: const int kShellWindowId_DockContainer = 23;
On 2013/06/10 20:58:07, flackr wrote:
> On 2013/06/10 20:40:57, Jun Mukai wrote:
> > this looks too high.  IIRC the order of id here corresponds to the order of
> the
> > container layers (someone in ash/wm uses that assumption).  Does it really
> need
> > to appear above of phantom
> > windows/partial-screenshot-UI/power-button-animation??
> > If not, probably good to move aside of PanelContainer and increment all
other
> > containers above.
> 
> Just looked into this, stacking_controller.cc uses this assumption to
determine
> if windows were created from containers visible on the lock screen. So yes,
the
> id's should be renumbered to make room for this as a lower id.

Done.

https://codereview.chromium.org/13896026/diff/192002/ash/shell_window_ids.h#n...
ash/shell_window_ids.h:103: const int kShellWindowId_DockContainer = 23;
On 2013/06/10 20:40:57, Jun Mukai wrote:
> this looks too high.  IIRC the order of id here corresponds to the order of
the
> container layers (someone in ash/wm uses that assumption).  Does it really
need
> to appear above of phantom
> windows/partial-screenshot-UI/power-button-animation??
> If not, probably good to move aside of PanelContainer and increment all other
> containers above.

Done.

https://codereview.chromium.org/13896026/diff/192002/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/192002/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:20: #include "ash/wm/workspace_controller.h"
On 2013/06/10 20:40:57, Jun Mukai wrote:
> unnecessary include?

Done.

https://codereview.chromium.org/13896026/diff/192002/ash/wm/dock/dock_window_...
File ash/wm/dock/dock_window_resizer.cc (right):

https://codereview.chromium.org/13896026/diff/192002/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.cc:116:
workspace_controller->SetActiveWorkspaceByWindow(window);
Thanks, It will probably be unnecessary once your CL lands (I will remove it
then). For now it prevents assertion when a docked window is undocked and a
maximized window exists.

[sky, 2013-06-10 23:00:47]

Can you better outline what this is attempting to do then? From the bug:

"When a window is stuck to the side of a workspace (See  issue 228955 ) it
should have a window property assigned to mark it as stuck to the workspace.
While stuck they will not fall through to the desktop workspace. This should be
behind a runtime flag while we work out the behavior of these windows."

Because of Mukai's change we only have other worskpace for fullscreen window. I
believe that if another window opens up when fullscreen we'll exit out of it. If
this is right, I don't see the need for this change then. Maybe I'm missing
something? Or maybe the description in the bug is just outdated?

[sky, 2013-06-11 00:03:12]

https://codereview.chromium.org/13896026/diff/211001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/13896026/diff/211001/ash/ash.gyp#newcode369
ash/ash.gyp:369: 'wm/dock/dock_layout_manager.cc',
sort

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:67: if (shelf_layout_manager_)
If you're going to have a Shutdown() method, shouldn't all of this be in
Shutdown()? I'm fine with the destructor invoking Shutdown, it just seems weird
to have Shutdown mean partial shutdown.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:90: DockEdge
DockLayoutManager::FindDockEdge(aura::Window* window) {
Static sections should be prefixed with
// static

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:93: gfx::Rect dock_bounds =
layout_manager->dock_container_->bounds();
The coordinates are not necessarily the same here.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:118: void
DockLayoutManager::SetLauncher(ash::Launcher* launcher) {
ordering should match header.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:132: if
(window->GetProperty(aura::client::kShowStateKey) ==
Why is there here (and apparently in PanelLayoutManager since this is likely a
copy of it)? Seems like it should be in a more common location.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:134:
window->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_NORMAL);
What if the window was maximized?

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:159: return;
nit: spacing

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:177: SetChildBoundsDirect(child,
requested_bounds);
Does this need to undock?

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:300: alignment_ =
GetDockEdge(dock_container_->children()[0]) == DOCK_EDGE_LEFT ?
How come you need to cache the edge per window? Isn't the edge more of per
DockLayoutManager or even something you can calculate based on position rather
than needing to set as a property?

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.h (right):

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
Dates are wrong in all new files.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.h:33: // DockLayoutManager is responsible for
organizing windows within the
IMO dock is an overloaded term and the use of it here is confusing. There are a
couple of places in the document you pointed me at that use 'dock' when they
mean launcher. This was my initial thinking when looking at these files.

WorkspaceWindowResizer uses 'stick to work area' for clarity. Maybe
WorkAreaAlignedLayoutManager. 'Docked' would be better than 'dock', eg
DockedWindowLayoutManager.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.h:59: void ToggleMinimize(aura::Window* window);
Description?

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.h:64: // checks if the window touches the screen
edge (no dock yet) or has entered
Start with cap and end with appropriate punctuation.

Also, seems like this is more of 'Returns edge for specified window' rather than
'checks'.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.h:130: enum DockAlignment {
enums should be near the top of a section (see style guide for specific
ordering).

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager_unittest.cc (right):

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager_unittest.cc:229: // Tests that a created panel
window is successfully added to the panel
panel?

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_window_...
File ash/wm/dock/dock_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.h:27: // moving or resizing of a window while in
a dock.
Again, in 'dock' is confusing here.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/property_util.h
File ash/wm/property_util.h (right):

https://codereview.chromium.org/13896026/diff/211001/ash/wm/property_util.h#n...
ash/wm/property_util.h:66: enum DockEdge {
Again, Docked would be better here.

[varkha, 2013-06-11 02:34:23]

PTAL. Almost all resolved and a couple questions.

https://codereview.chromium.org/13896026/diff/211001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/13896026/diff/211001/ash/ash.gyp#newcode369
ash/ash.gyp:369: 'wm/dock/dock_layout_manager.cc',
On 2013/06/11 00:03:13, sky wrote:
> sort

Done.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:67: if (shelf_layout_manager_)
On 2013/06/11 00:03:13, sky wrote:
> If you're going to have a Shutdown() method, shouldn't all of this be in
> Shutdown()? I'm fine with the destructor invoking Shutdown, it just seems
weird
> to have Shutdown mean partial shutdown.

Done.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:90: DockEdge
DockLayoutManager::FindDockEdge(aura::Window* window) {
On 2013/06/11 00:03:13, sky wrote:
> Static sections should be prefixed with
> // static

Done.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:93: gfx::Rect dock_bounds =
layout_manager->dock_container_->bounds();
Done. (I had this already fixed in the second CL. Moving over.)

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:118: void
DockLayoutManager::SetLauncher(ash::Launcher* launcher) {
Done. (I had this already fixed in the second CL. Moving over.)

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:132: if
(window->GetProperty(aura::client::kShowStateKey) ==
I don't think this is doing anything useful at the moment. Removed it for now. I
will check with flackr@ what the purpose was in PanelLayoutManager (where is
seems to be unused as well).

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:134:
window->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_NORMAL);
Removed.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:159: return;
On 2013/06/11 00:03:13, sky wrote:
> nit: spacing

Done.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:177: SetChildBoundsDirect(child,
requested_bounds);
I don't think so. The behavior we are trying to get is to have windows dock and
dock only in response to user moving them, so I tried not to get any
programmatic scenarios change the docking.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:300: alignment_ =
GetDockEdge(dock_container_->children()[0]) == DOCK_EDGE_LEFT ?
We had this discussion so it is not unexpected. I thought that a cost of having
a boolean ("docked or not" property) or a emum in a window as a property is same
while having layout calculate the edge alignment based on coordinates will
likely complicate the code a bit. I can change it if you think this is important
in this CL or address it with the followup.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.h (right):

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
On 2013/06/11 00:03:13, sky wrote:
> Dates are wrong in all new files.

Done.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.h:33: // DockLayoutManager is responsible for
organizing windows within the
I like DockedWindowLayoutManager better. I checked "dock" in code and it did not
seem to be overly used. We usually call the launcher "launcher" or "shelf". I
think "stick to work area" as a term leaves more to interpretation.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.h:59: void ToggleMinimize(aura::Window* window);
This one is removed.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.h:64: // checks if the window touches the screen
edge (no dock yet) or has entered
On 2013/06/11 00:03:13, sky wrote:
> Start with cap and end with appropriate punctuation.
> 
> Also, seems like this is more of 'Returns edge for specified window' rather
than
> 'checks'.

Done.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.h:130: enum DockAlignment {
On 2013/06/11 00:03:13, sky wrote:
> enums should be near the top of a section (see style guide for specific
> ordering).

Done.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager_unittest.cc (right):

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager_unittest.cc:229: // Tests that a created panel
window is successfully added to the panel
On 2013/06/11 00:03:13, sky wrote:
> panel?

Done.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_window_...
File ash/wm/dock/dock_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_window_...
ash/wm/dock/dock_window_resizer.h:27: // moving or resizing of a window while in
a dock.
On 2013/06/11 00:03:13, sky wrote:
> Again, in 'dock' is confusing here.

Done.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/property_util.h
File ash/wm/property_util.h (right):

https://codereview.chromium.org/13896026/diff/211001/ash/wm/property_util.h#n...
ash/wm/property_util.h:66: enum DockEdge {
On 2013/06/11 00:03:13, sky wrote:
> Again, Docked would be better here.

Done.

[sky, 2013-06-11 16:15:26]

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:177: SetChildBoundsDirect(child,
requested_bounds);
On 2013/06/11 02:34:24, varkha wrote:
> I don't think so. The behavior we are trying to get is to have windows dock
and
> dock only in response to user moving them, so I tried not to get any
> programmatic scenarios change the docking.

Fair enough. It would be clearer if you had a comment to this effect and perhaps
only set the size (and not the position).

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:300: alignment_ =
GetDockEdge(dock_container_->children()[0]) == DOCK_EDGE_LEFT ?
On 2013/06/11 02:34:24, varkha wrote:
> We had this discussion so it is not unexpected. I thought that a cost of
having
> a boolean ("docked or not" property) or a emum in a window as a property is
same
> while having layout calculate the edge alignment based on coordinates will
> likely complicate the code a bit. I can change it if you think this is
important
> in this CL or address it with the followup.

More properties typically means more things to get out of sync. If you can
accomplish what you need without needing another property things are less likely
to get out of sync.

I'm hoping this function could remain the same, it's really only the GetDockEdge
that needs to change, right?

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.h (right):

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.h:33: // DockLayoutManager is responsible for
organizing windows within the
On 2013/06/11 02:34:24, varkha wrote:
> I like DockedWindowLayoutManager better. I checked "dock" in code and it did
not
> seem to be overly used. We usually call the launcher "launcher" or "shelf". I
> think "stick to work area" as a term leaves more to interpretation.

I think many people use the term 'dock' to mean 'launcher'. The document you
pointed me at for the wm spec uses 'dock' in a couple of places to mean
'launcher'. This why I think using 'dock' is too ambiguous here. 'docked' is a
better match for what is happening.

[flackr, 2013-06-11 17:59:31]

https://codereview.chromium.org/13896026/diff/130005/ash/wm/activation_contro...
File ash/wm/activation_controller.cc (right):

https://codereview.chromium.org/13896026/diff/130005/ash/wm/activation_contro...
ash/wm/activation_controller.cc:46: kShellWindowId_DockContainer,
I think this should probably be above PanelContainer with respect to activation.
I think the user would expect docked windows to be focused before panels.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/ash_focus_rules.cc
File ash/wm/ash_focus_rules.cc (right):

https://codereview.chromium.org/13896026/diff/130005/ash/wm/ash_focus_rules.c...
ash/wm/ash_focus_rules.cc:31: internal::kShellWindowId_DockContainer,
Here tool.

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

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:330: abs(bounds.x() -
dock_bounds.x()) < kMagnetismWidth) {
Snapping windows during drags seems like something the docked window resizer
should do.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_layout_manager_unittest.cc (right):

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager_unittest.cc:156:
EXPECT_EQ(CorrectContainerIdDuringDrag(), window->parent()->id());
Shouldn't the window be in the correct container during drag after the drag has
started?

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager_unittest.cc:180: }
The snapping behavior is already tested in panel_window_resizer. This should
probably just drag far enough to ensure the window is detached (and expect so)
since not doing so might invalidate the rest of the test. But I think the panel
snapping behavior can remain tested by panel window resizer.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager_unittest.cc:191: // Release the mouse
and the panel should be attached to the dock.
Add Expect that the window's parent is the dock container.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager_unittest.cc:199: void
WindowInScreen(aura::Window* window) {
This doesn't seem to be used.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager_unittest.cc:247:
INSTANTIATE_TEST_CASE_P(PanelsOrNormal, DockLayoutManagerTest, testing::Bool());
Rather than adding a bool parameter can you pass the window type as the
parameter (i.e. testing::Values(aura::client::WINDOW_TYPE_NORMAL,
aura::client::WINDOW_TYPE_PANEL) and use the type directly in
CreateTestWindowInShellWithDelegateAndType?

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer_unittest.cc (right):

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:151: DragMove(0, 5);
FWIW, this is dragging in the opposite direction to the one which detaches the
panel (i.e. should be dragging up to try to detach / test snapping behavior),
but since this is already tested in panel_window_resizer we don't need to retest
it here.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:239: // the screen edge it does
not get attached to the dock and stays in WS
s/WS/the workspace

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:252: // Verifies a window can be
dragged and detached and then attached to the dock.
I think these comments are confusing, detaching the window is only applicable to
panel windows however the test would seem to be for both panel and non panel
windows. Since detaching is already verified in panel_window_resizer can we
assume it works here and just test the dock behavior?

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:394: if (test_panels()) {
I'm a little confused about what this is doing. It is done in two tests though,
perhaps you could add a helper function and elaborate on why there are different
offsets for these.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:559:
INSTANTIATE_TEST_CASE_P(PanelsOrNormal, DockWindowResizerTest, testing::Bool());
As before, perhaps use window type as parameter.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/panels/panel_wind...
File ash/wm/panels/panel_window_resizer.cc (right):

https://codereview.chromium.org/13896026/diff/130005/ash/wm/panels/panel_wind...
ash/wm/panels/panel_window_resizer.cc:194: if (!was_attached_ &&
!IsWindowDocked(GetTarget())) {
Don't we want dragged panels to show in front of other panels even if they were
dragged from the dock?

https://codereview.chromium.org/13896026/diff/130005/ash/wm/panels/panel_wind...
ash/wm/panels/panel_window_resizer.cc:211:
GetTarget()->SetDefaultParentByRootWindow(root_window_,
This is using the root window from the start of the drag but this would change
when dragging to another display.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/stacking_controll...
File ash/wm/stacking_controller.cc (right):

https://codereview.chromium.org/13896026/diff/130005/ash/wm/stacking_controll...
ash/wm/stacking_controller.cc:91: internal::kShellWindowId_DockContainer);
nit: multi-line block requires curlies {}

[varkha, 2013-06-12 04:52:13]

Thanks for all the comments. The CL looks much simpler now with no property.
There are couple files where I have already made typo corrections and now those
are the only changes in those files - hopefully this is OK. Docking resizer is
now always chained in allowing it to snap windows when necessary.
PTAL.

https://codereview.chromium.org/13896026/diff/192002/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/192002/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:148: if (!IsWindowDocked(child))
Not anymore.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.cc (right):

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:177: SetChildBoundsDirect(child,
requested_bounds);
Added comment. This gets called for resizes and moves and so needs to allow the
requested bounds to be set (or the window will not be resizable or movable while
docked).

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.cc:300: alignment_ =
GetDockEdge(dock_container_->children()[0]) == DOCK_EDGE_LEFT ?
That actually forced me to simplify code quite a bit (a good thing). Thanks and
should be Done.

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
File ash/wm/dock/dock_layout_manager.h (right):

https://codereview.chromium.org/13896026/diff/211001/ash/wm/dock/dock_layout_...
ash/wm/dock/dock_layout_manager.h:33: // DockLayoutManager is responsible for
organizing windows within the
I now try to avoid using "dock" as a term for the area where it is confusing and
rather mention "windows docked to the side of a screen" now. I still use
"dock_container" and kShellWindowId_DockContainer, I hope those are OK since
they don't really overlap with the other similar items in code and are shorter
than kShellWindowId_ContainerOfWindowsDockedToSide.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/activation_contro...
File ash/wm/activation_controller.cc (right):

https://codereview.chromium.org/13896026/diff/130005/ash/wm/activation_contro...
ash/wm/activation_controller.cc:46: kShellWindowId_DockContainer,
On 2013/06/11 17:59:31, flackr wrote:
> I think this should probably be above PanelContainer with respect to
activation.
> I think the user would expect docked windows to be focused before panels.

Done.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/ash_focus_rules.cc
File ash/wm/ash_focus_rules.cc (right):

https://codereview.chromium.org/13896026/diff/130005/ash/wm/ash_focus_rules.c...
ash/wm/ash_focus_rules.cc:31: internal::kShellWindowId_DockContainer,
On 2013/06/11 17:59:31, flackr wrote:
> Here tool.

Done.

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

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:330: abs(bounds.x() -
dock_bounds.x()) < kMagnetismWidth) {
On 2013/06/11 17:59:31, flackr wrote:
> Snapping windows during drags seems like something the docked window resizer
> should do.

Done.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_layout_manager_unittest.cc (right):

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager_unittest.cc:156:
EXPECT_EQ(CorrectContainerIdDuringDrag(), window->parent()->id());
On 2013/06/11 17:59:31, flackr wrote:
> Shouldn't the window be in the correct container during drag after the drag
has
> started?

Done.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager_unittest.cc:180: }
On 2013/06/11 17:59:31, flackr wrote:
> The snapping behavior is already tested in panel_window_resizer. This should
> probably just drag far enough to ensure the window is detached (and expect so)
> since not doing so might invalidate the rest of the test. But I think the
panel
> snapping behavior can remain tested by panel window resizer.

Done.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager_unittest.cc:191: // Release the mouse
and the panel should be attached to the dock.
Whether docking succeeds actually depends on the drag vector (parameter) and so
is tested outside of this method by the caller.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager_unittest.cc:199: void
WindowInScreen(aura::Window* window) {
On 2013/06/11 17:59:31, flackr wrote:
> This doesn't seem to be used.

Done.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager_unittest.cc:247:
INSTANTIATE_TEST_CASE_P(PanelsOrNormal, DockLayoutManagerTest, testing::Bool());
On 2013/06/11 17:59:31, flackr wrote:
> Rather than adding a bool parameter can you pass the window type as the
> parameter (i.e. testing::Values(aura::client::WINDOW_TYPE_NORMAL,
> aura::client::WINDOW_TYPE_PANEL) and use the type directly in
> CreateTestWindowInShellWithDelegateAndType?

Done.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer_unittest.cc (right):

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:151: DragMove(0, 5);
Removed this initial drag so, Done.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:239: // the screen edge it does
not get attached to the dock and stays in WS
On 2013/06/11 17:59:31, flackr wrote:
> s/WS/the workspace

Done.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:252: // Verifies a window can be
dragged and detached and then attached to the dock.
On 2013/06/11 17:59:31, flackr wrote:
> I think these comments are confusing, detaching the window is only applicable
to
> panel windows however the test would seem to be for both panel and non panel
> windows. Since detaching is already verified in panel_window_resizer can we
> assume it works here and just test the dock behavior?

Done.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:394: if (test_panels()) {
On 2013/06/11 17:59:31, flackr wrote:
> I'm a little confused about what this is doing. It is done in two tests
though,
> perhaps you could add a helper function and elaborate on why there are
different
> offsets for these.

Done.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:559:
INSTANTIATE_TEST_CASE_P(PanelsOrNormal, DockWindowResizerTest, testing::Bool());
On 2013/06/11 17:59:31, flackr wrote:
> As before, perhaps use window type as parameter.

Done.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/panels/panel_wind...
File ash/wm/panels/panel_window_resizer.cc (right):

https://codereview.chromium.org/13896026/diff/130005/ash/wm/panels/panel_wind...
ash/wm/panels/panel_window_resizer.cc:194: if (!was_attached_ &&
!IsWindowDocked(GetTarget())) {
Eliminating window property made this redundant I think so this file is only
changed to fix the bug with moving panels near screen edge.

https://codereview.chromium.org/13896026/diff/130005/ash/wm/panels/panel_wind...
ash/wm/panels/panel_window_resizer.cc:211:
GetTarget()->SetDefaultParentByRootWindow(root_window_,
We end up using a root pointed by the |near_last_mouse_location| and not really
using the first argument. StackingController::GetDefaultParent() is ignoring the
|context| argument and calling wm::GetRootWindowMatching(gfx::Rect&) to get the
root. I will change the argument here and in DockedWindowResizer (and that
removes the need for root_window_ members) because it is a bit confusing and add
a comment but this will not change the behavior. Can you take a look at the code
in
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/stacking_co...
and see if it is doing the right thing ignoring the |context| argument?

https://codereview.chromium.org/13896026/diff/130005/ash/wm/stacking_controll...
File ash/wm/stacking_controller.cc (right):

https://codereview.chromium.org/13896026/diff/130005/ash/wm/stacking_controll...
ash/wm/stacking_controller.cc:91: internal::kShellWindowId_DockContainer);
Eliminating the window property made this piece unnecessary I think.

[sky, 2013-06-12 21:50:17]

https://codereview.chromium.org/13896026/diff/260001/ash/shell_window_ids.h
File ash/shell_window_ids.h (right):

https://codereview.chromium.org/13896026/diff/260001/ash/shell_window_ids.h#n...
ash/shell_window_ids.h:54: const int kShellWindowId_DockContainer = 8;
DockedContainer?

https://codereview.chromium.org/13896026/diff/260001/ash/wm/activation_contro...
File ash/wm/activation_controller.cc (right):

https://codereview.chromium.org/13896026/diff/260001/ash/wm/activation_contro...
ash/wm/activation_controller.cc:42: // Panel, launcher and status are
intentionally checked after other
nit: add docked here too.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/dock_types.h
File ash/wm/dock/dock_types.h (right):

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/dock_types.h...
ash/wm/dock/dock_types.h:14: DOCK_ALIGNMENT_NONE,
DOCKED_ALIGNMENT?

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

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:69: }
Remove observer on all child windows.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:157: aura::Window* child, const
gfx::Rect& requested_bounds) {
nit: when you wrap, one param per line.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:177:
DCHECK(!dock_container_->children().empty());
How come this is a DCHECK? How do you know there is at least one child window?
284 checks for empty.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:179: gfx::Rect dock_bounds =
dock_container_->bounds();
const gfx::Rect&

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:221: aura::Window* window, bool
visible) {
nit: one param per line.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:230: aura::Window* lost_active) {
nit: indentation is off.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:232: if (gained_active &&
gained_active->parent() == dock_container_)
Seems like this should be an ancestor check. That is, if gained_active if a
descendant of dock_container_, and if so find the child of dock_container_.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:250: for (size_t i = 0; i <
dock_container_->children().size(); ++i) {
Might this end up doing n Relayouts (n=number of children)?

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

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.h:46: : public aura::LayoutManager,
BaseLayoutManager provides default implementation for some things, are you sure
you don't want to subclass it? If not, you should look through the
implementation and make sure there isn't anything you've missed. I took a quick
look and at least your implementation of OnChildWindowVisibilityChanged doesn't
do a lot of what BaseLayoutManager does. You need to take a closer look to make
sure you don't miss things.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.h:56: // Call Shutdown() before
deleting children of dock_container.
This comment is misleading. AFAICT Shutdown() isn't needed before children are
removed, it's needed to make sure observers are removed before objects are
destroyed. Are you sure this is even needed? Can you instead delete objects in
the right order? If not, update comment.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer.h:67: gfx::Point last_mouse_location_;
This is not a good name since the drag may be the result of a touch. Maybe
last_location_?

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer_unittest.cc (right):

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:101: ASSERT_TRUE(resizer_.get());
Remember that ASSERTS only return out of the current function. You likely need
to wrap all calls to method that have asserts in ASSERT_NO_FATAL_FAILURE,
otherwise the test will continue even though this assert triggered.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/panels/panel_wind...
File ash/wm/panels/panel_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/260001/ash/wm/panels/panel_wind...
ash/wm/panels/panel_window_resizer.h:68: gfx::Point last_mouse_location_;
last_location_ (since not necessarily mouse).

[varkha, 2013-06-13 05:02:11]

Thanks for the comments. PTAL.

https://codereview.chromium.org/13896026/diff/260001/ash/shell_window_ids.h
File ash/shell_window_ids.h (right):

https://codereview.chromium.org/13896026/diff/260001/ash/shell_window_ids.h#n...
ash/shell_window_ids.h:54: const int kShellWindowId_DockContainer = 8;
On 2013/06/12 21:50:17, sky wrote:
> DockedContainer?

Done.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/activation_contro...
File ash/wm/activation_controller.cc (right):

https://codereview.chromium.org/13896026/diff/260001/ash/wm/activation_contro...
ash/wm/activation_controller.cc:42: // Panel, launcher and status are
intentionally checked after other
On 2013/06/12 21:50:17, sky wrote:
> nit: add docked here too.

Done.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/dock_types.h
File ash/wm/dock/dock_types.h (right):

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/dock_types.h...
ash/wm/dock/dock_types.h:14: DOCK_ALIGNMENT_NONE,
On 2013/06/12 21:50:17, sky wrote:
> DOCKED_ALIGNMENT?

Done.

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

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:69: }
On 2013/06/12 21:50:17, sky wrote:
> Remove observer on all child windows.

Done.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:157: aura::Window* child, const
gfx::Rect& requested_bounds) {
Done (here and above).

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:177:
DCHECK(!dock_container_->children().empty());
My logic was that since the only assignments of anything other than NONE to
alignment_ happen after the check for children().empty() the check above is
sufficient.  It should not be possible to get into this method with all children
removed and alignment_ not set to NONE in Relayout. I have added a run-time
check as well but I want that assertion.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:179: gfx::Rect dock_bounds =
dock_container_->bounds();
On 2013/06/12 21:50:17, sky wrote:
> const gfx::Rect&

Done.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:221: aura::Window* window, bool
visible) {
On 2013/06/12 21:50:17, sky wrote:
> nit: one param per line.

Done.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:230: aura::Window* lost_active) {
On 2013/06/12 21:50:17, sky wrote:
> nit: indentation is off.

Done.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:232: if (gained_active &&
gained_active->parent() == dock_container_)
On 2013/06/12 21:50:17, sky wrote:
> Seems like this should be an ancestor check. That is, if gained_active if a
> descendant of dock_container_, and if so find the child of dock_container_.

Done.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:250: for (size_t i = 0; i <
dock_container_->children().size(); ++i) {
More like 3xN because of upstream and downstream visibility update messages. Of
course Relayout is running an N loop so this is quite bad. We are lucky there
are not too many windows usually. This should be fixable.

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

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.h:46: : public aura::LayoutManager,
Looking at the BaseLayoutManager it deals with many scenarios that are not
necessary or correct such as ensuring that all windows fit in work area or
dealing with insets. Of course we can override all or some of that, but from
what I could see the similarity is very small. We will likely not need
side-maximized windows, in fact the eventual layout will probably be forced
similar to attached panels. I will be watching this as the behavior develops
with actual layout and size manipulation.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.h:56: // Call Shutdown() before
deleting children of dock_container.
I checked the logic and it seems that normal terminating sequence should be fine
without a need for this. I will run some trybot tests just to be sure.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer.h:67: gfx::Point last_mouse_location_;
On 2013/06/12 21:50:17, sky wrote:
> This is not a good name since the drag may be the result of a touch. Maybe
> last_location_?

Done.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer_unittest.cc (right):

https://codereview.chromium.org/13896026/diff/260001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:101: ASSERT_TRUE(resizer_.get());
On 2013/06/12 21:50:17, sky wrote:
> Remember that ASSERTS only return out of the current function. You likely need
> to wrap all calls to method that have asserts in ASSERT_NO_FATAL_FAILURE,
> otherwise the test will continue even though this assert triggered.

Done.

https://codereview.chromium.org/13896026/diff/260001/ash/wm/panels/panel_wind...
File ash/wm/panels/panel_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/260001/ash/wm/panels/panel_wind...
ash/wm/panels/panel_window_resizer.h:68: gfx::Point last_mouse_location_;
On 2013/06/12 21:50:17, sky wrote:
> last_location_ (since not necessarily mouse).

Done.

[sky, 2013-06-13 15:36:45]

LGTM

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

https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:257: new_state == ash::SHELF_HIDDEN;
nit: no need to wrap here.

[flackr, 2013-06-13 17:28:29]

LGTM

[stevenjb, 2013-06-13 17:54:40]

lgtm

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

https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:183: return;
Since we handle the error condition, we should use LOG(ERROR) inside the if()
instead of DCHECK. Alternately remove the if/return if logically this should
never be posisble, even in an edge case.

https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:307: }
nit: don't need {}

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

https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.h:61: // unless when other windows are
already docked on the other side or
nit: s/unless when/except when/

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

https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer.cc:39: } // namespace
nit: 2 spaces before //

https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer.cc:115: return;
nit: invert if, no else (prefer early exit)

[varkha, 2013-06-13 20:19:14]

Thanks everybody. This needed a bit more work because chained resizers break the
DragWindowResizerTest. The test was assuming that the end of the chain is the
DragWindowResizer and accessing its internals after casting the pointer. This
assumption is now broken so I had to get the DragWindowResizer exposed to allow
proper access for the test to resizer internals. Not pretty but any other
alternative seemed to me just as troublesome if not more.

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

https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:183: return;
On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> Since we handle the error condition, we should use LOG(ERROR) inside the if()
> instead of DCHECK. Alternately remove the if/return if logically this should
> never be posisble, even in an edge case.

Done.

https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:257: new_state == ash::SHELF_HIDDEN;
On 2013/06/13 15:36:45, sky wrote:
> nit: no need to wrap here.

Done.

https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:307: }
On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> nit: don't need {}

Done.

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

https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.h:61: // unless when other windows are
already docked on the other side or
On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> nit: s/unless when/except when/

Done.

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

https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer.cc:39: } // namespace
On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> nit: 2 spaces before //

Done.

https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer.cc:115: return;
On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> nit: invert if, no else (prefer early exit)

Done.

[flackr, 2013-06-14 18:31:32]

Hmm, I knew that chaining resizers would get tricky when we make assumptions
about the type of the outside one. Ideally we wouldn't have to rely on any
internals, but if we generalized the concept of a chained window resizer and
added a protected method to get a DragWindowResizer* or NULL you could walk the
chain until you found the correct one without having to modify the window
resizer factory constructor.

I believe I've seen this pattern elsewhere, but I can't recall where. This might
make for a slightly larger change and perhaps it would make sense to temporarily
construct a known chain of resizers just for the unit test with a TODO.

On 2013/06/13 20:19:14, varkha wrote:
> Thanks everybody. This needed a bit more work because chained resizers break
the
> DragWindowResizerTest. The test was assuming that the end of the chain is the
> DragWindowResizer and accessing its internals after casting the pointer. This
> assumption is now broken so I had to get the DragWindowResizer exposed to
allow
> proper access for the test to resizer internals. Not pretty but any other
> alternative seemed to me just as troublesome if not more.
> 
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> File ash/wm/dock/docked_window_layout_manager.cc (right):
> 
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> ash/wm/dock/docked_window_layout_manager.cc:183: return;
> On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> > Since we handle the error condition, we should use LOG(ERROR) inside the
if()
> > instead of DCHECK. Alternately remove the if/return if logically this should
> > never be posisble, even in an edge case.
> 
> Done.
> 
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> ash/wm/dock/docked_window_layout_manager.cc:257: new_state ==
ash::SHELF_HIDDEN;
> On 2013/06/13 15:36:45, sky wrote:
> > nit: no need to wrap here.
> 
> Done.
> 
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> ash/wm/dock/docked_window_layout_manager.cc:307: }
> On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> > nit: don't need {}
> 
> Done.
> 
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> File ash/wm/dock/docked_window_layout_manager.h (right):
> 
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> ash/wm/dock/docked_window_layout_manager.h:61: // unless when other windows
are
> already docked on the other side or
> On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> > nit: s/unless when/except when/
> 
> Done.
> 
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> File ash/wm/dock/docked_window_resizer.cc (right):
> 
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> ash/wm/dock/docked_window_resizer.cc:39: } // namespace
> On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> > nit: 2 spaces before //
> 
> Done.
> 
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> ash/wm/dock/docked_window_resizer.cc:115: return;
> On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> > nit: invert if, no else (prefer early exit)
> 
> Done.

[flackr, 2013-06-14 18:31:49]

https://codereview.chromium.org/13896026/diff/321001/ash/wm/window_resizer.h
File ash/wm/window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/321001/ash/wm/window_resizer.h#...
ash/wm/window_resizer.h:55: virtual const gfx::Point&
GetInitialLocationForTest() const = 0;
This should be protected since it's only for tests.

[varkha, 2013-06-14 18:42:53]

I thought about it. The only thing I don't like about it is that if we ever need
other interfaces out of the chain we would need to add another method to get
say, PanelWindowResizer* or DockedWindowResizer*. Of course the current approach
with a modified factory is no better in this regard. Still I'll give it a try.
And oh, all the files are already pulled into this CL because of initial
location method so this is not a concern.

[sky, 2013-06-14 19:46:48]

On 2013/06/14 18:31:32, flackr wrote:
> Hmm, I knew that chaining resizers would get tricky when we make assumptions
> about the type of the outside one. Ideally we wouldn't have to rely on any
> internals, but if we generalized the concept of a chained window resizer and
> added a protected method to get a DragWindowResizer* or NULL you could walk
the
> chain until you found the correct one without having to modify the window
> resizer factory constructor.
> 
> I believe I've seen this pattern elsewhere, but I can't recall where. This
might
> make for a slightly larger change and perhaps it would make sense to
temporarily
> construct a known chain of resizers just for the unit test with a TODO.

I feel that chaining (while used in other OSs) is a particularly fragile for
code like this. I agree that WindowResizer is due for a refactoring (Varkha add
the TODO there) but figured it was out of scope for this patch. I believe what
is needed is an uber windowresizer. UberWindowResizer delegates to a per window
specific ContainerWindowResizer. ContainerWindowResizer can tell the
WindowResizer that it needs to move a window to a new container at which point
UberWindowResizer determines the new ContainerWindowResizer. I think we should
also bake in dragging across displays to UberWindowResizer.

I too don't like CreateWindowResizerExposeDragResizer. I've suggested in the
interm Valery add a DragWindowResizer::instance().

  -Scott

> 
> On 2013/06/13 20:19:14, varkha wrote:
> > Thanks everybody. This needed a bit more work because chained resizers break
> the
> > DragWindowResizerTest. The test was assuming that the end of the chain is
the
> > DragWindowResizer and accessing its internals after casting the pointer.
This
> > assumption is now broken so I had to get the DragWindowResizer exposed to
> allow
> > proper access for the test to resizer internals. Not pretty but any other
> > alternative seemed to me just as troublesome if not more.
> > 
> >
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> > File ash/wm/dock/docked_window_layout_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> > ash/wm/dock/docked_window_layout_manager.cc:183: return;
> > On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> > > Since we handle the error condition, we should use LOG(ERROR) inside the
> if()
> > > instead of DCHECK. Alternately remove the if/return if logically this
should
> > > never be posisble, even in an edge case.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> > ash/wm/dock/docked_window_layout_manager.cc:257: new_state ==
> ash::SHELF_HIDDEN;
> > On 2013/06/13 15:36:45, sky wrote:
> > > nit: no need to wrap here.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> > ash/wm/dock/docked_window_layout_manager.cc:307: }
> > On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> > > nit: don't need {}
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> > File ash/wm/dock/docked_window_layout_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> > ash/wm/dock/docked_window_layout_manager.h:61: // unless when other windows
> are
> > already docked on the other side or
> > On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> > > nit: s/unless when/except when/
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> > File ash/wm/dock/docked_window_resizer.cc (right):
> > 
> >
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> > ash/wm/dock/docked_window_resizer.cc:39: } // namespace
> > On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> > > nit: 2 spaces before //
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/13896026/diff/275001/ash/wm/dock/docked_windo...
> > ash/wm/dock/docked_window_resizer.cc:115: return;
> > On 2013/06/13 17:54:41, stevenjb (chromium) wrote:
> > > nit: invert if, no else (prefer early exit)
> > 
> > Done.

[varkha, 2013-06-14 22:11:00]

Done.

[varkha, 2013-06-14 22:13:08]

PTAL. The static accessor for the last created DragWindowResizer is hopefully
temporary solution to the fragile use of resizers chain in tests.

https://codereview.chromium.org/13896026/diff/321001/ash/wm/window_resizer.h
File ash/wm/window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/321001/ash/wm/window_resizer.h#...
ash/wm/window_resizer.h:55: virtual const gfx::Point&
GetInitialLocationForTest() const = 0;
On 2013/06/14 18:31:50, flackr wrote:
> This should be protected since it's only for tests.

Done.

[flackr, 2013-06-17 18:49:41]

This approach with the last instance accessor LGTM. A few nits, overridden
protected methods should be protected (didn't comment on every case).

https://codereview.chromium.org/13896026/diff/351001/ash/wm/default_window_re...
File ash/wm/default_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/351001/ash/wm/default_window_re...
ash/wm/default_window_resizer.h:42: virtual const gfx::Point&
GetInitialLocationForTest() const OVERRIDE;
protected

https://codereview.chromium.org/13896026/diff/351001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/351001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer.h:48: virtual const gfx::Point&
GetInitialLocationForTest() const OVERRIDE;
protected

https://codereview.chromium.org/13896026/diff/351001/ash/wm/drag_window_resiz...
File ash/wm/drag_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/351001/ash/wm/drag_window_resiz...
ash/wm/drag_window_resizer.h:58: static DragWindowResizer* instance() { return
instance_; }
nit: I think we should call this last_instance() so as not to imply that there
is only ever 1 instance.
nit: s/return/Returns

https://codereview.chromium.org/13896026/diff/351001/ash/wm/panels/panel_wind...
File ash/wm/panels/panel_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/351001/ash/wm/panels/panel_wind...
ash/wm/panels/panel_window_resizer.h:41: virtual const gfx::Point&
GetInitialLocationForTest() const OVERRIDE;
protected

[varkha, 2013-06-17 19:05:36]

Done.

https://codereview.chromium.org/13896026/diff/351001/ash/wm/default_window_re...
File ash/wm/default_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/351001/ash/wm/default_window_re...
ash/wm/default_window_resizer.h:42: virtual const gfx::Point&
GetInitialLocationForTest() const OVERRIDE;
On 2013/06/17 18:49:42, flackr wrote:
> protected

Done.

https://codereview.chromium.org/13896026/diff/351001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/351001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer.h:48: virtual const gfx::Point&
GetInitialLocationForTest() const OVERRIDE;
On 2013/06/17 18:49:42, flackr wrote:
> protected

Done.

https://codereview.chromium.org/13896026/diff/351001/ash/wm/drag_window_resiz...
File ash/wm/drag_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/351001/ash/wm/drag_window_resiz...
ash/wm/drag_window_resizer.h:58: static DragWindowResizer* instance() { return
instance_; }
On 2013/06/17 18:49:42, flackr wrote:
> nit: I think we should call this last_instance() so as not to imply that there
> is only ever 1 instance.
> nit: s/return/Returns

Done.

https://codereview.chromium.org/13896026/diff/351001/ash/wm/panels/panel_wind...
File ash/wm/panels/panel_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/351001/ash/wm/panels/panel_wind...
ash/wm/panels/panel_window_resizer.h:41: virtual const gfx::Point&
GetInitialLocationForTest() const OVERRIDE;
On 2013/06/17 18:49:42, flackr wrote:
> protected

Done.

[sky, 2013-06-17 19:49:46]

https://codereview.chromium.org/13896026/diff/347002/ash/wm/drag_window_resiz...
File ash/wm/drag_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/347002/ash/wm/drag_window_resiz...
ash/wm/drag_window_resizer.h:75: static DragWindowResizer* last_instance;
Why the last here? Isn't this the current instance? This should also end up with
a '_' (it's a member). So, I suggest instance_. Also, I recommend nuking
instance() entirely and using instance_ directly.

https://codereview.chromium.org/13896026/diff/347002/ash/wm/window_resizer.h
File ash/wm/window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/347002/ash/wm/window_resizer.h#...
ash/wm/window_resizer.h:66: virtual const gfx::Point&
GetInitialLocationForTest() const = 0;
There is no point in hiding this. Make it public and nuke the 'ForTest'. This
way you can get rid of the friends and all that.

[flackr, 2013-06-17 20:02:39]

https://codereview.chromium.org/13896026/diff/347002/ash/wm/drag_window_resiz...
File ash/wm/drag_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/347002/ash/wm/drag_window_resiz...
ash/wm/drag_window_resizer.h:75: static DragWindowResizer* last_instance;
On 2013/06/17 19:49:47, sky wrote:
> Why the last here? Isn't this the current instance? This should also end up
with
> a '_' (it's a member). So, I suggest instance_. Also, I recommend nuking
> instance() entirely and using instance_ directly.

Ah, my mistake, I had just recommended this to avoid confusing this for a
singleton class. Agree with nuking the accessor.

[varkha, 2013-06-17 20:34:57]

Should be done.

https://codereview.chromium.org/13896026/diff/347002/ash/wm/drag_window_resiz...
File ash/wm/drag_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/347002/ash/wm/drag_window_resiz...
ash/wm/drag_window_resizer.h:75: static DragWindowResizer* last_instance;
On 2013/06/17 19:49:47, sky wrote:
> Why the last here? Isn't this the current instance? This should also end up
with
> a '_' (it's a member). So, I suggest instance_. Also, I recommend nuking
> instance() entirely and using instance_ directly.

Done.

https://codereview.chromium.org/13896026/diff/347002/ash/wm/drag_window_resiz...
ash/wm/drag_window_resizer.h:75: static DragWindowResizer* last_instance;
On 2013/06/17 20:02:40, flackr wrote:
> On 2013/06/17 19:49:47, sky wrote:
> > Why the last here? Isn't this the current instance? This should also end up
> with
> > a '_' (it's a member). So, I suggest instance_. Also, I recommend nuking
> > instance() entirely and using instance_ directly.
> 
> Ah, my mistake, I had just recommended this to avoid confusing this for a
> singleton class. Agree with nuking the accessor.

Done.

https://codereview.chromium.org/13896026/diff/347002/ash/wm/window_resizer.h
File ash/wm/window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/347002/ash/wm/window_resizer.h#...
ash/wm/window_resizer.h:66: virtual const gfx::Point&
GetInitialLocationForTest() const = 0;
On 2013/06/17 19:49:47, sky wrote:
> There is no point in hiding this. Make it public and nuke the 'ForTest'. This
> way you can get rid of the friends and all that.

Done.

[sky, 2013-06-17 20:37:22]

LGTM

https://codereview.chromium.org/13896026/diff/379001/ash/wm/drag_window_resiz...
File ash/wm/drag_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/379001/ash/wm/drag_window_resiz...
ash/wm/drag_window_resizer.h:68: // Last instance created - for use by the
DragWindowResizerTest.
Last instance created -> Current instance

[commit-bot: I haz the power, 2013-06-17 20:41:59]

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

[commit-bot: I haz the power, 2013-06-17 20:42:07]

Failed to apply patch for ash/wm/drag_window_resizer_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ash/wm/drag_window_resizer_unittest.cc
  Hunk #2 FAILED at 107.
  Hunk #3 succeeded at 151 (offset 1 line).
  Hunk #4 succeeded at 169 (offset 1 line).
  Hunk #5 succeeded at 186 (offset 1 line).
  Hunk #6 succeeded at 215 (offset 1 line).
  Hunk #7 succeeded at 240 (offset 1 line).
  Hunk #8 succeeded at 275 (offset 1 line).
  Hunk #9 succeeded at 292 (offset 1 line).
  Hunk #10 succeeded at 317 (offset 1 line).
  Hunk #11 succeeded at 328 (offset 1 line).
  Hunk #12 succeeded at 338 (offset 1 line).
  Hunk #13 succeeded at 349 (offset 1 line).
  Hunk #14 succeeded at 383 (offset 1 line).
  Hunk #15 succeeded at 403 (offset 1 line).
  Hunk #16 succeeded at 437 (offset 1 line).
  Hunk #17 succeeded at 452 (offset 1 line).
  Hunk #18 succeeded at 467 (offset 1 line).
  Hunk #19 succeeded at 482 (offset 1 line).
  Hunk #20 succeeded at 498 (offset 1 line).
  Hunk #21 succeeded at 513 (offset 1 line).
  1 out of 21 hunks FAILED -- saving rejects to file
ash/wm/drag_window_resizer_unittest.cc.rej

Patch:       ash/wm/drag_window_resizer_unittest.cc
Index: ash/wm/drag_window_resizer_unittest.cc
diff --git a/ash/wm/drag_window_resizer_unittest.cc
b/ash/wm/drag_window_resizer_unittest.cc
index
1f632de4b8fc04bbb116b0635e594c60f9f62472..c8e751be4a62bd64c00836377315cb58bc7d7808
100644
--- a/ash/wm/drag_window_resizer_unittest.cc
+++ b/ash/wm/drag_window_resizer_unittest.cc
@@ -94,10 +94,10 @@ class DragWindowResizerTest : public test::AshTestBase {
   }
 
  protected:
-  gfx::Point CalculateDragPoint(const DragWindowResizer& resizer,
+  gfx::Point CalculateDragPoint(const WindowResizer& resizer,
                                 int delta_x,
                                 int delta_y) const {
-    gfx::Point location = resizer.GetInitialLocationInParentForTest();
+    gfx::Point location = resizer.GetInitialLocation();
     location.set_x(location.x() + delta_x);
     location.set_y(location.y() + delta_y);
     return location;
@@ -107,14 +107,6 @@ class DragWindowResizerTest : public test::AshTestBase {
     return Shell::GetPrimaryRootWindowController()->GetShelfLayoutManager();
   }
 
-  static DragWindowResizer* CreateDragWindowResizer(
-      aura::Window* window,
-      const gfx::Point& point_in_parent,
-      int window_component) {
-    return static_cast<DragWindowResizer*>(CreateWindowResizer(
-        window, point_in_parent, window_component).release());
-  }
-
   aura::test::TestWindowDelegate delegate_;
   aura::test::TestWindowDelegate delegate2_;
   aura::test::TestWindowDelegate delegate3_;
@@ -150,7 +142,7 @@ TEST_F(DragWindowResizerTest, WindowDragWithMultiDisplays) {
   EXPECT_EQ(root_windows[0], window_->GetRootWindow());
   {
     // Grab (0, 0) of the window.
-    scoped_ptr<DragWindowResizer> resizer(CreateDragWindowResizer(
+    scoped_ptr<WindowResizer> resizer(CreateWindowResizer(
         window_.get(), gfx::Point(), HTCAPTION));
     ASSERT_TRUE(resizer.get());
     // Drag the pointer to the right. Once it reaches the right edge of the
@@ -168,7 +160,7 @@ TEST_F(DragWindowResizerTest, WindowDragWithMultiDisplays) {
   EXPECT_EQ(root_windows[0], window_->GetRootWindow());
   {
     // Grab (0, 0) of the window and move the pointer to (790, 10).
-    scoped_ptr<DragWindowResizer> resizer(CreateDragWindowResizer(
+    scoped_ptr<WindowResizer> resizer(CreateWindowResizer(
         window_.get(), gfx::Point(), HTCAPTION));
     ASSERT_TRUE(resizer.get());
     resizer->Drag(CalculateDragPoint(*resizer, 790, 10), 0);
@@ -185,7 +177,7 @@ TEST_F(DragWindowResizerTest, WindowDragWithMultiDisplays) {
   {
     // Grab the top-right edge of the window and move the pointer to (0, 10)
     // in the secondary root window's coordinates.
-    scoped_ptr<DragWindowResizer> resizer(CreateDragWindowResizer(
+    scoped_ptr<WindowResizer> resizer(CreateWindowResizer(
         window_.get(), gfx::Point(49, 0), HTCAPTION));
     ASSERT_TRUE(resizer.get());
     resizer->Drag(CalculateDragPoint(*resizer, 751, 10), ui::EF_CONTROL_DOWN);
@@ -214,7 +206,7 @@ TEST_F(DragWindowResizerTest,
WindowDragWithMultiDisplaysRightToLeft) {
   EXPECT_EQ(root_windows[1], window_->GetRootWindow());
   {
     // Grab (0, 0) of the window.
-    scoped_ptr<DragWindowResizer> resizer(CreateDragWindowResizer(
+    scoped_ptr<WindowResizer> resizer(CreateWindowResizer(
         window_.get(), gfx::Point(), HTCAPTION));
     ASSERT_TRUE(resizer.get());
     // Move the mouse near the right edge, (798, 0), of the primary display.
@@ -239,20 +231,22 @@ TEST_F(DragWindowResizerTest, DragWindowController) {
   EXPECT_EQ(root_windows[0], window_->GetRootWindow());
   EXPECT_FLOAT_EQ(1.0f, window_->layer()->opacity());
   {
-    scoped_ptr<DragWindowResizer> resizer(CreateDragWindowResizer(
+    scoped_ptr<WindowResizer> resizer(CreateWindowResizer(
         window_.get(), gfx::Point(), HTCAPTION));
     ASSERT_TRUE(resizer.get());
-    EXPECT_FALSE(resizer->drag_window_controller_.get());
+    internal::DragWindowResizer* drag_resizer = DragWindowResizer::instance_;
+    ASSERT_TRUE(drag_resizer);
+    EXPECT_FALSE(drag_resizer->drag_window_controller_.get());
 
     // The pointer is inside the primary root. The drag window controller
     // should be NULL.
     resizer->Drag(CalculateDragPoint(*resizer, 10, 10), 0);
-    EXPECT_FALSE(resizer->drag_window_controller_.get());
+    EXPECT_FALSE(drag_resizer->drag_window_controller_.get());
 
     // The window spans both root windows.
     resizer->Drag(CalculateDragPoint(*resizer, 798, 10), 0);
     DragWindowController* controller =
-        resizer->drag_window_controller_.get();
+        drag_resizer->drag_window_controller_.get();
     ASSERT_TRUE(controller);
 
     ASSERT_TRUE(controller->drag_widget_);
@@ -272,7 +266,7 @@ TEST_F(DragWindowResizerTest, DragWindowController) {
 
     // Enter the pointer to the secondary display.
     resizer->Drag(CalculateDragPoint(*resizer, 800, 10), 0);
-    controller = resizer->drag_window_controller_.get();
+    controller = drag_resizer->drag_window_controller_.get();
     ASSERT_TRUE(controller);
     // |window_| should be transparent, and the drag window should be opaque.
     EXPECT_GT(1.0f, window_->layer()->opacity());
@@ -289,10 +283,12 @@ TEST_F(DragWindowResizerTest, DragWindowController) {
   EXPECT_EQ(root_windows[0], window_->GetRootWindow());
   EXPECT_FLOAT_EQ(1.0f, window_->layer()->opacity());
   {
-    scoped_ptr<DragWindowResizer> resizer(CreateDragWindowResizer(
+    scoped_ptr<WindowResizer> resizer(CreateWindowResizer(
         window_.get(), gfx::Point(), HTCAPTION));
     ASSERT_TRUE(resizer.get());
-    EXPECT_FALSE(resizer->drag_window_controller_.get());
+    internal::DragWindowResizer* drag_resizer = DragWindowResizer::instance_;
+    ASSERT_TRUE(drag_resizer);
+    EXPECT_FALSE(drag_resizer->drag_window_controller_.get());
 
     resizer->Drag(CalculateDragPoint(*resizer, 0, 610), 0);
     resizer->RevertDrag();
@@ -312,7 +308,7 @@ TEST_F(DragWindowResizerTest, WarpMousePointer) {
   EXPECT_EQ(MouseCursorEventFilter::WARP_ALWAYS,
             event_filter->mouse_warp_mode_);
   {
-    scoped_ptr<DragWindowResizer> resizer(CreateDragWindowResizer(
+    scoped_ptr<WindowResizer> resizer(CreateWindowResizer(
         window_.get(), gfx::Point(), HTCAPTION));
     // While dragging a window, warp should be allowed.
     EXPECT_EQ(MouseCursorEventFilter::WARP_DRAG,
@@ -323,7 +319,7 @@ TEST_F(DragWindowResizerTest, WarpMousePointer) {
             event_filter->mouse_warp_mode_);
 
   {
-    scoped_ptr<DragWindowResizer> resizer(CreateDragWindowResizer(
+    scoped_ptr<WindowResizer> resizer(CreateWindowResizer(
         window_.get(), gfx::Point(), HTCAPTION));
     EXPECT_EQ(MouseCursorEventFilter::WARP_DRAG,
               event_filter->mouse_warp_mode_);
@@ -333,7 +329,7 @@ TEST_F(DragWindowResizerTest, WarpMousePointer) {
             event_filter->mouse_warp_mode_);
 
   {
-    scoped_ptr<DragWindowResizer> resizer(CreateDragWindowResizer(
+    scoped_ptr<WindowResizer> resizer(CreateWindowResizer(
         window_.get(), gfx::Point(), HTRIGHT));
     // While resizing a window, warp should NOT be allowed.
     EXPECT_EQ(MouseCursorEventFilter::WARP_NONE,
@@ -344,7 +340,7 @@ TEST_F(DragWindowResizerTest, WarpMousePointer) {
             event_filter->mouse_warp_mode_);
 
   {
-    scoped_ptr<DragWindowResizer> resizer(CreateDragWindowResizer(
+    scoped_ptr<WindowResizer> resizer(CreateWindowResizer(
         window_.get(), gfx::Point(), HTRIGHT));
     EXPECT_EQ(MouseCursorEventFilter::WARP_NONE,
               event_filter->mouse_warp_mode_);
@@ -378,7 +374,7 @@ TEST_F(DragWindowResizerTest, CursorDeviceScaleFactor) {
                                Shell::GetScreen()->GetPrimaryDisplay());
     EXPECT_EQ(root_windows[0], window_->GetRootWindow());
     // Grab (0, 0) of the window.
-    scoped_ptr<DragWindowResizer> resizer(CreateDragWindowResizer(
+    scoped_ptr<WindowResizer> resizer(CreateWindowResizer(
         window_.get(), gfx::Point(), HTCAPTION));
     EXPECT_EQ(1.0f, cursor_test_api.GetDisplay().device_scale_factor());
     ASSERT_TRUE(resizer.get());
@@ -398,7 +394,7 @@ TEST_F(DragWindowResizerTest, CursorDeviceScaleFactor) {
         Shell::GetScreen()->GetDisplayNearestWindow(root_windows[1]));
     EXPECT_EQ(root_windows[1], window_->GetRootWindow());
     // Grab (0, 0) of the window.
-    scoped_ptr<DragWindowResizer> resizer(CreateDragWindowResizer(
+    scoped_ptr<WindowResizer> resizer(CreateWindowResizer(
         window_.get(), gfx::Point(), HTCAPTION));
     EXPECT_EQ(2.0f, cursor_test_api.GetDisplay().device_scale_factor());
     ASSERT_TRUE(resizer.get());
@@ -432,7 +428,7 @@ TEST_F(DragWindowResizerTest, MoveWindowAcrossDisplays) {
     window->SetBoundsInScreen(gfx::Rect(0, 0, 50, 60),
                         …
(message too large)

[commit-bot: I haz the power, 2013-06-18 03:21:44]

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

[varkha, 2013-06-18 03:22:26]

https://codereview.chromium.org/13896026/diff/379001/ash/wm/drag_window_resiz...
File ash/wm/drag_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/379001/ash/wm/drag_window_resiz...
ash/wm/drag_window_resizer.h:68: // Last instance created - for use by the
DragWindowResizerTest.
While it is not possible with user interaction I thought that nothing prevents
creating multiple instances of resizer in which case it would be technically
inaccurate.

[commit-bot: I haz the power, 2013-06-18 05:55:03]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-18 11:19:05]

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

[flackr, 2013-06-18 13:35:03]

https://codereview.chromium.org/13896026/diff/379001/ash/wm/drag_window_resiz...
File ash/wm/drag_window_resizer.h (right):

https://codereview.chromium.org/13896026/diff/379001/ash/wm/drag_window_resiz...
ash/wm/drag_window_resizer.h:68: // Last instance created - for use by the
DragWindowResizerTest.
On 2013/06/18 03:22:27, varkha wrote:
> While it is not possible with user interaction I thought that nothing prevents
> creating multiple instances of resizer in which case it would be technically
> inaccurate.

It is actually possible by dragging one window with touch and the other with the
mouse, just rather uncommon.

[commit-bot: I haz the power, 2013-06-18 15:37:08]

Change committed as 207002