Windows docking should get triggered by pressing against the screen edge

Windows docking should get triggered by pressing against the screen edge

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

[varkha, 2013-08-31 07:11:01]

Windows docking should get triggered by pressing against the screen edge 11

[varkha, 2013-09-04 16:35:08]

Can you please take a look. This will need further simplification / change once
https://codereview.chromium.org/23471004/ lands.

[flackr, 2013-09-06 02:18:42]

https://codereview.chromium.org/23431009/diff/54001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/23431009/diff/54001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:33: const int kMaxVerticalPercentage
= 90;
Just to simplify this CL, adding the max vertical percentage seems like a
tangential change that could be done as a separate CL.

https://codereview.chromium.org/23431009/diff/54001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:555: if (height ==
work_area.height())
This allows the height to be greater than kMaxVerticalPercentage as long as it's
less than work_area.height(). Setting height to min of the percentage and height
would be cleaner.

https://codereview.chromium.org/23431009/diff/54001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_resizer.cc (right):

https://codereview.chromium.org/23431009/diff/54001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_resizer.cc:257: bool should_dock = is_docked_;
if should_dock == is_docked_, can we just use is_docked_ below?

https://codereview.chromium.org/23431009/diff/54001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_resizer.cc:264: attached_panel =
Won't moving this check here make it possible to dock attached panels by
resizing them?

https://codereview.chromium.org/23431009/diff/54001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_resizer.cc:265: window->type() ==
aura::client::WINDOW_TYPE_PANEL &&
nit: indent 4 spaces.

https://codereview.chromium.org/23431009/diff/54001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_resizer.cc:299: if (was_docked_ &&
was_docked_ is true iff the window was docked before the drag. Shouldn't we
retain restore bounds if the window was not initially docked?

https://codereview.chromium.org/23431009/diff/54001/ash/wm/drag_window_resize...
File ash/wm/drag_window_resizer.cc (right):

https://codereview.chromium.org/23431009/diff/54001/ash/wm/drag_window_resize...
ash/wm/drag_window_resizer.cc:114: if (!dst_display.is_valid())
What happens in this case? If the drag crossed into another monitor and then
went outside the display (not sure when this happens) wouldn't that leave the
window parented to the first display but in the second monitor's bounds?

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/snap_siz...
File ash/wm/workspace/snap_sizer.cc (right):

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/snap_siz...
ash/wm/workspace/snap_sizer.cc:48: // Fill the |usable_width_| list with items
from |kIdealWidth| which fit on
Looks like an out of date comment, this returns the list.

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/snap_siz...
ash/wm/workspace/snap_sizer.cc:51: // value in the |kIdealWidth| list (to get a
step between the values).
Comment |allow_max| and |allow_dock|.

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/snap_siz...
ash/wm/workspace/snap_sizer.cc:77: //  }
Remove commented code.

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/snap_siz...
ash/wm/workspace/snap_sizer.cc:109: !allow_dock ||
nit: Move up to previous line.

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/snap_siz...
ash/wm/workspace/snap_sizer.cc:114: usable_width_ = BuildIdealWidthList(window,
allow_snap, allow_dock);
In BuildIdealWidthList, you call allow_snap allox_max. It's unclear why these
are the same.

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/snap_siz...
ash/wm/workspace/snap_sizer.cc:235: if (!CanDockWindow(window_, edge_))
If !CanDockWindow, 0 shouldn't be in the list of usable widths. Can we not just
check for a usable width of 0?

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/snap_siz...
ash/wm/workspace/snap_sizer.cc:237: // Avoid returning true when the last step
is not docking.
This comment is confusing. The last step is the only one that can be docking
right? So you should avoid returning true if the window shouldn't dock, but then
that's the name of the method so self explanatory. Is there some extra condition
to convey here?

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/snap_siz...
ash/wm/workspace/snap_sizer.cc:240: size_index_ ==
static_cast<int>(usable_width_.size()) - 1);
Shouldn't 0 always be the last element in usable_width_? I'm not sure checking
that we're at the end is necessary.

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:394: // Keep the snap phantom on
screen when pointer gets off screen.
This comment sort of describes what we're doing when the condition is not true.
Can you change it to be what the condition is? Maybe something like:
Track the last screen the pointer was on.

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:399: bool destroyed = false;
FYI, I'm not especially fond of this pattern, I put up a CL to see about
changing it: https://chromiumcodereview.appspot.com/23645008/

