WorkspaceLayoutManager backdrop should fill entire region.

WorkspaceLayoutManager backdrop should fill entire region.

Changes to the visible state of the shelf causes the WorkspaceLayoutManager to change the size of its children. This includes changing the size of the backdrop. In order for the backdrop to cover the entire workspace it needs to be notified of changes to the available area. Such as when the shelf causes the inset regions to update.

TEST=WorkspaceLayoutManagerBackdropTest.ShelfVisibilityChangesBounds
BUG=383383
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278106

[jonross, 2014-06-16 18:50:59]

Hey Rob,

A review to adjust the workspace backdrop when the shelf disappears.

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

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:7: #include <algorithm>
cpplint

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:140: if (!text_input_client)
cpplint

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:7: #include <string>
cpplint

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:44: explicit
MaximizeDelegateView(const gfx::Rect& initial_bounds)
cpplint

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1016:
WorkspaceLayoutManager* layout_manager_;
cpplint

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1023: explicit
FakeTextInputClient(gfx::NativeWindow window) : window_(window) {}
cpplint

[flackr, 2014-06-17 14:01:08]

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:950: // Set to the size of
the display to cause sizing issues with the shelf
The comment isn't clear, to cause what sizing issues? Are you intentionally
making the container overlap the shelf?

I'm also not sure I follow why you're forcing the default container bounds.
Isn't this reproducible with just shelf changes?

[jonross, 2014-06-17 14:05:59]

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:950: // Set to the size of
the display to cause sizing issues with the shelf
On 2014/06/17 14:01:08, flackr wrote:
> The comment isn't clear, to cause what sizing issues? Are you intentionally
> making the container overlap the shelf?
> 
> I'm also not sure I follow why you're forcing the default container bounds.
> Isn't this reproducible with just shelf changes?

This is reproducible with just shelf changes. However this unittest class forces
the default container to have smaller bounds (line 795.) The documentation there
claims that this is to avoid sizing issues. I did not run the other tests with a
full sized default container to see what issues the comment refers to. However
with a smaller container the bug I'm testing for no longer occurs.

[flackr, 2014-06-17 14:13:25]

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:950: // Set to the size of
the display to cause sizing issues with the shelf
On 2014/06/17 14:05:58, jonross wrote:
> On 2014/06/17 14:01:08, flackr wrote:
> > The comment isn't clear, to cause what sizing issues? Are you intentionally
> > making the container overlap the shelf?
> > 
> > I'm also not sure I follow why you're forcing the default container bounds.
> > Isn't this reproducible with just shelf changes?
> 
> This is reproducible with just shelf changes. However this unittest class
forces
> the default container to have smaller bounds (line 795.) The documentation
there
> claims that this is to avoid sizing issues. I did not run the other tests with
a
> full sized default container to see what issues the comment refers to. However
> with a smaller container the bug I'm testing for no longer occurs.

Can you check what those issues are? I'd prefer to see line 795 go and use the
default sizing.

[jonross, 2014-06-17 14:24:31]

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/337193002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:950: // Set to the size of
the display to cause sizing issues with the shelf
On 2014/06/17 14:13:25, flackr wrote:
> On 2014/06/17 14:05:58, jonross wrote:
> > On 2014/06/17 14:01:08, flackr wrote:
> > > The comment isn't clear, to cause what sizing issues? Are you
intentionally
> > > making the container overlap the shelf?
> > > 
> > > I'm also not sure I follow why you're forcing the default container
bounds.
> > > Isn't this reproducible with just shelf changes?
> > 
> > This is reproducible with just shelf changes. However this unittest class
> forces
> > the default container to have smaller bounds (line 795.) The documentation
> there
> > claims that this is to avoid sizing issues. I did not run the other tests
with
> a
> > full sized default container to see what issues the comment refers to.
However
> > with a smaller container the bug I'm testing for no longer occurs.
> 
> Can you check what those issues are? I'd prefer to see line 795 go and use the
> default sizing.

The other tests pass with line 795 gone. Whatever errors it was documenting seem
to no longer occur. Removed it and line 951.

[flackr, 2014-06-17 15:05:31]

FYI in case the auto hide animation is causing problems, have a look at
ShelfLayoutManagerTest.AutoHide. It seems to call LayoutShelf to force the
animation to complete immediately.

https://codereview.chromium.org/337193002/diff/40001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/337193002/diff/40001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:952: gfx::Rect
initial_bounds = default_container()->children()[0]->bounds();
Not really clear what the shelf state is at this point. Maybe set or expect_eq
it?

