Status Trays delayed visibility

Status Trays delayed visibility

Change the shelf visibility animation so that the status area is visible before animating any properties. This way it is rendering for the full animation.

TEST=ShelfLayoutManagerTest.SetAutoHideBehavior
BUG=416618
Committed: https://crrev.com/50e36d1cd3e1d2706214b42f959563d301911cd7
Cr-Commit-Position: refs/heads/master@{#296307}

[jonross, 2014-09-23 18:50:20]

jonross@chromium.org changed reviewers:
+ flackr@chromium.org

[jonross, 2014-09-23 18:50:20]

Hey Rob,

Could you take a look at this review? It's for an m38 stable blocker.

[flackr, 2014-09-23 19:14:04]

Wrong bug link?

https://codereview.chromium.org/596863003/diff/1/ash/shelf/shelf_layout_manag...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/596863003/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:658: shelf_->status_area_widget()->Show();
This doesn't cause the invalid window state (Visible window with 0 opacity)
updating visibility before we've set target opacity?

If you call show after setting up the opacity animation, but outside of the
scope of the ScopedLayerAnimationSettings it should not have the invalid state.

[jonross, 2014-09-23 20:50:06]

https://codereview.chromium.org/596863003/diff/1/ash/shelf/shelf_layout_manag...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/596863003/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:658: shelf_->status_area_widget()->Show();
On 2014/09/23 19:14:03, flackr wrote:
> This doesn't cause the invalid window state (Visible window with 0 opacity)
> updating visibility before we've set target opacity?
> 
> If you call show after setting up the opacity animation, but outside of the
> scope of the ScopedLayerAnimationSettings it should not have the invalid
state.

This apparently does trigger an invalid state, however it seems that it should.

Window::Show() performs the invalid state checks before calling
SetVisible(true). So a window can be set to visible=false and opacity=0, then
Show() would trigger visible=true and opacity=0 after the current check.

[jonross, 2014-09-23 21:06:20]

https://codereview.chromium.org/596863003/diff/1/ash/shelf/shelf_layout_manag...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/596863003/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:658: shelf_->status_area_widget()->Show();
On 2014/09/23 20:50:06, jonross wrote:
> On 2014/09/23 19:14:03, flackr wrote:
> > This doesn't cause the invalid window state (Visible window with 0 opacity)
> > updating visibility before we've set target opacity?
> > 
> > If you call show after setting up the opacity animation, but outside of the
> > scope of the ScopedLayerAnimationSettings it should not have the invalid
> state.
> 
> This apparently does trigger an invalid state, however it seems that it
should.
> 
> Window::Show() performs the invalid state checks before calling
> SetVisible(true). So a window can be set to visible=false and opacity=0, then
> Show() would trigger visible=true and opacity=0 after the current check.

Moved it for correctness.

[flackr, 2014-09-23 21:18:40]

https://codereview.chromium.org/596863003/diff/20001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/596863003/diff/20001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:694: // animate. Override the animation
settings to immediately set the
I'd prefer to see the scope of the ScopedLayerAnimationSettings above ending
before this call rather than setting up another ScopedLayerAnimationSettings.

[jonross, 2014-09-23 21:34:26]

https://codereview.chromium.org/596863003/diff/20001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/596863003/diff/20001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:694: // animate. Override the animation
settings to immediately set the
On 2014/09/23 21:18:40, flackr wrote:
> I'd prefer to see the scope of the ScopedLayerAnimationSettings above ending
> before this call rather than setting up another ScopedLayerAnimationSettings.

Done.

[flackr, 2014-09-23 21:56:22]

lgtm

https://codereview.chromium.org/596863003/diff/40001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/596863003/diff/40001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:654: base::AutoReset<bool>
auto_reset_updating_bounds(&updating_bounds_, true);
nit: I think this should be scoped to the entire method.

[jonross, 2014-09-23 22:03:35]

https://codereview.chromium.org/596863003/diff/40001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/596863003/diff/40001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:654: base::AutoReset<bool>
auto_reset_updating_bounds(&updating_bounds_, true);
On 2014/09/23 21:56:22, flackr wrote:
> nit: I think this should be scoped to the entire method.

Done.

[jonross, 2014-09-23 22:04:30]

jonross@chromium.org changed reviewers:
+ skuhne@chromium.org

[jonross, 2014-09-23 22:04:30]

skuhne@chromium.org: Please review changes in 

Hi Stefan,

Could you provide an owner review for this stable-blocker?

[Mr4D (OOO till 08-26), 2014-09-23 22:42:48]

lgtm.

[jonross, 2014-09-23 22:53:41]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-09-23 22:54:17]

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

[commit-bot: I haz the power, 2014-09-23 23:59:31]

Committed patchset #4 (id:60001) as fb21981785e6fdef809f5f899ab700df9acd72c1

[commit-bot: I haz the power, 2014-09-24 00:00:13]

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