Does SetBounds have to be called before updating the snap phantom window
necessitating this? It's not especially clear why, perhaps a comment to this
effect.

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:402:
LayoutAttachedWindows(&bounds);
Presumably this cannot delete the resizer since you only check destroyed after
SetBounds? If so, can you move the destroyed_ assignment to just be around the
SetBounds call where it's needed?

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:415:
snap_phantom_window_controller_.reset();
Should snap_sizer_ be reset here too since we're not showing that the window
will snap?

https://codereview.chromium.org/23431009/diff/54001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:441: if (snap_sizer_ &&
wm::CanResizeWindow(window()) &&
Is it possible that snap_sizer_ will not be instantiated when snap_type_ is left
or right? Maybe a DCHECK instead?

[varkha, 2013-09-09 15:38:42]

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/dock/docked...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/dock/docked...
ash/wm/dock/docked_window_layout_manager.cc:33: const int kMaxVerticalPercentage
= 90;
On 2013/09/06 02:18:43, flackr wrote:
> Just to simplify this CL, adding the max vertical percentage seems like a
> tangential change that could be done as a separate CL.

Done.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/dock/docked...
ash/wm/dock/docked_window_layout_manager.cc:555: if (height ==
work_area.height())
On 2013/09/06 02:18:43, flackr wrote:
> This allows the height to be greater than kMaxVerticalPercentage as long as
it's
> less than work_area.height(). Setting height to min of the percentage and
height
> would be cleaner.

This was intentional to target vertically maximized windows rather than just
tall windows. I wanted to preserve vertical size when resized by a user but make
docking more friendly for the tiled windows. I am taking this part out as part
of vertical size split / minimizing.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/dock/docked...
File ash/wm/dock/docked_window_resizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/dock/docked...
ash/wm/dock/docked_window_resizer.cc:257: bool should_dock = is_docked_;
On 2013/09/06 02:18:43, flackr wrote:
> if should_dock == is_docked_, can we just use is_docked_ below?

Done.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/dock/docked...
ash/wm/dock/docked_window_resizer.cc:264: attached_panel =
On 2013/09/06 02:18:43, flackr wrote:
> Won't moving this check here make it possible to dock attached panels by
> resizing them?

No. is_docked_ will be false unless the window was docked before. So resizing a
docked window won't undock but will never dock an undocked window.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/dock/docked...
ash/wm/dock/docked_window_resizer.cc:265: window->type() ==
aura::client::WINDOW_TYPE_PANEL &&
On 2013/09/06 02:18:43, flackr wrote:
> nit: indent 4 spaces.

Done.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/dock/docked...
ash/wm/dock/docked_window_resizer.cc:299: if (was_docked_ &&
On 2013/09/06 02:18:43, flackr wrote:
> was_docked_ is true iff the window was docked before the drag. Shouldn't we
> retain restore bounds if the window was not initially docked?

That part is already handled by existing code in
WorkspaceWindowResizer::CompleteDrag that saves the restore bounds when snapping
(and now docking). What I am doing here is restoring the restore bounds after
they have been cleared by DefaultWindowResizer::Drag. I do this only in case
then a docked window is dragged but not resized inside the dock.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/drag_window...
File ash/wm/drag_window_resizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/drag_window...
ash/wm/drag_window_resizer.cc:114: if (!dst_display.is_valid())
On 2013/09/06 02:18:43, flackr wrote:
> What happens in this case? If the drag crossed into another monitor and then
> went outside the display (not sure when this happens) wouldn't that leave the
> window parented to the first display but in the second monitor's bounds?

I think this only happens when you don't --ash-constrain-pointer-to-root. The
window dragged from the right screen to the left and beyond was disappearing.
This was bothering me during development but I left this out of this CL for now.
The alternative as you point out correctly is not much better.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/s...
File ash/wm/workspace/snap_sizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:48: // Fill the |usable_width_| list with items
from |kIdealWidth| which fit on
On 2013/09/06 02:18:43, flackr wrote:
> Looks like an out of date comment, this returns the list.

Done.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:51: // value in the |kIdealWidth| list (to get a
step between the values).
On 2013/09/06 02:18:43, flackr wrote:
> Comment |allow_max| and |allow_dock|.

Done.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:77: //  }
On 2013/09/06 02:18:43, flackr wrote:
> Remove commented code.

