Change behaviour of the Alt-] and Alt-[ keys so that it cycles through SnapLeft/SnapRight to DockLe…

Change behaviour of the Alt-] and Alt-[ keys so that it cycles through SnapLeft/SnapRight to DockLeft/DockRight to Restore.

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

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

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

[varkha, 2014-09-24 03:16:23]

I think it is quite close, just need to make sure nothing is lost in transition
about all the conditions that could previously prevent a window from getting
side-snapped.
A unit test would be good, maybe using a window type as a test parameter so that
the same test code is used for panels and regular windows.

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
ash/accelerators/accelerator_controller.cc:517: return 0;
s/0/NULL

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
ash/accelerators/accelerator_controller.cc:535: void RestoreWindow(bool left) {
Do we need to know the side here? If it is only for UMA we can ignore it I
think.
Also using an enum instead of bool would be more self descriptive at a call
site.

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
ash/accelerators/accelerator_controller.cc:558: void SnapWindow(bool left) {
I think using WM_EVENT_SNAP_xxx instead of bool would be more self descriptive
at a call site.

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
ash/accelerators/accelerator_controller.cc:572: if (dock_layout) {
nit: no need for parentheses.

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
ash/accelerators/accelerator_controller.cc:580: if (dock_layout) {
nit: no need for parentheses.

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
ash/accelerators/accelerator_controller.cc:603: if (!window_state->IsDocked() ||
Do we still need to check CanSnap() somewhere?

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
ash/accelerators/accelerator_controller.cc:605: currentDockAlignment !=
desiredDockAlignment)) {
The logic could be just a bit simpler if the condition is revered and restore
action is taken first with an early return. The next two conditions then do not
need additional scope.

https://codereview.chromium.org/594383002/diff/1/ash/wm/panels/panel_layout_m...
File ash/wm/panels/panel_layout_manager.cc (right):

https://codereview.chromium.org/594383002/diff/1/ash/wm/panels/panel_layout_m...
ash/wm/panels/panel_layout_manager.cc:487: void
PanelLayoutManager::OnPreWindowStateTypeChange(
Would be nice to move this into some place that is more dock-aware.

https://codereview.chromium.org/594383002/diff/1/ash/wm/workspace/workspace_l...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/594383002/diff/1/ash/wm/workspace/workspace_l...
ash/wm/workspace/workspace_layout_manager.cc:264: void
WorkspaceLayoutManager::OnPreWindowStateTypeChange(
Can this code be placed in some place that is docking-specific or maybe in
window state classes that are more aware? We were trying to limit how much the
rest of the layout managers know about docking. Same goes for panels.

[dtapuska, 2014-09-25 19:52:33]

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

[dtapuska, 2014-09-25 23:27:09]

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
ash/accelerators/accelerator_controller.cc:517: return 0;
On 2014/09/24 03:16:22, varkha wrote:
> s/0/NULL

Done.

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
ash/accelerators/accelerator_controller.cc:535: void RestoreWindow(bool left) {
On 2014/09/24 03:16:23, varkha wrote:
> Do we need to know the side here? If it is only for UMA we can ignore it I
> think.
> Also using an enum instead of bool would be more self descriptive at a call
> site.

Done.

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
ash/accelerators/accelerator_controller.cc:558: void SnapWindow(bool left) {
On 2014/09/24 03:16:23, varkha wrote:
> I think using WM_EVENT_SNAP_xxx instead of bool would be more self descriptive
> at a call site.

Done.

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
ash/accelerators/accelerator_controller.cc:572: if (dock_layout) {
On 2014/09/24 03:16:23, varkha wrote:
> nit: no need for parentheses.

Done.

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
ash/accelerators/accelerator_controller.cc:580: if (dock_layout) {
On 2014/09/24 03:16:22, varkha wrote:
> nit: no need for parentheses.

Done.

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
ash/accelerators/accelerator_controller.cc:603: if (!window_state->IsDocked() ||
On 2014/09/24 03:16:23, varkha wrote:
> Do we still need to check CanSnap() somewhere?

Done.

https://codereview.chromium.org/594383002/diff/1/ash/accelerators/accelerator...
ash/accelerators/accelerator_controller.cc:605: currentDockAlignment !=
desiredDockAlignment)) {
On 2014/09/24 03:16:23, varkha wrote:
> The logic could be just a bit simpler if the condition is revered and restore
> action is taken first with an early return. The next two conditions then do
not
> need additional scope.

I don't agree. The 3rd patch now has some additional logic to bounce the window.
I think it might be complicated reversing all this logic.

https://codereview.chromium.org/594383002/diff/1/ash/wm/panels/panel_layout_m...
File ash/wm/panels/panel_layout_manager.cc (right):

https://codereview.chromium.org/594383002/diff/1/ash/wm/panels/panel_layout_m...
ash/wm/panels/panel_layout_manager.cc:487: void
PanelLayoutManager::OnPreWindowStateTypeChange(
On 2014/09/24 03:16:23, varkha wrote:
> Would be nice to move this into some place that is more dock-aware.

I collapsed the moving code; but the state management needs to be done here. We
have a panel layout and a window now has become docked so we need to re-parent
it.

https://codereview.chromium.org/594383002/diff/1/ash/wm/workspace/workspace_l...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/594383002/diff/1/ash/wm/workspace/workspace_l...
ash/wm/workspace/workspace_layout_manager.cc:264: void
WorkspaceLayoutManager::OnPreWindowStateTypeChange(
On 2014/09/24 03:16:23, varkha wrote:
> Can this code be placed in some place that is docking-specific or maybe in
> window state classes that are more aware? We were trying to limit how much the
> rest of the layout managers know about docking. Same goes for panels.

see comment in panel_layout_manager.cc

[varkha, 2014-09-29 21:04:35]

Mostly nits at this point - Chrome style is a bit picky.

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:589: bool HandleWindowSnapOrDock(int
action) {
I am not sure it this should be AcceleratorAction rather than int. I think the
only reason this is int was to allow not including
ash/accelerators/accelerator_table.h in
ash/accelerators/accelerator_controller.h but there might be some better
explanation.

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:603: DOCKED_ALIGNMENT_LEFT :
DOCKED_ALIGNMENT_RIGHT;
nit: alignment +4

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:626: } else {
nit: No need for else after return (mentioned in
http://www.chromium.org/developers/coding-style).

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller_unittest.cc:510: // check snap left,
dock left, restore cycle
Comments should be English sentences - capitalized and ending with punctuation.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Punctuation,_...

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller_unittest.cc:525: // check snap right,
dock right, restore cycle
Consider separating those into separate methods. It is easier to track breakage
when one particular small test breaks rather than one of several cases in a
single test.

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
ash/accelerators/accelerator_table.cc:426: WINDOW_SNAP_OR_DOCK_LEFT,
Commenting out the next few accelerators would allow them to be handled even
when there is no window in workspace such as when all the windows are docked. I
mention this even though this may not be the right thing to do (I suspect that
the right thing would be to allow BuildWindowListInternal and
MruWindowTracker::BuildWindowList do the right thing for docked windows but this
may get you into figuring out what to do about the F5 overview mode).

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

https://codereview.chromium.org/594383002/diff/80001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:601: preferred_alignment_ =
preferred_alignment;
This is simple enough to qualify for a header inline. If so the name could be
set_preferred_alignment and as an added bonus you can skip the comment in the
header since it becomes trivial.

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

https://codereview.chromium.org/594383002/diff/80001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.h:121: // Set the preferred alignment
of the dock
s/Set/Sets.
nit: period at the end of the sentence.

https://codereview.chromium.org/594383002/diff/80001/ash/wm/panels/panel_layo...
File ash/wm/panels/panel_layout_manager.cc (right):

https://codereview.chromium.org/594383002/diff/80001/ash/wm/panels/panel_layo...
ash/wm/panels/panel_layout_manager.cc:490: aura::Window* window =
window_state->window();
Since you have this you could s/'window_state->window()'/'window' below.

https://codereview.chromium.org/594383002/diff/80001/ash/wm/panels/panel_layo...
ash/wm/panels/panel_layout_manager.cc:492: wm::DockWindow(window);
Just a question - is this what allows the accelerator to work on panels or do
you need it for some other reason?

https://codereview.chromium.org/594383002/diff/80001/ash/wm/window_util.h
File ash/wm/window_util.h (right):

https://codereview.chromium.org/594383002/diff/80001/ash/wm/window_util.h#new...
ash/wm/window_util.h:111: // Dock the given window; reparenting it to the docked
container
s/Dock/Docks
s/given window/|window|
nit: period at the end of a sentence.

https://codereview.chromium.org/594383002/diff/80001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/594383002/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:268: if (window_state->IsDocked())
{
nit: no need for parens with one-liner.

https://codereview.chromium.org/594383002/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:269: wm::DockWindow(window);
Just inline window_state->window() here (it gets inlined anyway).

https://codereview.chromium.org/594383002/diff/100001/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

https://codereview.chromium.org/594383002/diff/100001/ash/wm/window_util.cc#n...
ash/wm/window_util.cc:210: void DockWindow(aura::Window* window) {
This could be used in a few places now where we call docked_container =
GetContainer(kShellWindowId_DockedContainer) and then
ReparentChildWithTransientChildren(docked_container).

[dtapuska, 2014-09-30 14:40:50]

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:589: bool HandleWindowSnapOrDock(int
action) {
On 2014/09/29 21:04:34, varkha wrote:
> I am not sure it this should be AcceleratorAction rather than int. I think the
> only reason this is int was to allow not including
> ash/accelerators/accelerator_table.h in
> ash/accelerators/accelerator_controller.h but there might be some better
> explanation.
It appears they wanted to speed up compilation by avoiding needless includes.
This can be addressed once the forward declaration of enums is approved in the
C++ style guide. (http://http://chromium-cpp.appspot.com/) But I'd prefer not
monkeying with it for now as that is the way it was before. PerformAction takes
an int as opposed to an enum.

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:603: DOCKED_ALIGNMENT_LEFT :
DOCKED_ALIGNMENT_RIGHT;
On 2014/09/29 21:04:34, varkha wrote:
> nit: alignment +4

Done.

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:626: } else {
On 2014/09/29 21:04:34, varkha wrote:
> nit: No need for else after return (mentioned in
> http://www.chromium.org/developers/coding-style).

Done.

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller_unittest.cc:525: // check snap right,
dock right, restore cycle
On 2014/09/29 21:04:34, varkha wrote:
> Consider separating those into separate methods. It is easier to track
breakage
> when one particular small test breaks rather than one of several cases in a
> single test.

Done.

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
ash/accelerators/accelerator_table.cc:426: WINDOW_SNAP_OR_DOCK_LEFT,
On 2014/09/29 21:04:34, varkha wrote:
> Commenting out the next few accelerators would allow them to be handled even
> when there is no window in workspace such as when all the windows are docked.
I
> mention this even though this may not be the right thing to do (I suspect that
> the right thing would be to allow BuildWindowListInternal and
> MruWindowTracker::BuildWindowList do the right thing for docked windows but
this
> may get you into figuring out what to do about the F5 overview mode).

Acknowledged.

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

https://codereview.chromium.org/594383002/diff/80001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:601: preferred_alignment_ =
preferred_alignment;
On 2014/09/29 21:04:34, varkha wrote:
> This is simple enough to qualify for a header inline. If so the name could be
> set_preferred_alignment and as an added bonus you can skip the comment in the
> header since it becomes trivial.

Done.

https://codereview.chromium.org/594383002/diff/80001/ash/wm/panels/panel_layo...
File ash/wm/panels/panel_layout_manager.cc (right):

https://codereview.chromium.org/594383002/diff/80001/ash/wm/panels/panel_layo...
ash/wm/panels/panel_layout_manager.cc:490: aura::Window* window =
window_state->window();
On 2014/09/29 21:04:34, varkha wrote:
> Since you have this you could s/'window_state->window()'/'window' below.

Done.

https://codereview.chromium.org/594383002/diff/80001/ash/wm/panels/panel_layo...
ash/wm/panels/panel_layout_manager.cc:492: wm::DockWindow(window);
On 2014/09/29 21:04:34, varkha wrote:
> Just a question - is this what allows the accelerator to work on panels or do
> you need it for some other reason?
Yes; a panel is in the panel layout manager; and it changes to Docked state;
this removes it from the panel layout manager and adds it to the
DockedLayoutManager (it does the reparenting; the wm::DockWindow(...)

https://codereview.chromium.org/594383002/diff/80001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/594383002/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:268: if (window_state->IsDocked())
{
On 2014/09/29 21:04:34, varkha wrote:
> nit: no need for parens with one-liner.

Done.

https://codereview.chromium.org/594383002/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:269: wm::DockWindow(window);
On 2014/09/29 21:04:34, varkha wrote:
> Just inline window_state->window() here (it gets inlined anyway).

Done.

https://codereview.chromium.org/594383002/diff/100001/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

https://codereview.chromium.org/594383002/diff/100001/ash/wm/window_util.cc#n...
ash/wm/window_util.cc:210: void DockWindow(aura::Window* window) {
On 2014/09/29 21:04:35, varkha wrote:
> This could be used in a few places now where we call docked_container =
> GetContainer(kShellWindowId_DockedContainer) and then
> ReparentChildWithTransientChildren(docked_container).

Done.

[varkha, 2014-09-30 20:31:04]

Again, nits at this point. Please see if you can rebase this on top of the other
patch to minimize the diff that gets reviewed. I think you can ask oshima to
review at this point for ash OWNERS.

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/594383002/diff/80001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:589: bool HandleWindowSnapOrDock(int
action) {
On 2014/09/30 14:40:49, Dave Tapuska wrote:
> On 2014/09/29 21:04:34, varkha wrote:
> > I am not sure it this should be AcceleratorAction rather than int. I think
the
> > only reason this is int was to allow not including
> > ash/accelerators/accelerator_table.h in
> > ash/accelerators/accelerator_controller.h but there might be some better
> > explanation.
> It appears they wanted to speed up compilation by avoiding needless includes.
> This can be addressed once the forward declaration of enums is approved in the
> C++ style guide. (http://http://chromium-cpp.appspot.com/) But I'd prefer not
> monkeying with it for now as that is the way it was before. PerformAction
takes
> an int as opposed to an enum. 

Acknowledged.

https://codereview.chromium.org/594383002/diff/120001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/594383002/diff/120001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:630:
::wm::WINDOW_ANIMATION_TYPE_BOUNCE);
nit: align with ( above.

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

https://codereview.chromium.org/594383002/diff/120001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.h:197: // alignment is returned.
This will probably get merged from the other patch. Same comment - a bit simpler
3rd person language and a name change.

https://codereview.chromium.org/594383002/diff/120001/ash/wm/window_util.h
File ash/wm/window_util.h (right):

https://codereview.chromium.org/594383002/diff/120001/ash/wm/window_util.h#ne...
ash/wm/window_util.h:111: // Docks the given |window|; reparenting it to the
docked container.
nit: just |window| would be unambiguous here.

[dtapuska, 2014-10-01 15:02:16]

https://codereview.chromium.org/594383002/diff/120001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/594383002/diff/120001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:630:
::wm::WINDOW_ANIMATION_TYPE_BOUNCE);
On 2014/09/30 20:31:03, varkha wrote:
> nit: align with ( above.

Done.

https://codereview.chromium.org/594383002/diff/120001/ash/wm/window_util.h
File ash/wm/window_util.h (right):

https://codereview.chromium.org/594383002/diff/120001/ash/wm/window_util.h#ne...
ash/wm/window_util.h:111: // Docks the given |window|; reparenting it to the
docked container.
On 2014/09/30 20:31:03, varkha wrote:
> nit: just |window| would be unambiguous here.

Done.

[varkha, 2014-10-01 19:08:51]

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

https://codereview.chromium.org/594383002/diff/140001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.h:306: // The preferred alignment of
the next window to be added.
nit: s/added/docked or s/added/'added to docked layout'.

https://codereview.chromium.org/594383002/diff/140001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_resizer.cc (right):

https://codereview.chromium.org/594383002/diff/140001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_resizer.cc:276: dock_container);
Can use DockWindow and maybe check for parent()->id ==
kShellWindowId_DockedContainer above if you want to avoid calling
Shell::GetContainer.

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

https://codereview.chromium.org/594383002/diff/140001/ash/wm/panels/panel_lay...
ash/wm/panels/panel_layout_manager.cc:492: wm::DockWindow(window);
nit: Early return here and eliminate else. Added bonus - the diff will be
smaller and easier to merge.

https://codereview.chromium.org/594383002/diff/140001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/594383002/diff/140001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:268:
wm::DockWindow(window_state->window());
I assume this can never get called when the window is already parented to docked
container, right?

https://codereview.chromium.org/594383002/diff/140001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/594383002/diff/140001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:688: <action name="Accel_Window_Restore">
I think the intent here was to record use of actual accelerators rather than
actions taken by those accelerators. If this is indeed the intent then I think
this new metric should be reverted and we should just record snap_left or
snap_right according to what key was actually pressed.

[dtapuska, 2014-10-01 20:17:42]

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

https://codereview.chromium.org/594383002/diff/140001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.h:306: // The preferred alignment of
the next window to be added.
On 2014/10/01 19:08:51, varkha wrote:
> nit: s/added/docked or s/added/'added to docked layout'.

Done.

https://codereview.chromium.org/594383002/diff/140001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_resizer.cc (right):

https://codereview.chromium.org/594383002/diff/140001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_resizer.cc:276: dock_container);
On 2014/10/01 19:08:51, varkha wrote:
> Can use DockWindow and maybe check for parent()->id ==
> kShellWindowId_DockedContainer above if you want to avoid calling
> Shell::GetContainer.

Done.

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

https://codereview.chromium.org/594383002/diff/140001/ash/wm/panels/panel_lay...
ash/wm/panels/panel_layout_manager.cc:492: wm::DockWindow(window);
On 2014/10/01 19:08:51, varkha wrote:
> nit: Early return here and eliminate else. Added bonus - the diff will be
> smaller and easier to merge.

Done.

https://codereview.chromium.org/594383002/diff/140001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/594383002/diff/140001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:268:
wm::DockWindow(window_state->window());
On 2014/10/01 19:08:51, varkha wrote:
> I assume this can never get called when the window is already parented to
docked
> container, right?

No I don't believe so. The listeners are removed when when the child is removed
from this layout.

https://codereview.chromium.org/594383002/diff/140001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/594383002/diff/140001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:688: <action name="Accel_Window_Restore">
On 2014/10/01 19:08:51, varkha wrote:
> I think the intent here was to record use of actual accelerators rather than
> actions taken by those accelerators. If this is indeed the intent then I think
> this new metric should be reverted and we should just record snap_left or
> snap_right according to what key was actually pressed.

Done.

[varkha, 2014-10-01 20:37:13]

LGTM for ash/wm and general concept. This still needs
https://codereview.chromium.org/597683003/ to land first.

[oshima, 2014-10-01 23:39:24]

https://codereview.chromium.org/594383002/diff/160001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/594383002/diff/160001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:507: DockedWindowLayoutManager*
GetDockedWindowLayoutManager()
could you please define
WM_EVENT_TOGGLE_SNAP_DOCK and move these code into default_state.cc ?

Please refer to WM_EVENT_GOGGLE_VERTICAL_MAXIMIZE as an example.

https://codereview.chromium.org/594383002/diff/160001/ash/accelerators/accele...
File ash/accelerators/accelerator_table.h (right):

https://codereview.chromium.org/594383002/diff/160001/ash/accelerators/accele...
ash/accelerators/accelerator_table.h:134: WINDOW_SNAP_OR_DOCK_RIGHT,
WINDOW_CYCLE_SNAP_DOCK_LEFT/RIGHT ?

https://codereview.chromium.org/594383002/diff/160001/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

https://codereview.chromium.org/594383002/diff/160001/ash/wm/window_util.cc#n...
ash/wm/window_util.cc:216: docked_container);
can we move this code into DefaultState::UpdateBoundsFromState in
default_state.cc ?

Having this where in window_util.h make this look like an API and can be
misleading.

[dtapuska, 2014-10-02 15:06:11]

https://codereview.chromium.org/594383002/diff/160001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/594383002/diff/160001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:507: DockedWindowLayoutManager*
GetDockedWindowLayoutManager()
On 2014/10/01 23:39:24, oshima wrote:
> could you please define
> WM_EVENT_TOGGLE_SNAP_DOCK and move these code into default_state.cc ?
> 
> Please refer to WM_EVENT_GOGGLE_VERTICAL_MAXIMIZE as an example.

I'll need to add a WM_EVENT_TOGGLE_SNAP_DOCK_LEFT and
WM_EVENT_TOGGLE_SNAP_DOCK_RIGHT events.

As if the user is docked left; a CYCLE_SNAP_DOCK_RIGHT will either snap it right
or dock it right (depending on if it can be snapped or not). 

Make sense? (I'll start changing the code for this case)

[oshima, 2014-10-02 17:39:43]

On 2014/10/02 15:06:11, Dave Tapuska wrote:
>
https://codereview.chromium.org/594383002/diff/160001/ash/accelerators/accele...
> File ash/accelerators/accelerator_controller.cc (right):
> 
>
https://codereview.chromium.org/594383002/diff/160001/ash/accelerators/accele...
> ash/accelerators/accelerator_controller.cc:507: DockedWindowLayoutManager*
> GetDockedWindowLayoutManager()
> On 2014/10/01 23:39:24, oshima wrote:
> > could you please define
> > WM_EVENT_TOGGLE_SNAP_DOCK and move these code into default_state.cc ?
> > 
> > Please refer to WM_EVENT_GOGGLE_VERTICAL_MAXIMIZE as an example.
> 
> I'll need to add a WM_EVENT_TOGGLE_SNAP_DOCK_LEFT and
> WM_EVENT_TOGGLE_SNAP_DOCK_RIGHT events.

_LEFT, _RIGHT sgtm. Alternatively, you can define a new event class (like
SetBounds) with left/right
parameter. I don't have strong opinion, so I'll leave it to you.

> As if the user is docked left; a CYCLE_SNAP_DOCK_RIGHT will either snap it
right
> or dock it right (depending on if it can be snapped or not). 
> 
> Make sense? (I'll start changing the code for this case)

My understanding, from the bug and the description, is that this accelerator is
for cycyling
snap -> dock -> normal, but it may skip certain state if it's not possible.
If that's the case, I'd still call it cycle. CYCLE_FORWARD_MRU toggles the focus
if
there are only two windows, for example.

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

https://codereview.chromium.org/594383002/diff/160001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/594383002/diff/160001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:507: DockedWindowLayoutManager*
GetDockedWindowLayoutManager()
On 2014/10/01 23:39:24, oshima wrote:
> could you please define
> WM_EVENT_TOGGLE_SNAP_DOCK and move these code into default_state.cc ?
> 
> Please refer to WM_EVENT_GOGGLE_VERTICAL_MAXIMIZE as an example.

Done.

https://codereview.chromium.org/594383002/diff/160001/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

https://codereview.chromium.org/594383002/diff/160001/ash/wm/window_util.cc#n...
ash/wm/window_util.cc:216: docked_container);
On 2014/10/01 23:39:24, oshima wrote:
> can we move this code into DefaultState::UpdateBoundsFromState in
> default_state.cc ?
> 
> Having this where in window_util.h make this look like an API and can be
> misleading.

Done. (In other dependent change)

[oshima, 2014-10-02 21:58:42]

lgtm with nits

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

https://codereview.chromium.org/594383002/diff/180001/ash/wm/default_state.cc...
ash/wm/default_state.cc:69: {
nit: move { to previous line  (see style guide)

https://codereview.chromium.org/594383002/diff/180001/ash/wm/default_state.cc...
ash/wm/default_state.cc:95:
DISALLOW_COPY_AND_ASSIGN(ScopedPreferredAlignmentResetter);
nit: move DISALLOW... to the last, with one new line between this and
"docked_window_layout_manager_"

https://codereview.chromium.org/594383002/diff/180001/ash/wm/default_state.cc...
ash/wm/default_state.cc:134: const wm::WMEvent event(wm::WM_EVENT_DOCK);
This is ok for now. Alternative way is to pass the parameter using event class.

https://codereview.chromium.org/594383002/diff/180001/ash/wm/wm_event.h
File ash/wm/wm_event.h (right):

https://codereview.chromium.org/594383002/diff/180001/ash/wm/wm_event.h#newco...
ash/wm/wm_event.h:58: WM_EVENT_TOGGLE_SNAP_DOCK_LEFT,
nit: CYCLE_

[varkha, 2014-10-02 22:17:59]

No real implementation issues left but UMA needs to be recorded for all docked
transitions.

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

https://codereview.chromium.org/594383002/diff/180001/ash/wm/default_state.cc...
ash/wm/default_state.cc:119: window_state->OnWMEvent(&event);
May need to RecordUmaAction DOCKED_ACTION_MAXIMIZE (this is what we record when
transitioning from docked to vertically maximized aka "snapped" state). Also can
you please add a new DockedActionSource for KEYBOARD.

https://codereview.chromium.org/594383002/diff/180001/ash/wm/default_state.cc...
ash/wm/default_state.cc:135: window_state->OnWMEvent(&event);
Need to RecordUmaAction DOCKED_ACTION_DOCK.

https://codereview.chromium.org/594383002/diff/180001/ash/wm/default_state.cc...
ash/wm/default_state.cc:141: window_state->Restore();
May need to RecordUmaAction DOCKED_ACTION_UNDOCK.

https://codereview.chromium.org/594383002/diff/180001/ash/wm/wm_event.h
File ash/wm/wm_event.h (right):

https://codereview.chromium.org/594383002/diff/180001/ash/wm/wm_event.h#newco...
ash/wm/wm_event.h:57: // A user requested a toggle of dock and snap left
I think this would be a good place to describe the cycle through restored ->
snapped -> docked -> restored ... states and mention that some states may be
skipped in this cycle.

[dtapuska, 2014-10-03 15:44:59]

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

https://codereview.chromium.org/594383002/diff/180001/ash/wm/default_state.cc...
ash/wm/default_state.cc:69: {
On 2014/10/02 21:58:41, oshima wrote:
> nit: move { to previous line  (see style guide)

Done.

https://codereview.chromium.org/594383002/diff/180001/ash/wm/default_state.cc...
ash/wm/default_state.cc:95:
DISALLOW_COPY_AND_ASSIGN(ScopedPreferredAlignmentResetter);
On 2014/10/02 21:58:41, oshima wrote:
> nit: move DISALLOW... to the last, with one new line between this and
> "docked_window_layout_manager_"

Done.

https://codereview.chromium.org/594383002/diff/180001/ash/wm/default_state.cc...
ash/wm/default_state.cc:119: window_state->OnWMEvent(&event);
On 2014/10/02 22:17:59, varkha wrote:
> May need to RecordUmaAction DOCKED_ACTION_MAXIMIZE (this is what we record
when
> transitioning from docked to vertically maximized aka "snapped" state). Also
can
> you please add a new DockedActionSource for KEYBOARD.

Done.

https://codereview.chromium.org/594383002/diff/180001/ash/wm/default_state.cc...
ash/wm/default_state.cc:134: const wm::WMEvent event(wm::WM_EVENT_DOCK);
On 2014/10/02 21:58:41, oshima wrote:
> This is ok for now. Alternative way is to pass the parameter using event
class. 

Acknowledged.

https://codereview.chromium.org/594383002/diff/180001/ash/wm/default_state.cc...
ash/wm/default_state.cc:135: window_state->OnWMEvent(&event);
On 2014/10/02 22:17:59, varkha wrote:
> Need to RecordUmaAction DOCKED_ACTION_DOCK.

Applied in the docked_window_layout_manager when a child is added

https://codereview.chromium.org/594383002/diff/180001/ash/wm/default_state.cc...
ash/wm/default_state.cc:141: window_state->Restore();
On 2014/10/02 22:17:59, varkha wrote:
> May need to RecordUmaAction DOCKED_ACTION_UNDOCK.

Applied in the docked_window_layout_manager.cc

https://codereview.chromium.org/594383002/diff/180001/ash/wm/wm_event.h
File ash/wm/wm_event.h (right):

https://codereview.chromium.org/594383002/diff/180001/ash/wm/wm_event.h#newco...
ash/wm/wm_event.h:57: // A user requested a toggle of dock and snap left
On 2014/10/02 22:17:59, varkha wrote:
> I think this would be a good place to describe the cycle through restored ->
> snapped -> docked -> restored ... states and mention that some states may be
> skipped in this cycle.

Done.

https://codereview.chromium.org/594383002/diff/180001/ash/wm/wm_event.h#newco...
ash/wm/wm_event.h:58: WM_EVENT_TOGGLE_SNAP_DOCK_LEFT,
On 2014/10/02 21:58:41, oshima wrote:
> nit: CYCLE_

Done.

[varkha, 2014-10-03 17:07:26]

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

https://codereview.chromium.org/594383002/diff/200001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:707: if (event_source_ !=
DOCKED_ACTION_SOURCE_UNKNOWN)
I think it is better to always record the UMA action here and change all other
places where we send DOCKED_ACTION_DOCK to send _NONE.
Otherwise we sometimes use it in the caller and sometimes here which may be
confusing.

[varkha, 2014-10-03 17:13:50]

slgtm

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

https://codereview.chromium.org/594383002/diff/200001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:707: if (event_source_ !=
DOCKED_ACTION_SOURCE_UNKNOWN)
On 2014/10/03 17:07:26, varkha wrote:
> I think it is better to always record the UMA action here and change all other
> places where we send DOCKED_ACTION_DOCK to send _NONE.
> Otherwise we sometimes use it in the caller and sometimes here which may be
> confusing.

As we talked offline this won't be possible (the dragged windows get added
earlier and would not get counted properly. I think this is OK for now if you
add a short comment why DOCKED_ACTION_SOURCE_UNKNOWN is treated this way.

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

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

https://codereview.chromium.org/594383002/diff/200001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:707: if (event_source_ !=
DOCKED_ACTION_SOURCE_UNKNOWN)
On 2014/10/03 17:13:50, varkha wrote:
> On 2014/10/03 17:07:26, varkha wrote:
> > I think it is better to always record the UMA action here and change all
other
> > places where we send DOCKED_ACTION_DOCK to send _NONE.
> > Otherwise we sometimes use it in the caller and sometimes here which may be
> > confusing.
> 
> As we talked offline this won't be possible (the dragged windows get added
> earlier and would not get counted properly. I think this is OK for now if you
> add a short comment why DOCKED_ACTION_SOURCE_UNKNOWN is treated this way.

Done.

[varkha, 2014-10-03 17:24:36]

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

https://codereview.chromium.org/594383002/diff/220001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:710: // or docked windows.
Maybe shorten it a bit to "Only keyboard-initiated actions are recorded here.
Dragging cases are handled in
DockedWindowResizer::MaybeReparentWindowOnDragCompletion."

[dtapuska, 2014-10-03 17:32:50]

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

https://codereview.chromium.org/594383002/diff/220001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:710: // or docked windows.
On 2014/10/03 17:24:36, varkha wrote:
> Maybe shorten it a bit to "Only keyboard-initiated actions are recorded here.
> Dragging cases are handled in
> DockedWindowResizer::MaybeReparentWindowOnDragCompletion."

Done.

[varkha, 2014-10-03 17:42:12]

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

https://codereview.chromium.org/594383002/diff/260001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:708: // are handled in
FinishDragging
nit: period at the end of a sentence.

[dtapuska, 2014-10-03 19:06:00]

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

https://codereview.chromium.org/594383002/diff/260001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:708: // are handled in
FinishDragging
On 2014/10/03 17:42:12, varkha wrote:
> nit: period at the end of a sentence.

Done.

[dtapuska, 2014-10-03 20:04:07]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2014-10-03 20:05:20]

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

[commit-bot: I haz the power, 2014-10-03 20:10:17]

Committed patchset #15 (id:280001) as 6d5ad2079951de8843f39c61fe47d194377926c6

[commit-bot: I haz the power, 2014-10-03 20:10:56]

Patchset 15 (id:??) landed as
https://crrev.com/c6e5e38c6843add6e7aa5d427958164d0550f20b
Cr-Commit-Position: refs/heads/master@{#298084}