app_list: Show apps grid after installing from cws result.

app_list: Show apps grid after installing from cws result.

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

[xiyuan, 2013-08-01 20:47:28]

https://codereview.chromium.org/21592003/diff/1/chrome/browser/ui/views/app_l...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (left):

https://codereview.chromium.org/21592003/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:67: #endif
Removed because it is already included around line 47.

[benwells, 2013-08-01 22:53:12]

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

https://codereview.chromium.org/21592003/diff/1/ash/wm/app_list_controller.cc...
ash/wm/app_list_controller.cc:8: 
Nit: Why the new include? Is it for iwyu for existing code?

https://codereview.chromium.org/21592003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_result.cc (right):

https://codereview.chromium.org/21592003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_result.cc:186:
controller_->ShowAppsGrid();
Can the websore result take a reference to the view instead of plumbing through
the controller? Are there lifetime issues with that if the view is hidden? It
would be much simpler....

https://codereview.chromium.org/21592003/diff/1/chrome/browser/ui/views/app_l...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/21592003/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:560:
DCHECK(current_view_);
Nit: it's not obvious why this assumption is valid. Can you add a comment
explaining? We might want this api to be used more in future.

https://codereview.chromium.org/21592003/diff/1/ui/app_list/views/app_list_vi...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/21592003/diff/1/ui/app_list/views/app_list_vi...
ui/app_list/views/app_list_view.cc:186: app_list_main_view_->ShowAppsGrid();
Should you use app_list_main_view_->search_box_view()->ClearSearch() instead?
Won't this leave the current search in the box, or is that intentional?

[xiyuan, 2013-08-01 23:18:28]

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

https://codereview.chromium.org/21592003/diff/1/ash/wm/app_list_controller.cc...
ash/wm/app_list_controller.cc:8: 
On 2013/08/01 22:53:12, benwells wrote:
> Nit: Why the new include? Is it for iwyu for existing code?

Yep, it's for the std::min/max in existing code. Reported by lint.

https://codereview.chromium.org/21592003/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/webstore_result.cc (right):

https://codereview.chromium.org/21592003/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/webstore_result.cc:186:
controller_->ShowAppsGrid();
On 2013/08/01 22:53:12, benwells wrote:
> Can the websore result take a reference to the view instead of plumbing
through
> the controller? Are there lifetime issues with that if the view is hidden? It
> would be much simpler....

Right now, the AppListModel is independent of AppListView. WebstoreResult is
part of the model. It does not sound right to put a view reference in the model.

Besides, the model is constructed without the view reference. To pass it down
would also take some work.

https://codereview.chromium.org/21592003/diff/1/chrome/browser/ui/views/app_l...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/21592003/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:560:
DCHECK(current_view_);
On 2013/08/01 22:53:12, benwells wrote:
> Nit: it's not obvious why this assumption is valid. Can you add a comment
> explaining? We might want this api to be used more in future.

Well, you have to create a view before you can change its state, right?

Do you prefer to make ShowAppsGrid() a no-op if there is no |current_view_|?

https://codereview.chromium.org/21592003/diff/1/ui/app_list/views/app_list_vi...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/21592003/diff/1/ui/app_list/views/app_list_vi...
ui/app_list/views/app_list_view.cc:186: app_list_main_view_->ShowAppsGrid();
On 2013/08/01 22:53:12, benwells wrote:
> Should you use app_list_main_view_->search_box_view()->ClearSearch() instead?
> Won't this leave the current search in the box, or is that intentional?

I was thinking of leaving the query there might save user some typing. But now I
think you are right that it is better to keep UI consistent.

Changed as suggested.

[tapted, 2013-08-01 23:28:27]

drive-by: Would a valid alternative be to hook in to
AppsGridView::EnsureViewVisible / AppsGridViewDelegate? That is,
'EnsureViewVisible' could also ensure that the apps grid is shown, rather than
search results. I think a successful webstore install calls this already.

Although it might need some separation to keep the results displayed at the
start of a one-click install..

[xiyuan, 2013-08-02 00:34:56]

