Fix shelf auto-hide calculation for app-list visibility.

Fix shelf auto-hide calculation for app-list visibility.

AppListButton tracks per-display [target] visibility.
(sets a new flag on Shown/Dismissed callbacks)

Check widget/button state, not global shell state.
Ignore dismissed app-list windows that are animating closed.

Fix a ShelfLayoutManager test to actually catch this.

BUG=654661
TEST=Shelf stays visible when the app-list is hidden.
R=jamescook@chromium.org

Committed: https://crrev.com/f05a6730ca4ecdfe336276f0f96d33c2aeb028be
Cr-Commit-Position: refs/heads/master@{#435554}

[msw, 2016-11-30 22:16:03]

Patchset #2 (id:20001) has been deleted

[msw, 2016-11-30 22:16:30]

Description was changed from

==========
Fix shelf auto-hide calculation for app-list visiibility.

Support display-specific app-list visibility checks.
Check the app-list's target and current visibility.

BUG=654661
TEST=Shelf stays visible when the app-list is hidden.
R=jamescook@chromium.org
==========

to

==========
Fix shelf auto-hide calculation for app-list visiibility.

Support display-specific app-list visibility checks.
Check the app-list's target and current visibility.
Fix a ShelfLayoutManager test to actually catch this.

BUG=654661
TEST=Shelf stays visible when the app-list is hidden.
R=jamescook@chromium.org
==========

[msw, 2016-11-30 22:18:42]

Description was changed from

==========
Fix shelf auto-hide calculation for app-list visiibility.

Support display-specific app-list visibility checks.
Check the app-list's target and current visibility.
Fix a ShelfLayoutManager test to actually catch this.

BUG=654661
TEST=Shelf stays visible when the app-list is hidden.
R=jamescook@chromium.org
==========

to

==========
Fix shelf auto-hide calculation for app-list visiibility.

Support display-specific app-list visibility checks.
Check the app-list's target and current visibility.
Fix a ShelfLayoutManager test to actually catch this.

BUG=654661
TEST=Shelf stays visible when the app-list is hidden.
R=jamescook@chromium.org,stevenjb@chromium.org
==========

[msw, 2016-11-30 22:18:42]

msw@chromium.org changed reviewers:
+ stevenjb@chromium.org

[msw, 2016-11-30 22:20:08]

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

[commit-bot: I haz the power, 2016-11-30 22:20:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2016-11-30 22:58:59]

Hey James, please take a look at ash/*, thanks!
Hey Steven, please take a look at ui/app_list/*, thanks!

[commit-bot: I haz the power, 2016-11-30 23:21:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 23:21:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2016-11-30 23:27:20]

nit: Typo "visibility" in CL description

Thanks for jumping on this!

https://codereview.chromium.org/2534953006/diff/40001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2534953006/diff/40001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:279: // Pass a valid display id to check target visibility
for a specific display.
If you don't pass a valid id what does it check? Just primary display or all
displays?

https://codereview.chromium.org/2534953006/diff/40001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:285: int64_t display_id = display::kInvalidDisplayId)
const;
optional: I wonder if these methods would be clearer if they didn't use default
parameters, but had two explicit versions of each method. Then it would be
clearer that IsAppListVisible() means visible-anywhere and
IsAppListVisible(display_id) means just that display. The fact that you're using
kInvalidDisplayId to mean "any display" is kind of an implementation detail.

(And I see there are 55 callers of GetAppListTargetVisibility() so it's quite
reasonable not to change all the callers to pass an explicit display id.)

https://codereview.chromium.org/2534953006/diff/40001/ash/mus/app_list_presen...
File ash/mus/app_list_presenter_mus.h (right):

https://codereview.chromium.org/2534953006/diff/40001/ash/mus/app_list_presen...
ash/mus/app_list_presenter_mus.h:30: int64_t display_id =
display::kInvalidDisplayId) const override;
I don't think you're allowed to use default parameters on virtual functions.

https://google.github.io/styleguide/cppguide.html#Default_Arguments

https://codereview.chromium.org/2534953006/diff/40001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2534953006/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:1165:
EXPECT_EQ(SHELF_AUTO_HIDE_SHOWN, shelf->GetAutoHideState());
Thanks for checking this case.

https://codereview.chromium.org/2534953006/diff/40001/ui/app_list/presenter/a...
File ui/app_list/presenter/app_list_presenter_impl.cc (right):

https://codereview.chromium.org/2534953006/diff/40001/ui/app_list/presenter/a...
ui/app_list/presenter/app_list_presenter_impl.cc:117: }
this function might be easier to read with some early return false-es.

https://codereview.chromium.org/2534953006/diff/40001/ui/app_list/presenter/a...
ui/app_list/presenter/app_list_presenter_impl.cc:120: return target_display_id_
!= display::kInvalidDisplayId &&
nit: This might be clearer as:

if (target_display_id_ == invalid)
  return false;  // Not shown anywhere.

return display_id == target_ || display_id == invalid;

or something like that.

https://codereview.chromium.org/2534953006/diff/40001/ui/app_list/presenter/a...
File ui/app_list/presenter/app_list_presenter_impl.h (right):

https://codereview.chromium.org/2534953006/diff/40001/ui/app_list/presenter/a...
ui/app_list/presenter/app_list_presenter_impl.h:102: // means the target
visiblity is shown, while invalid means hidden.
nit: visibility

https://codereview.chromium.org/2534953006/diff/40001/ui/app_list/presenter/a...
ui/app_list/presenter/app_list_presenter_impl.h:103: int64_t target_display_id_
= display::kInvalidDisplayId;
optional: I'd also be OK with separate target_visibility_ and target_display_id_
params if you didn't want to have to explain that invalid means hidden.

[msw, 2016-11-30 23:32:40]

Description was changed from

==========
Fix shelf auto-hide calculation for app-list visiibility.

Support display-specific app-list visibility checks.
Check the app-list's target and current visibility.
Fix a ShelfLayoutManager test to actually catch this.

BUG=654661
TEST=Shelf stays visible when the app-list is hidden.
R=jamescook@chromium.org,stevenjb@chromium.org
==========

to

==========
Fix shelf auto-hide calculation for app-list visibility.

Support display-specific app-list visibility checks.
Check the app-list's target and current visibility.
Fix a ShelfLayoutManager test to actually catch this.

BUG=654661
TEST=Shelf stays visible when the app-list is hidden.
R=jamescook@chromium.org,stevenjb@chromium.org
==========

[msw, 2016-12-01 01:42:52]

Description was changed from

==========
Fix shelf auto-hide calculation for app-list visibility.

Support display-specific app-list visibility checks.
Check the app-list's target and current visibility.
Fix a ShelfLayoutManager test to actually catch this.

BUG=654661
TEST=Shelf stays visible when the app-list is hidden.
R=jamescook@chromium.org,stevenjb@chromium.org
==========

to

==========
Fix shelf auto-hide calculation for app-list visibility.

AppListButton tracks per-display [target] visibility.
(sets a new flag on Shown/Dismissed callbacks)

Check widget/button state, not global shell state.
Ignore dismissed app-list windows that are animating closed.

Fix a ShelfLayoutManager test to actually catch this.

BUG=654661
TEST=Shelf stays visible when the app-list is hidden.
R=jamescook@chromium.org
==========

[msw, 2016-12-01 01:42:52]

msw@chromium.org changed reviewers:
- stevenjb@chromium.org

[msw, 2016-12-01 01:43:47]

Hey James, please take a fresh look at this new approach; thanks!
It's not ideal, but we'll need to update this for mash anyway...

[msw, 2016-12-01 01:43:55]

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

[commit-bot: I haz the power, 2016-12-01 01:44:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-12-01 02:02:08]

LGTM with nits

https://codereview.chromium.org/2534953006/diff/60001/ash/common/shelf/app_li...
File ash/common/shelf/app_list_button.h (right):

https://codereview.chromium.org/2534953006/diff/60001/ash/common/shelf/app_li...
ash/common/shelf/app_list_button.h:68: // True if the app list is currently
showing for this button.
nit: button/display (since the fact that this is per-display is important). You
might also comment that this is different from WmShell::IsApplistVisible().

Basically, it's weird that we can't use WmShell::IsApplistVisible() so it would
be nice to say something about that somewhere.

https://codereview.chromium.org/2534953006/diff/60001/ash/common/shelf/app_li...
ash/common/shelf/app_list_button.h:69: bool is_showing_app_list_ = false;
nit: I prefer to either initialize all members in the header or all members in
the .cc file. In this case, it looks like they will all need to go in .cc since
some of them are constructed.

[commit-bot: I haz the power, 2016-12-01 02:24:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 02:24:38]

Dry run: This issue passed the CQ dry run.

[msw, 2016-12-01 04:18:13]

https://codereview.chromium.org/2534953006/diff/60001/ash/common/shelf/app_li...
File ash/common/shelf/app_list_button.h (right):

https://codereview.chromium.org/2534953006/diff/60001/ash/common/shelf/app_li...
ash/common/shelf/app_list_button.h:68: // True if the app list is currently
showing for this button.
On 2016/12/01 02:02:07, James Cook wrote:
> nit: button/display (since the fact that this is per-display is important).
You
> might also comment that this is different from WmShell::IsApplistVisible().
> 
> Basically, it's weird that we can't use WmShell::IsApplistVisible() so it
would
> be nice to say something about that somewhere.
> 

Done.

https://codereview.chromium.org/2534953006/diff/60001/ash/common/shelf/app_li...
ash/common/shelf/app_list_button.h:69: bool is_showing_app_list_ = false;
On 2016/12/01 02:02:07, James Cook wrote:
> nit: I prefer to either initialize all members in the header or all members in
> the .cc file. In this case, it looks like they will all need to go in .cc
since
> some of them are constructed.

Done.

[msw, 2016-12-01 04:18:39]

The CQ bit was checked by msw@chromium.org

[msw, 2016-12-01 04:18:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2534953006/#ps80001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-12-01 04:18:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 04:51:34]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480565919784610,
"parent_rev": "4633a3780a6aaf2cd99ee5161966322adfc4f07d", "commit_rev":
"e80f42e5990b12afa2e69862f2e2a025889b6749"}

[commit-bot: I haz the power, 2016-12-01 04:52:20]

Description was changed from

==========
Fix shelf auto-hide calculation for app-list visibility.

AppListButton tracks per-display [target] visibility.
(sets a new flag on Shown/Dismissed callbacks)

Check widget/button state, not global shell state.
Ignore dismissed app-list windows that are animating closed.

Fix a ShelfLayoutManager test to actually catch this.

BUG=654661
TEST=Shelf stays visible when the app-list is hidden.
R=jamescook@chromium.org
==========

to

==========
Fix shelf auto-hide calculation for app-list visibility.

AppListButton tracks per-display [target] visibility.
(sets a new flag on Shown/Dismissed callbacks)

Check widget/button state, not global shell state.
Ignore dismissed app-list windows that are animating closed.

Fix a ShelfLayoutManager test to actually catch this.

BUG=654661
TEST=Shelf stays visible when the app-list is hidden.
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2016-12-01 04:52:21]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-12-01 04:55:39]

Description was changed from

==========
Fix shelf auto-hide calculation for app-list visibility.

AppListButton tracks per-display [target] visibility.
(sets a new flag on Shown/Dismissed callbacks)

Check widget/button state, not global shell state.
Ignore dismissed app-list windows that are animating closed.

Fix a ShelfLayoutManager test to actually catch this.

BUG=654661
TEST=Shelf stays visible when the app-list is hidden.
R=jamescook@chromium.org
==========

to

==========
Fix shelf auto-hide calculation for app-list visibility.

AppListButton tracks per-display [target] visibility.
(sets a new flag on Shown/Dismissed callbacks)

Check widget/button state, not global shell state.
Ignore dismissed app-list windows that are animating closed.

Fix a ShelfLayoutManager test to actually catch this.

BUG=654661
TEST=Shelf stays visible when the app-list is hidden.
R=jamescook@chromium.org

Committed: https://crrev.com/f05a6730ca4ecdfe336276f0f96d33c2aeb028be
Cr-Commit-Position: refs/heads/master@{#435554}
==========

[commit-bot: I haz the power, 2016-12-01 04:55:40]

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