Implement automatic layout and stacking for docked windows

Implement automatic layout and stacking for docked windows.

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

[varkha, 2013-07-12 01:31:14]

Please take an early look. More unit tests pending but I wanted to start looking
at the algorithm (simpler than with panels because it is a single group). I have
made it so that the gaps (or overlaps) are all same - this I think looks better
in the dock. I have also removed a special case for two windows - it is handled
by the same multiple windows case.

[flackr, 2013-07-16 22:05:45]

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

https://codereview.chromium.org/19054013/diff/16001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:549:
visible_windows.push_back(position_info);
The window position is entirely based on the major_length which also doesn't
change during the repositioning logic. I think just a vector of Window* would be
sufficient instead of reusing this structure.

https://codereview.chromium.org/19054013/diff/16001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:565: min_major, max_major);
Since there's only one pass / one iteration, it would be nice to avoid
intermediate work and assign the bounds directly. You could inline the function
here or move the construction of the windows to fan out to the function.

https://codereview.chromium.org/19054013/diff/16001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:630: // |  |  |  |       |  |  |
nit: Should be a vertical deck of cards.

https://codereview.chromium.org/19054013/diff/16001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:651: 
nit: Only one empty line.

https://codereview.chromium.org/19054013/diff/16001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:655: it != window_ordering.end() &&
it->second != active_window; ++it) {
How about using the midpoint of the active window as the terminating condition.
Then you only need to add children of the dock to window_ordering and don't have
to check if the previous parent was the dock container.

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

https://codereview.chromium.org/19054013/diff/16001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_resizer.cc:219:
window->SetProperty(internal::kPanelAttachedKey, false);
Separate CL please.

https://codereview.chromium.org/19054013/diff/16001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/19054013/diff/16001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:7036: +      </message>
Separate CL please.

[varkha, 2013-07-18 21:56:59]

Unit tests still pending.

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

https://codereview.chromium.org/19054013/diff/16001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:549:
visible_windows.push_back(position_info);
On 2013/07/16 22:05:45, flackr wrote:
> The window position is entirely based on the major_length which also doesn't
> change during the repositioning logic. I think just a vector of Window* would
be
> sufficient instead of reusing this structure.

Done.

https://codereview.chromium.org/19054013/diff/16001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:565: min_major, max_major);
On 2013/07/16 22:05:45, flackr wrote:
> Since there's only one pass / one iteration, it would be nice to avoid
> intermediate work and assign the bounds directly. You could inline the
function
> here or move the construction of the windows to fan out to the function.

Done.

https://codereview.chromium.org/19054013/diff/16001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:630: // |  |  |  |       |  |  |
On 2013/07/16 22:05:45, flackr wrote:
> nit: Should be a vertical deck of cards.

Done.

https://codereview.chromium.org/19054013/diff/16001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:651: 
On 2013/07/16 22:05:45, flackr wrote:
> nit: Only one empty line.

Done.

https://codereview.chromium.org/19054013/diff/16001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:655: it != window_ordering.end() &&
it->second != active_window; ++it) {
On 2013/07/16 22:05:45, flackr wrote:
> How about using the midpoint of the active window as the terminating
condition.
> Then you only need to add children of the dock to window_ordering and don't
have
> to check if the previous parent was the dock container.

Done.

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

https://codereview.chromium.org/19054013/diff/16001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_resizer.cc:219:
window->SetProperty(internal::kPanelAttachedKey, false);
On 2013/07/16 22:05:45, flackr wrote:
> Separate CL please.

Done.

https://codereview.chromium.org/19054013/diff/16001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/19054013/diff/16001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:7036: +      </message>
On 2013/07/16 22:05:45, flackr wrote:
> Separate CL please.

Done.

[flackr, 2013-07-18 22:16:53]

Nice! Much cleaner and easier to read :-)

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

https://codereview.chromium.org/19054013/diff/40001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:504: y_pos)));
The clamping within the work area should never be necessary right? It might be
nice to instead just have a DCHECK for this.

https://codereview.chromium.org/19054013/diff/40001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.h (right):

https://codereview.chromium.org/19054013/diff/40001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.h:163: aura::Window*
dragged_former_child_;
Is there any reason not to use dragged_window_ for this? It seems like as long
as you allow dragged_window_ to not be a child of the dock container this should
work fine.

[varkha, 2013-07-18 22:58:14]

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

https://codereview.chromium.org/19054013/diff/40001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:504: y_pos)));
I think it may still need clamping in valid corner cases, e.g. when there are
two windows, one slim and one very tall (i.e. almost screen-tall which can
happen when a shelf was hidden and then shown again - making that window taller
than a work area). Tall window being second will start initially above the work
area and end below it. With clamping it will still end below the shelf top (work
area boundary) but at least it will be on screen and draggable.

https://codereview.chromium.org/19054013/diff/40001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.h (right):