Done.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:109: !allow_dock ||
On 2013/09/06 02:18:43, flackr wrote:
> nit: Move up to previous line.

Done.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:114: usable_width_ = BuildIdealWidthList(window,
allow_snap, allow_dock);
On 2013/09/06 02:18:43, flackr wrote:
> In BuildIdealWidthList, you call allow_snap allox_max. It's unclear why these
> are the same.

Done (renamed allow_max -> allow_snap for consistency).

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:235: if (!CanDockWindow(window_, edge_))
On 2013/09/06 02:18:43, flackr wrote:
> If !CanDockWindow, 0 shouldn't be in the list of usable widths. Can we not
just
> check for a usable width of 0?

Done.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:237: // Avoid returning true when the last step
is not docking.
On 2013/09/06 02:18:43, flackr wrote:
> This comment is confusing. The last step is the only one that can be docking
> right? So you should avoid returning true if the window shouldn't dock, but
then
> that's the name of the method so self explanatory. Is there some extra
condition
> to convey here?

More to do with how this code was modified. This is of no value anymore so
removed.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:240: size_index_ ==
static_cast<int>(usable_width_.size()) - 1);
On 2013/09/06 02:18:43, flackr wrote:
> Shouldn't 0 always be the last element in usable_width_? I'm not sure checking
> that we're at the end is necessary.

Done.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/w...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/w...
ash/wm/workspace/workspace_window_resizer.cc:394: // Keep the snap phantom on
screen when pointer gets off screen.
On 2013/09/06 02:18:43, flackr wrote:
> This comment sort of describes what we're doing when the condition is not
true.
> Can you change it to be what the condition is? Maybe something like:
> Track the last screen the pointer was on.

Done.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/w...
ash/wm/workspace/workspace_window_resizer.cc:399: bool destroyed = false;
On 2013/09/06 02:18:43, flackr wrote:
> FYI, I'm not especially fond of this pattern, I put up a CL to see about
> changing it: https://chromiumcodereview.appspot.com/23645008/
> 
> Does SetBounds have to be called before updating the snap phantom window
> necessitating this? It's not especially clear why, perhaps a comment to this
> effect.

Done.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/w...
ash/wm/workspace/workspace_window_resizer.cc:402:
LayoutAttachedWindows(&bounds);
On 2013/09/06 02:18:43, flackr wrote:
> Presumably this cannot delete the resizer since you only check destroyed after
> SetBounds? If so, can you move the destroyed_ assignment to just be around the
> SetBounds call where it's needed?

Done.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/w...
ash/wm/workspace/workspace_window_resizer.cc:415:
snap_phantom_window_controller_.reset();
On 2013/09/06 02:18:43, flackr wrote:
> Should snap_sizer_ be reset here too since we're not showing that the window
> will snap?

