Split AppListController::SetVisible into Show and Dismiss.

Split AppListController::SetVisible into Show and Dismiss.

This CL is in preparation for passing a reason for showing the app list.
This split is necessary to pass a reason only when showing and not
when dismissing.

BUG=391348
Committed: https://crrev.com/51802a7764aacfad5804a92e82e4f4fbaf35cd9b
Cr-Commit-Position: refs/heads/master@{#292080}

[Matt Giuca, 2014-08-19 08:26:30]

https://codereview.chromium.org/485453003/diff/1/ash/wm/app_list_controller.cc
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/485453003/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:234: ScheduleAnimation();
Is there a reason to invert the if statement? Why not just leave this where it
was (but delete the comment and the "if (!visible)" bit).

https://codereview.chromium.org/485453003/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:253: if (!view_)
This slightly changes the logic in that you won't call SchedulePaint if !view_,
whereas previously you would have.

Is that important?

[calamity, 2014-08-20 00:44:53]

https://codereview.chromium.org/485453003/diff/1/ash/wm/app_list_controller.cc
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/485453003/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:234: ScheduleAnimation();
On 2014/08/19 08:26:29, Matt Giuca wrote:
> Is there a reason to invert the if statement? Why not just leave this where it
> was (but delete the comment and the "if (!visible)" bit).

Done.

https://codereview.chromium.org/485453003/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:253: if (!view_)
On 2014/08/19 08:26:29, Matt Giuca wrote:
> This slightly changes the logic in that you won't call SchedulePaint if
!view_,
> whereas previously you would have.
> 
> Is that important?

I don't think this will be a problem. !view_ should never happen except while
the widget is being destroyed. To be absolutely safe, we can add a
SchedulePaint() to ResetView().

[Matt Giuca, 2014-08-20 06:13:12]

https://codereview.chromium.org/485453003/diff/1/ash/wm/app_list_controller.cc
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/485453003/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:253: if (!view_)
Note: We discussed this and decided to replace this if statement with a
DCHECK(view_).

It *should* always be true; if it isn't true, returning from here would not be
correct.

https://codereview.chromium.org/485453003/diff/40001/ash/wm/app_list_controll...
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/485453003/diff/40001/ash/wm/app_list_controll...
ash/wm/app_list_controller.cc:236: 
nit: extra blank line.

https://codereview.chromium.org/485453003/diff/40001/ash/wm/app_list_controll...
ash/wm/app_list_controller.cc:258: DCHECK(view_);
nit: I think this DCHECK should be immediately after the !is_visible if
statement. Add a comment // If the app list is currently visible, there should
be an existing view.

The rationale for both of these changes is that it makes it obvious why the
DCHECK should be valid.

[Matt Giuca, 2014-08-21 06:45:03]

Gaaaahh. This was supposed to be lgtm yesterday!!

[Matt Giuca, 2014-08-21 06:45:16]

Sorry!

[calamity, 2014-08-26 03:26:22]

calamity@chromium.org changed reviewers:
+ jamescook@chromium.org

[calamity, 2014-08-26 03:26:22]

+jamescook for OWNERS.

[James Cook, 2014-08-26 16:01:19]

LGTM

[calamity, 2014-08-27 01:09:43]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-08-27 01:12:21]

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

[commit-bot: I haz the power, 2014-08-27 02:11:37]

Committed patchset #4 (60001) as 04d442b1f39eab62b45e8dbe4132df68a0f0ac76

[commit-bot: I haz the power, 2014-09-10 02:49:09]

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