Ignores programmatic bounds changes for docked windows

Ignores programmatic bounds changes for docked windows. This prevents a window that is using resizeTo or similar API from changing window bounds while it is managed by DockedLayoutManager.

BUG=496852
TEST=DockedWindowLayoutManagerTest.DockedWindowBoundsDontChange

Committed: https://crrev.com/352e317da0aeda5c4e1e21a81d4480536e77b81e
Cr-Commit-Position: refs/heads/master@{#333226}

[varkha, 2015-06-04 19:55:26]

Patchset #1 (id:1) has been deleted

[varkha, 2015-06-04 20:30:44]

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

[varkha, 2015-06-04 20:50:53]

oshima@, PTAL. I think this small change is a better overall behavior even
though it may ignore the legitimate small window size changes. I think it is
easier to understand since the size always reverts to what it was before a
window got docked.

[varkha, 2015-06-04 20:54:27]

Sorry, wrong comment. This CL ignores programmatic change from the apps which
may be processed asynchronously and result in a window ending outside of the
dock while it is logically docked (while it is parented in the dock).
There was a suggestion (not sure who suggested, could be even myself) to defer
the programmatic changes until a window gets undocked but I think this behavior
is simpler and easier to understand.

[varkha, 2015-06-04 20:56:57]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-04 20:57:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149153007/20001

[oshima, 2015-06-04 22:40:29]

Ideally, these logic should move to WindowState/DefaultState.

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

https://codereview.chromium.org/1149153007/diff/40001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:213: // Certain windows (minimized,
hidden or popups) do not matter to docking.
The comment and the function name aren't clear. Can you rename to something like
IsWindowDockable and update the comment?

https://codereview.chromium.org/1149153007/diff/40001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:748: if (IsUsedByLayout(child) &&
child != dragged_window_)
doesn't this prevent relayouting docked windows upon display change?

[varkha, 2015-06-04 23:22:01]

Did you mean the logic in DockedWindowLayoutManager::SetChildBounds? I think
what I am doing is precisely aligned with the mandate in layout_manager.h -
"LayoutManager may modify |requested_bounds| before applying, or ignore the
request." This (ignoring the request in some docking-specific cases) is exactly
what I am doing here. Why do you think this belongs in default state?

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

https://codereview.chromium.org/1149153007/diff/40001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:213: // Certain windows (minimized,
hidden or popups) do not matter to docking.
On 2015/06/04 22:40:28, oshima wrote:
> The comment and the function name aren't clear. Can you rename to something
like
> IsWindowDockable and update the comment?

Done. This is mostly considered in this class for the windows that are already
docked so Dockable didn't seem right. See if you like this better.

https://codereview.chromium.org/1149153007/diff/40001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:748: if (IsUsedByLayout(child) &&
child != dragged_window_)
On 2015/06/04 22:40:29, oshima wrote:
> doesn't this prevent relayouting docked windows upon display change?

No. Display change is handled in OnWindowResized() when the parent (docked
container) gets resized. This handler is when docked children are resized
programmatically.

[oshima, 2015-06-05 23:08:36]

> Did you mean the logic in DockedWindowLayoutManager::SetChildBounds? I think
> what I am doing is precisely aligned with the mandate in layout_manager.h -
> "LayoutManager may modify |requested_bounds| before applying, or ignore the
> request." This (ignoring the request in some docking-specific cases) is
exactly
> what I am doing here. Why do you think this belongs in default state?

LayoutManager is best suited for passive window where the window's positioning
is always determined by the state of desktop. App/Browser windows has its own
state, and can change its own state. These type of window, WindowState is
the better place to put logic because WindowState make decision based on its
state.

We used to use WorkspaceLayoutManager to control window states such as
maximized, minimized
snap left right and it had a lot of issues. I consolidated into these logic into
state based in WindowState/DefaultState.

I'm just talking about my hope so you don't have to change this at all.

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

https://codereview.chromium.org/1149153007/diff/60001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:214: bool IsIgnoredForDocking(const
aura::Window* window) {
This is still confusing to me.

If this is to indicate that the window is docked, why not IsDocked?


Here is my interpretation is:

// Certain windows (minimized, hidden or popups) are ignored by dock layout
logic.
  |
  V
"minimized, hidden or popup windows aren't docked" ->

NotDocked() : minimized || !visible || popup
(IsIgnoredByDockLayout describes impl)
   |
  flip the condition
   V
IsDocked : visible && !minimized && !popup
IsUsedByDockLayout

[varkha, 2015-06-07 02:53:24]

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

https://codereview.chromium.org/1149153007/diff/60001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:214: bool IsIgnoredForDocking(const
aura::Window* window) {
On 2015/06/05 23:08:36, oshima wrote:
> This is still confusing to me.
> 
> If this is to indicate that the window is docked, why not IsDocked?
> 
> 
> Here is my interpretation is:
> 
> // Certain windows (minimized, hidden or popups) are ignored by dock layout
> logic.
>   |
>   V
> "minimized, hidden or popup windows aren't docked" ->
> 
> NotDocked() : minimized || !visible || popup
> (IsIgnoredByDockLayout describes impl)
>    |
>   flip the condition
>    V
> IsDocked : visible && !minimized && !popup
> IsUsedByDockLayout 
> 

Done. You are right, haven't noticed that I had reversed the name. Could have
been "IsNotIgnoredByDockLayout" but "IsWindowDocked" seems better. I tried to
avoid name clash with WindowState::IsDocked.

[oshima, 2015-06-07 04:36:53]

lgtm

thanks!

[varkha, 2015-06-07 04:38:20]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2015-06-07 04:38:23]

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

[commit-bot: I haz the power, 2015-06-07 05:31:39]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-06-07 05:32:29]

Patchset 4 (id:??) landed as
https://crrev.com/352e317da0aeda5c4e1e21a81d4480536e77b81e
Cr-Commit-Position: refs/heads/master@{#333226}