Done.

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/workspace/w...
ash/wm/workspace/workspace_window_resizer.cc:441: if (snap_sizer_ &&
wm::CanResizeWindow(window()) &&
On 2013/09/06 02:18:43, flackr wrote:
> Is it possible that snap_sizer_ will not be instantiated when snap_type_ is
left
> or right? Maybe a DCHECK instead?

Done.

[flackr, 2013-09-09 19:06:57]

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/dock/docked...
File ash/wm/dock/docked_window_resizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/dock/docked...
ash/wm/dock/docked_window_resizer.cc:264: attached_panel =
On 2013/09/09 15:38:42, varkha wrote:
> On 2013/09/06 02:18:43, flackr wrote:
> > Won't moving this check here make it possible to dock attached panels by
> > resizing them?
> 
> No. is_docked_ will be false unless the window was docked before. So resizing
a
> docked window won't undock but will never dock an undocked window.

I still think it feels wrong to only have attached_panel set correctly in
certain conditions. Why is this necessary?

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/dock/docked...
ash/wm/dock/docked_window_resizer.cc:299: if (was_docked_ &&
On 2013/09/09 15:38:42, varkha wrote:
> On 2013/09/06 02:18:43, flackr wrote:
> > was_docked_ is true iff the window was docked before the drag. Shouldn't we
> > retain restore bounds if the window was not initially docked?
> 
> That part is already handled by existing code in
> WorkspaceWindowResizer::CompleteDrag that saves the restore bounds when
snapping
> (and now docking). What I am doing here is restoring the restore bounds after
> they have been cleared by DefaultWindowResizer::Drag. I do this only in case
> then a docked window is dragged but not resized inside the dock.

I think it would make more sense to add an exception or check in
DefaultWindowResizer::Drag (that the window is in the default / workspace
container) rather than undo what it does here.

https://chromiumcodereview.appspot.com/23431009/diff/61002/ash/wm/workspace/w...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/61002/ash/wm/workspace/w...
ash/wm/workspace/workspace_window_resizer.cc:406: bool destroyed = false;
Please update this to use a weak_ptr as in
https://chromiumcodereview.appspot.com/23645008/

[varkha, 2013-09-09 20:37:54]

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/dock/docked...
File ash/wm/dock/docked_window_resizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/dock/docked...
ash/wm/dock/docked_window_resizer.cc:264: attached_panel =
On 2013/09/09 19:06:57, flackr wrote:
> On 2013/09/09 15:38:42, varkha wrote:
> > On 2013/09/06 02:18:43, flackr wrote:
> > > Won't moving this check here make it possible to dock attached panels by
> > > resizing them?
> > 
> > No. is_docked_ will be false unless the window was docked before. So
resizing
> a
> > docked window won't undock but will never dock an undocked window.
> 
> I still think it feels wrong to only have attached_panel set correctly in
> certain conditions. Why is this necessary?

See more explicit logic for preventing panel attachment on resize. Do you think
this belongs more in panel resizer?

https://chromiumcodereview.appspot.com/23431009/diff/54001/ash/wm/dock/docked...
ash/wm/dock/docked_window_resizer.cc:299: if (was_docked_ &&
On 2013/09/09 19:06:57, flackr wrote:
> On 2013/09/09 15:38:42, varkha wrote:
> > On 2013/09/06 02:18:43, flackr wrote:
> > > was_docked_ is true iff the window was docked before the drag. Shouldn't
we
> > > retain restore bounds if the window was not initially docked?
> > 
> > That part is already handled by existing code in
> > WorkspaceWindowResizer::CompleteDrag that saves the restore bounds when
> snapping
> > (and now docking). What I am doing here is restoring the restore bounds
after
> > they have been cleared by DefaultWindowResizer::Drag. I do this only in case
> > then a docked window is dragged but not resized inside the dock.
> 
> I think it would make more sense to add an exception or check in
> DefaultWindowResizer::Drag (that the window is in the default / workspace
> container) rather than undo what it does here.

I have noticed that I no longer need this since the docked windows get snapped
again when dragged even within the dock and that restores the restore bounds.

https://chromiumcodereview.appspot.com/23431009/diff/61002/ash/wm/workspace/w...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/61002/ash/wm/workspace/w...
ash/wm/workspace/workspace_window_resizer.cc:406: bool destroyed = false;
On 2013/09/09 19:06:58, flackr wrote:
> Please update this to use a weak_ptr as in
> https://chromiumcodereview.appspot.com/23645008/

Done.

[flackr, 2013-09-10 17:59:43]

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/dock/docked...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/dock/docked...
ash/wm/dock/docked_window_layout_manager.cc:546: int height =
(*iter)->bounds().height();
nit: Storing height is unnecessary for now.

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/s...
File ash/wm/workspace/snap_sizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:110: (window_->type() !=
aura::client::WINDOW_TYPE_PANEL));
This seems like it's redundant with the panel check in wm::CanSnapWindow.

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:119: dock_layout_->UndockDraggedWindow();
How does the window stay docked when dropped in the dock?

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:253: return true;
nit: This early return is a little dangerous in that if more conditions were
added it may be overlooked that this assumes it is the last check in this
function. I'd prefer to see: if (launcher && (SAME_EDGE_CONDITIONS)) return
false;

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/w...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/w...
ash/wm/workspace/workspace_window_resizer.cc:889: // Create and delete a
SnapSizer to maintain docked state.
Why do we create a SnapSizer only to delete it? This isn't clear to me.

[varkha, 2013-09-10 21:49:35]

PTAL.

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/dock/docked...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/dock/docked...
ash/wm/dock/docked_window_layout_manager.cc:546: int height =
(*iter)->bounds().height();
On 2013/09/10 17:59:44, flackr wrote:
> nit: Storing height is unnecessary for now.

Done.

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/s...
File ash/wm/workspace/snap_sizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:110: (window_->type() !=
aura::client::WINDOW_TYPE_PANEL));
On 2013/09/10 17:59:44, flackr wrote:
> This seems like it's redundant with the panel check in wm::CanSnapWindow.

Done.

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:119: dock_layout_->UndockDraggedWindow();
On 2013/09/10 17:59:44, flackr wrote:
> How does the window stay docked when dropped in the dock?

DockedWindowResizer::CompleteDrag is called while SnapSizer still exists
(snapping guide is still shown). CompleteDrag() docks the window (reparenting if
necessary) or leaves it docked. Then resizer chain gets dismantled and when the
SnapSizer gets destroyed its dtor does nothing because there is no dragged
window and is_dragged_window_docked() returns false (those states are cleared
when drag is completed).

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:253: return true;
On 2013/09/10 17:59:44, flackr wrote:
> nit: This early return is a little dangerous in that if more conditions were
> added it may be overlooked that this assumes it is the last check in this
> function. I'd prefer to see: if (launcher && (SAME_EDGE_CONDITIONS)) return
> false;

Done.

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/w...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/w...
ash/wm/workspace/workspace_window_resizer.cc:889: // Create and delete a
SnapSizer to maintain docked state.
On 2013/09/10 17:59:44, flackr wrote:
> Why do we create a SnapSizer only to delete it? This isn't clear to me.

This is to keep the awareness about docked state restricted to
DockedWindowResizer and SnapSizer classes. I have moved the creation of a
SnapSizer when the drag is first detected, this way I hope the state is clearer
- a SnapSizer is now always created when the window is snapped but may get
destroyed when it gets unsnapped.

[flackr, 2013-09-11 18:08:49]

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/s...
File ash/wm/workspace/snap_sizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:119: dock_layout_->UndockDraggedWindow();
On 2013/09/10 21:49:35, varkha wrote:
> On 2013/09/10 17:59:44, flackr wrote:
> > How does the window stay docked when dropped in the dock?
> 
> DockedWindowResizer::CompleteDrag is called while SnapSizer still exists
> (snapping guide is still shown). CompleteDrag() docks the window (reparenting
if
> necessary) or leaves it docked. Then resizer chain gets dismantled and when
the
> SnapSizer gets destroyed its dtor does nothing because there is no dragged
> window and is_dragged_window_docked() returns false (those states are cleared
> when drag is completed).

Understood, maybe a short comment that on completing the drag CompleteDrag will
be called before this making this a no-op?

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/w...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/w...
ash/wm/workspace/workspace_window_resizer.cc:889: // Create and delete a
SnapSizer to maintain docked state.
On 2013/09/10 21:49:35, varkha wrote:
> On 2013/09/10 17:59:44, flackr wrote:
> > Why do we create a SnapSizer only to delete it? This isn't clear to me.
> 
> This is to keep the awareness about docked state restricted to
> DockedWindowResizer and SnapSizer classes. I have moved the creation of a
> SnapSizer when the drag is first detected, this way I hope the state is
clearer
> - a SnapSizer is now always created when the window is snapped but may get
> destroyed when it gets unsnapped.

Yes, I think this is clearer, thanks.

https://chromiumcodereview.appspot.com/23431009/diff/33001/ash/wm/workspace/w...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/33001/ash/wm/workspace/w...
ash/wm/workspace/workspace_window_resizer.cc:541: snap_type_ =
GetSnapType(details_.initial_location_in_parent);
Move to argument initialization.

[varkha, 2013-09-11 18:22:56]

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/s...
File ash/wm/workspace/snap_sizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/92001/ash/wm/workspace/s...
ash/wm/workspace/snap_sizer.cc:119: dock_layout_->UndockDraggedWindow();
On 2013/09/11 18:08:50, flackr wrote:
> On 2013/09/10 21:49:35, varkha wrote:
> > On 2013/09/10 17:59:44, flackr wrote:
> > > How does the window stay docked when dropped in the dock?
> > 
> > DockedWindowResizer::CompleteDrag is called while SnapSizer still exists
> > (snapping guide is still shown). CompleteDrag() docks the window
(reparenting
> if
> > necessary) or leaves it docked. Then resizer chain gets dismantled and when
> the
> > SnapSizer gets destroyed its dtor does nothing because there is no dragged
> > window and is_dragged_window_docked() returns false (those states are
cleared
> > when drag is completed).
> 
> Understood, maybe a short comment that on completing the drag CompleteDrag
will
> be called before this making this a no-op?

Done.

https://chromiumcodereview.appspot.com/23431009/diff/33001/ash/wm/workspace/w...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/33001/ash/wm/workspace/w...
ash/wm/workspace/workspace_window_resizer.cc:541: snap_type_ =
GetSnapType(details_.initial_location_in_parent);
On 2013/09/11 18:08:50, flackr wrote:
> Move to argument initialization.

Done.

[flackr, 2013-09-11 18:29:23]

Do the snap sizer changes require new tests?

Otherwise lgtm.

[flackr, 2013-09-11 18:35:56]

https://chromiumcodereview.appspot.com/23431009/diff/111001/ash/wm/dock/docke...
File ash/wm/dock/docked_window_resizer_unittest.cc (right):

https://chromiumcodereview.appspot.com/23431009/diff/111001/ash/wm/dock/docke...
ash/wm/dock/docked_window_resizer_unittest.cc:158: 5);
nit: As per the style guide
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
this only should be one arg per line if it makes it more readable. I think this
would be more readable with multiple args on each line (possibly grouping x and
y pairs onto the same line).