https://codereview.chromium.org/19054013/diff/40001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.h:163: aura::Window*
dragged_former_child_;
Not anymore - nice catch. Done.

[flackr, 2013-07-18 23:18:41]

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

https://codereview.chromium.org/19054013/diff/44001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:496: std::min(work_area.bottom()-
bounds.height(),
nit: Space before minus

https://codereview.chromium.org/19054013/diff/44001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:498: y_pos += (bounds.height() +
delta);
nit: brackets unnecessary.

https://codereview.chromium.org/19054013/diff/44001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:546: active_window = last_active_;
What if the last_active_ window goes away? We should check for it being removed
from the layout to avoid a potential segmentation fault.

https://codereview.chromium.org/19054013/diff/44001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.h (right):

https://codereview.chromium.org/19054013/diff/44001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.h:180: // The last active window. Used
to maintain stacking even if no windows are
nit: stacking order

[varkha, 2013-07-19 16:52:01]

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

https://codereview.chromium.org/19054013/diff/44001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:496: std::min(work_area.bottom()-
bounds.height(),
On 2013/07/18 23:18:41, flackr wrote:
> nit: Space before minus

Done.

https://codereview.chromium.org/19054013/diff/44001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:498: y_pos += (bounds.height() +
delta);
On 2013/07/18 23:18:41, flackr wrote:
> nit: brackets unnecessary.

Done.

https://codereview.chromium.org/19054013/diff/44001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:546: active_window = last_active_;
On 2013/07/18 23:18:41, flackr wrote:
> What if the last_active_ window goes away? We should check for it being
removed
> from the layout to avoid a potential segmentation fault.

Done.

https://codereview.chromium.org/19054013/diff/44001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.h (right):

https://codereview.chromium.org/19054013/diff/44001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.h:180: // The last active window. Used
to maintain stacking even if no windows are
Done (in panels as well).

[flackr, 2013-07-19 17:53:32]

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

https://codereview.chromium.org/19054013/diff/50001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:114: void
DockedWindowLayoutManager::FinishDragging() {
I think you need to observe OnWindowDestroying or OnWindowDestroyed for if
dragged_window_ is destroyed during a drag. Since it's being dragged (and may
even be in a different parent) you could still have a pointer to a deleted
window if it was destroyed mid drag (i.e. by the app / window.close()).

[varkha, 2013-07-20 01:26:25]

Possibly more tests to be added.

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

https://codereview.chromium.org/19054013/diff/50001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:114: void
DockedWindowLayoutManager::FinishDragging() {
True (tested). Panels layout doesn't have this trouble because
OnWindowRemovedFromLayout is still called (because dragged_window_ can only be a
parent of panel container). Is there anything else that you can think of that
may be broken now that dragged_window_ is allowed to be not a child of
docked_container?

[flackr, 2013-07-22 21:19:49]

LGTM with nits.

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

https://codereview.chromium.org/19054013/diff/50001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:114: void
DockedWindowLayoutManager::FinishDragging() {
On 2013/07/20 01:26:25, varkha wrote:
> True (tested). Panels layout doesn't have this trouble because
> OnWindowRemovedFromLayout is still called (because dragged_window_ can only be
a
> parent of panel container). Is there anything else that you can think of that
> may be broken now that dragged_window_ is allowed to be not a child of
> docked_container?

Can't think of anything else, just making sure to track the window being
destroyed, or make sure FinishDragging gets called before it is (which might
already be the case).

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

https://codereview.chromium.org/19054013/diff/58001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:493: double y_pos = (available_room
> 0) ? delta : 0;
I think we use floats for DIP positions generally.

https://codereview.chromium.org/19054013/diff/58001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:506: (int)round(y_pos))));
I don't see round very often. I think (int)(y_pos + 0.5) might be more in line
with the rest of the code.

[varkha, 2013-07-23 14:33:12]

For OWNERS review. sky@, please take a look.

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

https://codereview.chromium.org/19054013/diff/58001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:493: double y_pos = (available_room
> 0) ? delta : 0;
On 2013/07/22 21:19:49, flackr wrote:
> I think we use floats for DIP positions generally.

Done.

https://codereview.chromium.org/19054013/diff/58001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:506: (int)round(y_pos))));
On 2013/07/22 21:19:49, flackr wrote:
> I don't see round very often. I think (int)(y_pos + 0.5) might be more in line
> with the rest of the code.

Done.

[sky, 2013-07-23 18:01:37]

LGTM

[commit-bot: I haz the power, 2013-07-23 20:50:51]

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

[commit-bot: I haz the power, 2013-07-24 00:12:21]

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

[commit-bot: I haz the power, 2013-07-24 01:47:58]

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

[commit-bot: I haz the power, 2013-07-24 06:21:57]

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

[commit-bot: I haz the power, 2013-07-24 10:00:57]

Change committed as 213380