On 2013/08/01 23:28:27, tapted wrote:
> drive-by: Would a valid alternative be to hook in to
> AppsGridView::EnsureViewVisible / AppsGridViewDelegate? That is,
> 'EnsureViewVisible' could also ensure that the apps grid is shown, rather than
> search results. I think a successful webstore install calls this already.
> 
> Although it might need some separation to keep the results displayed at the
> start of a one-click install..

How would the separation code be written? We might need to remember that there
is an install from the search results and delay the switching if the install
notifications are for this install. It feels like a work around and easy to
break. I think the current way is cleaner.

[benwells, 2013-08-02 04:26:25]

On 2013/08/02 00:34:56, xiyuan wrote:
> On 2013/08/01 23:28:27, tapted wrote:
> > drive-by: Would a valid alternative be to hook in to
> > AppsGridView::EnsureViewVisible / AppsGridViewDelegate? That is,
> > 'EnsureViewVisible' could also ensure that the apps grid is shown, rather
than
> > search results. I think a successful webstore install calls this already.
> > 
> > Although it might need some separation to keep the results displayed at the
> > start of a one-click install..
> 
> How would the separation code be written? We might need to remember that there
> is an install from the search results and delay the switching if the install
> notifications are for this install. It feels like a work around and easy to
> break. I think the current way is cleaner.

I agree we shouldn't add direct references to the view in the model, that was a
bad idea :)

Could we go through the AppListItemModelObserver, or some new observer? That
feels like it would be in harmony with the current design and remove a lot of
the plumbing needed to do this.

[benwells, 2013-08-02 04:26:41]

And one nit on a nit ...

https://codereview.chromium.org/21592003/diff/1/chrome/browser/ui/views/app_l...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/21592003/diff/1/chrome/browser/ui/views/app_l...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:560:
DCHECK(current_view_);
On 2013/08/01 23:18:28, xiyuan wrote:
> On 2013/08/01 22:53:12, benwells wrote:
> > Nit: it's not obvious why this assumption is valid. Can you add a comment
> > explaining? We might want this api to be used more in future.
> 
> Well, you have to create a view before you can change its state, right?
> 
> Do you prefer to make ShowAppsGrid() a no-op if there is no |current_view_|?

No, DCHECK is right. Sorry I should have been more clear. Let's say at some time
in the future some other part of the code wants to use this. The programmer
calls it and then under very rare circumstances it will hit this DCHECK (which
is better than silently failing as it would be a bug).

If I was that programmer a comment about why the DCHECK was there would help.

A comment like: 'This function is only called from app list code, so we can
assume |current_view_| exists. If you hit this DCHECK you probably need to
create and show the app list programmatically first.

[xiyuan, 2013-08-02 04:53:24]

On 2013/08/02 04:26:25, benwells wrote:
> On 2013/08/02 00:34:56, xiyuan wrote:
> > On 2013/08/01 23:28:27, tapted wrote:
> > > drive-by: Would a valid alternative be to hook in to
> > > AppsGridView::EnsureViewVisible / AppsGridViewDelegate? That is,
> > > 'EnsureViewVisible' could also ensure that the apps grid is shown, rather
> than
> > > search results. I think a successful webstore install calls this already.
> > > 
> > > Although it might need some separation to keep the results displayed at
the
> > > start of a one-click install..
> > 
> > How would the separation code be written? We might need to remember that
there
> > is an install from the search results and delay the switching if the install
> > notifications are for this install. It feels like a work around and easy to
> > break. I think the current way is cleaner.
> 
> I agree we shouldn't add direct references to the view in the model, that was
a
> bad idea :)
> 
> Could we go through the AppListItemModelObserver, or some new observer? That
> feels like it would be in harmony with the current design and remove a lot of
> the plumbing needed to do this.

Good idea. This would much cleaner than the current CL. Let me give it a try.

[xiyuan, 2013-08-02 05:20:00]

Revised to use model/observer to handle the installed event. Like the new
approach better. Thanks. :)

[benwells, 2013-08-02 07:11:42]

cool, lgtm :)

[commit-bot: I haz the power, 2013-08-02 14:28:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/21592003/39001

[commit-bot: I haz the power, 2013-08-02 18:48:36]

Change committed as 215331