[varkha, 2013-09-11 18:36:12]

On 2013/09/11 18:29:23, flackr wrote:
> Do the snap sizer changes require new tests?
> 
> Otherwise lgtm.

I will definitely add more tests for the exact coordinates of the docking guide
once https://codereview.chromium.org/23471004/ is landed and snapping behavior
is stable.

[varkha, 2013-09-11 18:36:53]

+sky@, could you please take a look?

[varkha, 2013-09-11 18:45:56]

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

https://codereview.chromium.org/23431009/diff/111001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:158: 5);
On 2013/09/11 18:35:56, flackr wrote:
> nit: As per the style guide
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> this only should be one arg per line if it makes it more readable. I think
this
> would be more readable with multiple args on each line (possibly grouping x
and
> y pairs onto the same line).

Done.

[sky, 2013-09-11 19:38:40]

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

https://codereview.chromium.org/23431009/diff/121001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:30: const int
DockedWindowLayoutManager::kMinDockWidth = 200;
Style guide says should have:
// static
here.

https://codereview.chromium.org/23431009/diff/121001/ash/wm/workspace/snap_si...
File ash/wm/workspace/snap_sizer.cc (right):

https://codereview.chromium.org/23431009/diff/121001/ash/wm/workspace/snap_si...
ash/wm/workspace/snap_sizer.cc:55: std::vector<int>
BuildIdealWidthList(aura::Window* window,
allow_snap is effectively should this function do anything. Such checks should
be at the call site, not the function itself.

https://codereview.chromium.org/23431009/diff/121001/ash/wm/workspace/snap_si...
File ash/wm/workspace/snap_sizer.h (right):

https://codereview.chromium.org/23431009/diff/121001/ash/wm/workspace/snap_si...
ash/wm/workspace/snap_sizer.h:69: // Returns true when snapping sequence is at
its last (docking) step.
Why do we need snapsizer to know about docked windows? Could we keep this class
at only the snap behavior and transition to another one when we need to dock?

[varkha, 2013-09-11 20:02:10]

https://codereview.chromium.org/23431009/diff/121001/ash/wm/workspace/snap_si...
File ash/wm/workspace/snap_sizer.cc (right):

https://codereview.chromium.org/23431009/diff/121001/ash/wm/workspace/snap_si...
ash/wm/workspace/snap_sizer.cc:55: std::vector<int>
BuildIdealWidthList(aura::Window* window,
On 2013/09/11 19:38:40, sky wrote:
> allow_snap is effectively should this function do anything. Such checks should
> be at the call site, not the function itself.

What about the docking guide (allow_dock)? There are three cases here - the
returned list may contain tiling sequence, docking zero marker or both. This
will be changed with https://codereview.chromium.org/23471004/ so let me rebase
to that and see what is left of this. I would like to leave the ownership of the
tiling or docking guide in this class and transition between those as if they
were part of the same sequence. On the other hand CanDockWindow can be moved
elsewhere.

https://codereview.chromium.org/23431009/diff/121001/ash/wm/workspace/snap_si...
File ash/wm/workspace/snap_sizer.h (right):

https://codereview.chromium.org/23431009/diff/121001/ash/wm/workspace/snap_si...
ash/wm/workspace/snap_sizer.h:69: // Returns true when snapping sequence is at
its last (docking) step.
On 2013/09/11 19:38:40, sky wrote:
> Why do we need snapsizer to know about docked windows? Could we keep this
class
> at only the snap behavior and transition to another one when we need to dock?

We moved to the model where docking is the last step in tiling sequence. Most of
the logic that deals with iterating along the sequence still applies. I agree
that most of the awareness about docked layout can be weeded out (it is in
CanWindowDock which is static and can be taken out easily) but I think the
docking guide target bounds can stay in this class for both tiled and docked
cases. This simplifies the resizer logic and leaves concerns better separated in
the resizer chain.
Consider that with a mouse drag it is possible to go back and forth between
tiling and docking and this simple class is all that coordinates those.

[sky, 2013-09-11 22:18:10]

Again, docking an snapping are two different concepts. They should be isolated
in their classes.

[varkha, 2013-09-12 15:04:33]

On 2013/09/11 22:18:10, sky wrote:
> Again, docking an snapping are two different concepts. They should be isolated
> in their classes.

Understand. sky@, do you think it is OK to reuse the same
PhantomWindowController instance for snapping and docking guides? This allows
the animations to shift smoothly from tiling to docking without being reset.
This will require the WorkspaceWindowResizer to control the docked / undocked
state during the drag but the WorkspaceWindowResizer is already aware of docked
layout. I prefer this to coupling the docked and workspace resizers tighter than
today.

[sky, 2013-09-12 18:05:33]

Yes, PhantomWindowController isn't really tied to snapping. It's about
drawing possible destinations. Just update the description.

On Thu, Sep 12, 2013 at 8:04 AM,  <varkha@chromium.org> wrote:
> On 2013/09/11 22:18:10, sky wrote:
>>
>> Again, docking an snapping are two different concepts. They should be
>> isolated
>> in their classes.
>
>
> Understand. sky@, do you think it is OK to reuse the same
> PhantomWindowController instance for snapping and docking guides? This
> allows
> the animations to shift smoothly from tiling to docking without being reset.
> This will require the WorkspaceWindowResizer to control the docked /
> undocked
> state during the drag but the WorkspaceWindowResizer is already aware of
> docked
> layout. I prefer this to coupling the docked and workspace resizers tighter
> than
> today.
>
> https://codereview.chromium.org/23431009/

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

[varkha, 2013-09-12 19:49:23]

PTAL. I have reworked it so that SnapSizer is only concerned with side tiling,
but can remember if a user attempted to go past the tiling sequence (SnapSizer
does not use this in any way though - it is up to the caller to make use of this
bit of info).

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

https://codereview.chromium.org/23431009/diff/121001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:30: const int
DockedWindowLayoutManager::kMinDockWidth = 200;
On 2013/09/11 19:38:40, sky wrote:
> Style guide says should have:
> // static
> here.

Done.

https://codereview.chromium.org/23431009/diff/121001/ash/wm/workspace/snap_si...
File ash/wm/workspace/snap_sizer.cc (right):

https://codereview.chromium.org/23431009/diff/121001/ash/wm/workspace/snap_si...
ash/wm/workspace/snap_sizer.cc:55: std::vector<int>
BuildIdealWidthList(aura::Window* window,
On 2013/09/11 19:38:40, sky wrote:
> allow_snap is effectively should this function do anything. Such checks should
> be at the call site, not the function itself.

I have removed docking state updates from the SnapSizer so this change is no
longer necessary. The only thing that I am adding here is ability to query this
class if the snapping sequence has been exhausted.

https://codereview.chromium.org/23431009/diff/121001/ash/wm/workspace/snap_si...
File ash/wm/workspace/snap_sizer.h (right):

https://codereview.chromium.org/23431009/diff/121001/ash/wm/workspace/snap_si...
ash/wm/workspace/snap_sizer.h:69: // Returns true when snapping sequence is at
its last (docking) step.
On 2013/09/11 19:38:40, sky wrote:
> Why do we need snapsizer to know about docked windows? Could we keep this
class
> at only the snap behavior and transition to another one when we need to dock?

Done.

[sky, 2013-09-13 16:25:31]

Thanks, LGTM

[commit-bot: I haz the power, 2013-09-13 16:54:55]

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

[commit-bot: I haz the power, 2013-09-14 02:57:45]

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

[commit-bot: I haz the power, 2013-09-14 03:03:39]

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=android_cl...

[commit-bot: I haz the power, 2013-09-14 03:05:43]

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

[commit-bot: I haz the power, 2013-09-14 11:49:30]

Change committed as 223237