https://codereview.chromium.org/337193002/diff/40001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:961:
EXPECT_LT(reduced_bounds.height(), initial_bounds.height());
So after hiding and showing the shelf the bounds are a bit smaller? Shouldn't
the bounds grow when the shelf is hidden and return to what they were after the
shelf is shown again?

[jonross, 2014-06-17 17:21:02]

https://codereview.chromium.org/337193002/diff/40001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/337193002/diff/40001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:952: gfx::Rect
initial_bounds = default_container()->children()[0]->bounds();
On 2014/06/17 15:05:31, flackr wrote:
> Not really clear what the shelf state is at this point. Maybe set or expect_eq
> it?

Shelf should be visible, I've added an assert to verify, and for clarity.

https://codereview.chromium.org/337193002/diff/40001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:961:
EXPECT_LT(reduced_bounds.height(), initial_bounds.height());
On 2014/06/17 15:05:31, flackr wrote:
> So after hiding and showing the shelf the bounds are a bit smaller? Shouldn't
> the bounds grow when the shelf is hidden and return to what they were after
the
> shelf is shown again?

The backdrop delegate is created with the size of the default container. This
extends it past the size of the shelf. However the delegate does not take into
account that WorkspaceLayoutManager triggers window manager events. (See
WorkspaceLayoutManager::AdjustAllWindowsBoundsForWorkAreaChange)

This causes the backdrop delegate to shrink its size when the shelf first comes
back. Then on subsequent shelf changes nothing was causing the size to update.
(It's not a maximized widget)

[flackr, 2014-06-17 23:48:33]

lgtm with nit.

https://codereview.chromium.org/337193002/diff/40001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/337193002/diff/40001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:961:
EXPECT_LT(reduced_bounds.height(), initial_bounds.height());
On 2014/06/17 17:21:02, jonross wrote:
> On 2014/06/17 15:05:31, flackr wrote:
> > So after hiding and showing the shelf the bounds are a bit smaller?
Shouldn't
> > the bounds grow when the shelf is hidden and return to what they were after
> the
> > shelf is shown again?
> 
> The backdrop delegate is created with the size of the default container. This
> extends it past the size of the shelf. However the delegate does not take into
> account that WorkspaceLayoutManager triggers window manager events. (See
> WorkspaceLayoutManager::AdjustAllWindowsBoundsForWorkAreaChange)
> 
> This causes the backdrop delegate to shrink its size when the shelf first
comes
> back. Then on subsequent shelf changes nothing was causing the size to update.
> (It's not a maximized widget)

Does this manifest on device in any way? It sounds like a bug that it's not
created at the right size. Presumably it could be created using the workspace's
size? Can you file a bug and reference it here? Otherwise this is a really
strange expectation to see in the test.

[jonross, 2014-06-18 13:08:57]

On 2014/06/17 23:48:33, flackr wrote:
> lgtm with nit.
> 
>
https://codereview.chromium.org/337193002/diff/40001/ash/wm/workspace/workspa...
> File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/337193002/diff/40001/ash/wm/workspace/workspa...
> ash/wm/workspace/workspace_layout_manager_unittest.cc:961:
> EXPECT_LT(reduced_bounds.height(), initial_bounds.height());
> On 2014/06/17 17:21:02, jonross wrote:
> > On 2014/06/17 15:05:31, flackr wrote:
> > > So after hiding and showing the shelf the bounds are a bit smaller?
> Shouldn't
> > > the bounds grow when the shelf is hidden and return to what they were
after
> > the
> > > shelf is shown again?
> > 
> > The backdrop delegate is created with the size of the default container.
This
> > extends it past the size of the shelf. However the delegate does not take
into
> > account that WorkspaceLayoutManager triggers window manager events. (See
> > WorkspaceLayoutManager::AdjustAllWindowsBoundsForWorkAreaChange)
> > 
> > This causes the backdrop delegate to shrink its size when the shelf first
> comes
> > back. Then on subsequent shelf changes nothing was causing the size to
update.
> > (It's not a maximized widget)
> 
> Does this manifest on device in any way? It sounds like a bug that it's not
> created at the right size. Presumably it could be created using the
workspace's
> size? Can you file a bug and reference it here? Otherwise this is a really
> strange expectation to see in the test.

This was first seen on device. Enter TouchView mode with a topmost window that
cannot maximize, like the Calculator app. Swipe away the Shelf, bring the shelf
back, swipe it away again. On the second, and subsequent swipes, the backdrop
doesn't encompass the entire workspace.

The backdrop is created at the right size, it is just being altered incorrectly
by the workspace manager.

[jonross, 2014-06-18 13:09:01]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-06-18 13:11:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jonross@chromium.org/337193002/60001

[commit-bot: I haz the power, 2014-06-18 17:02:55]

Change committed as 278106