Reset the app list when it is shown on Windows and Linux.

Reset the app list when it is shown on Windows and Linux.

This CL makes the Windows and Linux app list show the root grid view
when Show() is called. This would previously show the folder view if the
folder view was displayed when the app list was hidden.

This CL adds a ResetForShow() to several of the app list views as a
consistent place to reset any dodgy state before showing the app list.

BUG=357058
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260523

[tapted, 2014-03-27 11:04:51]

I haven't had a thorough look, but some initial comments below.

And, it might feel wrong, but I think in the reset it would be preferable just
to iterate through *all* AppListItemView subviews, setting them all to visible,
rather than keeping that raw pointer around.

https://codereview.chromium.org/214423002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/app_list/linux/app_list_linux.cc (right):

https://codereview.chromium.org/214423002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/linux/app_list_linux.cc:64: if
(!view_->GetWidget()->IsVisible())
Can AppListView override WidgetObserver::OnWidgetVisibilityChanged(Widget*
widget, bool visible)?  (making sure to call the parent's override in
BubbleDelegateView::OnWidgetVisibilityChanged(..)).

Then, do this when visible becomes `false` - I think that would be a better
time/place to trigger the reset.

https://codereview.chromium.org/214423002/diff/20001/ui/app_list/views/apps_c...
File ui/app_list/views/apps_container_view.h (right):

https://codereview.chromium.org/214423002/diff/20001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.h:102: AppListItemView*
activated_folder_view_;
This raw pointer scares me a little - AppListItemViews have a habit of getting
deleted at awkward times.

[jennyz, 2014-03-27 18:35:54]

This cl fixes two different bugs, can you split it into two separate cls?

https://codereview.chromium.org/214423002/diff/30001/ui/app_list/views/apps_c...
File ui/app_list/views/apps_container_view.cc (right):

https://codereview.chromium.org/214423002/diff/30001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.cc:56: ResetForShow();
We don't need to ResetForShow in the constructor. The show_state_ is default to
SHOW_APPS.

https://codereview.chromium.org/214423002/diff/30001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.cc:231: activated_folder_view_ =
apps_grid_view_->activated_item_view();
Cache activated_folder_view_ seems dangerous,the item could go away due to sync
and the raw pointer then became invalid, lead to crash. We can find a better way
to track if the folder open/close animation completes and handle the case
better. I don't see the scenario of crbug.com/357099 happening on chromeos, I
wonder if the animation behaves differently on windows. Also, chromeos always
destroy and create the launcher bubble when close/open launcher, windows only
hide/show the UI, so that could make the difference in UI too.

[calamity, 2014-03-28 03:43:27]

https://codereview.chromium.org/214423002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/app_list/linux/app_list_linux.cc (right):

https://codereview.chromium.org/214423002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/linux/app_list_linux.cc:64: if
(!view_->GetWidget()->IsVisible())
On 2014/03/27 11:04:52, tapted wrote:
> Can AppListView override WidgetObserver::OnWidgetVisibilityChanged(Widget*
> widget, bool visible)?  (making sure to call the parent's override in
> BubbleDelegateView::OnWidgetVisibilityChanged(..)).
> 
> Then, do this when visible becomes `false` - I think that would be a better
> time/place to trigger the reset.

Done.

https://codereview.chromium.org/214423002/diff/20001/ui/app_list/views/apps_c...
File ui/app_list/views/apps_container_view.h (right):

https://codereview.chromium.org/214423002/diff/20001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.h:102: AppListItemView*
activated_folder_view_;
On 2014/03/27 11:04:52, tapted wrote:
> This raw pointer scares me a little - AppListItemViews have a habit of getting
> deleted at awkward times.

Removed.

https://codereview.chromium.org/214423002/diff/30001/ui/app_list/views/apps_c...
File ui/app_list/views/apps_container_view.cc (right):

https://codereview.chromium.org/214423002/diff/30001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.cc:56: ResetForShow();
On 2014/03/27 18:35:54, jennyz wrote:
> We don't need to ResetForShow in the constructor. The show_state_ is default
to
> SHOW_APPS.

Done. Note that this removes the SetState() call that was previously there.

https://codereview.chromium.org/214423002/diff/30001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.cc:231: activated_folder_view_ =
apps_grid_view_->activated_item_view();
On 2014/03/27 18:35:54, jennyz wrote:
> Cache activated_folder_view_ seems dangerous,the item could go away due to
sync
> and the raw pointer then became invalid, lead to crash. We can find a better
way
> to track if the folder open/close animation completes and handle the case
> better. I don't see the scenario of crbug.com/357099 happening on chromeos, I
> wonder if the animation behaves differently on windows. Also, chromeos always
> destroy and create the launcher bubble when close/open launcher, windows only
> hide/show the UI, so that could make the difference in UI too.

This does happen on ChromeOS (checked on latest canary). The case is going into
a folder, leaving the folder and entering a different folder; not closing the
launcher, sorry for being unclear.

[tapted, 2014-03-28 07:39:14]

lgtm % nits but this really needs a regression test. I think we are missing a
test harness at this level, but I have one in
https://codereview.chromium.org/210083009 which I'm using for mac-aura (turns
out: adding a test harness at this level is complicated!).

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_c...
File ui/app_list/views/apps_container_view.h (right):

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.h:22: class AppListItemView;
nit: this doesn't look needed any more

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.h:48: void ResetForShow();
nit: add a comment

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:88: void ResetForShow();
nit: add a comment. Perhaps a clearer name would be 'ResetForShowApps' too.

[jennyz, 2014-03-28 16:59:46]

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_c...
File ui/app_list/views/apps_container_view.h (right):

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.h:48: void ResetForShow();
Better name should be like tapted suggested: ResetForShowApps, please comment
this should be called when user open launcher for the first time, or close and
reopen it, since we only hide/show on windows, it reset the state to fresh.

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1840: activated_item_view_ = NULL;
Thanks!

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:88: void ResetForShow();
On 2014/03/28 07:39:15, tapted wrote:
> nit: add a comment. Perhaps a clearer name would be 'ResetForShowApps' too.

+1.

[jennyz, 2014-03-28 17:00:09]

lgtm with nits

[calamity, 2014-03-31 08:23:54]

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_c...
File ui/app_list/views/apps_container_view.h (right):

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.h:22: class AppListItemView;
On 2014/03/28 07:39:15, tapted wrote:
> nit: this doesn't look needed any more

Done.

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.h:48: void ResetForShow();
On 2014/03/28 16:59:46, jennyz wrote:
> Better name should be like tapted suggested: ResetForShowApps, please comment
> this should be called when user open launcher for the first time, or close and
> reopen it, since we only hide/show on windows, it reset the state to fresh.

Done.

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/214423002/diff/70001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:88: void ResetForShow();
On 2014/03/28 16:59:46, jennyz wrote:
> On 2014/03/28 07:39:15, tapted wrote:
> > nit: add a comment. Perhaps a clearer name would be 'ResetForShowApps' too.
> 
> +1.

Done.

[calamity, 2014-03-31 08:24:02]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-03-31 08:24:17]

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

[commit-bot: I haz the power, 2014-03-31 11:39:33]

Change committed as 260523