Notify launcher page with onTransitionChanged event

Notify launcher page with onTransitionChanged event.

This CL adds a launcherPage API which notifies the launcher page
when the app launcher switches to and from the custom launcher page.

These events are only sent to the first custom launcher page.

BUG=425444
TBR=oshima@chromium.org

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

[calamity, 2014-10-28 00:53:31]

Patchset #1 (id:1) has been deleted

[calamity, 2014-10-28 00:55:10]

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

[Matt Giuca, 2014-10-28 05:41:22]

lgtm with nits

https://codereview.chromium.org/683703002/diff/20001/chrome/browser/apps/cust...
File chrome/browser/apps/custom_launcher_page_browsertest_views.cc (right):

https://codereview.chromium.org/683703002/diff/20001/chrome/browser/apps/cust...
chrome/browser/apps/custom_launcher_page_browsertest_views.cc:65:
OpenLauncherAndShowCustomPage) {
Perhaps s/Show/SwitchTo/

https://codereview.chromium.org/683703002/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/683703002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:169: // Overridden from
app_list::AppListModelObserver:
Nit: overrides ;)

https://codereview.chromium.org/683703002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:173: if (new_state ==
app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE) {
// If transitioning to the custom launcher page, send the onPageExpand event to
the page.

https://codereview.chromium.org/683703002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:179:
app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE) {
// If transitioning from the custom launcher page, send the onPageCollapse event
to the page.

https://codereview.chromium.org/683703002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/683703002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/_permission_features.json:584:
"07BD6A765FFC289FF755D7CAB2893A40EC337FEC",  // http://crbug.com/404000
Is there a way to avoid duplicating this whitelist?

[calamity, 2014-10-28 06:34:20]

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

[calamity, 2014-10-28 06:34:21]

+benwells for OWNERS of all things extensions.

https://codereview.chromium.org/683703002/diff/20001/chrome/browser/apps/cust...
File chrome/browser/apps/custom_launcher_page_browsertest_views.cc (right):

https://codereview.chromium.org/683703002/diff/20001/chrome/browser/apps/cust...
chrome/browser/apps/custom_launcher_page_browsertest_views.cc:65:
OpenLauncherAndShowCustomPage) {
On 2014/10/28 05:41:22, Matt Giuca wrote:
> Perhaps s/Show/SwitchTo/

Done.

https://codereview.chromium.org/683703002/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/683703002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:169: // Overridden from
app_list::AppListModelObserver:
On 2014/10/28 05:41:22, Matt Giuca wrote:
> Nit: overrides ;)

git grep '// Overridden from ' | wc -l
1770

git grep '//.*overrides:' | wc -l
1054

Overridden from forever!

https://codereview.chromium.org/683703002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:173: if (new_state ==
app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE) {
On 2014/10/28 05:41:22, Matt Giuca wrote:
> // If transitioning to the custom launcher page, send the onPageExpand event
to
> the page.

Done.

https://codereview.chromium.org/683703002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:179:
app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE) {
On 2014/10/28 05:41:22, Matt Giuca wrote:
> // If transitioning from the custom launcher page, send the onPageCollapse
event
> to the page.

Done.

https://codereview.chromium.org/683703002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/683703002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/_permission_features.json:584:
"07BD6A765FFC289FF755D7CAB2893A40EC337FEC",  // http://crbug.com/404000
On 2014/10/28 05:41:22, Matt Giuca wrote:
> Is there a way to avoid duplicating this whitelist?

Done.

[Matt Giuca, 2014-10-28 07:44:43]

lgtm

[calamity, 2014-10-31 03:12:58]

I changed this to be onTransitionChanged() since it's easier to go from there to
a onExpand() + onCollapse() API than it is to go the other way. This has
necessitated just a smidgen of extra code.

Salient points:
- LauncherPageEventDispatcherViews listens to the ContentsView's PaginationModel
for transition information because it would kinda gross to duplicate the
information in the AppListModel.
- To do this, we need to reach into the AppListView, which is being done by
asking the AppListServiceViews on non-ash and the Shell on ash.
- To accommodate this, AppListView's OnViewInitialized() call has been pushed up
a level into the AppListView creators (AppListService and AppListController) so
that the LauncherPageEventDispatcherViews can be created _after_ the AppListView
creator has updated its internal state, making the AppListView accessible
(otherwise we'd get notified during the AppListView creation and asking the
AppListService/Controller for its AppListView would yield nullptr).
- LauncherPageEventDispatcherViews needs to be attached to this on AppListView
creation as well as on profile change when the ContentsView switches.

Things I'm not concerned about:
- Making AppListService/Controller call OnViewInitialized() on the ALVD is a bit
gross. There seems to be an AppListService/AppListControllerDelegate which may
be other places we can plumb things but it doesn't look exactly easy..
- Seems kinda bad to have the LauncherPageEventDispatcher figure out the right
place to get the AppListView. Making AppListServiceAsh a subclass of
AppListServiceViews would at least make that bit less bad.
- Maybe putting the ContentsView's PaginationModel into the AppListModel would
be a better way to manage all of this?

Enjoy!

[tapted, 2014-11-03 00:31:20]

tapted@chromium.org changed reviewers:
+ tapted@chromium.org

[tapted, 2014-11-03 00:31:21]

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:470:
controller_->GetAppListWindow());
(overridden by later comments, but worth noting: 
GetAppListWindow can return NULL, which I guess will work here, but it's yucky.
There is AppListControllerDelegate::ForceNativeDesktop which returns a bool. You
can probably change that to return a desktop type (it's a bool to be consistent
with the app_list::AppListViewDelegate interface, which doesn't know about
chrome::DesktopType).

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:484:
launcher_page_event_dispatcher_.reset();
This lifetime is really confusing. Why can't the event dispatcher lifetime be
coupled with the WebContents?

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/launcher_page_event_dispatcher_views.cc (right):

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/launcher_page_event_dispatcher_views.cc:32:
app_list_view = ash::Shell::GetInstance()->GetAppListView();
ash::Shell shouldn't be used here - Mac will need to compile this code. It looks
like you could maybe pass in the AppListControllerDelegate instance, and have it
return a gfx::NativeView which must be guaranteed to be non-null here. That will
get rid of the ash dependency.

But I think the lifetime of the event dispatcher overall needs to be improved.

https://codereview.chromium.org/683703002/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/launcher_page_private.idl (right):

https://codereview.chromium.org/683703002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/launcher_page_private.idl:9: static void
onTransitionChanged(double progress);
What's the long-term plan for `progress`? In the current form, it looks like you
could just observe via `document.addEventListener('visibilitychange', function()
{..}` (and if that's not working properly for the custom launcher pages then it
should be fixed).

[Matt Giuca, 2014-11-03 00:57:42]

Urrgghhh. This is all pretty awful but seems necessary :|

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/apps/cus...
File chrome/browser/apps/custom_launcher_page_browsertest_views.cc (right):

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/apps/cus...
chrome/browser/apps/custom_launcher_page_browsertest_views.cc:81:
ExtensionTestMessageListener listener("onPageExpanded", false);
Looks like this hasn't been updated with the new API.

(Does it still pass? If so, that's troubling.)

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/launcher_page_event_dispatcher_views.cc (right):

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/launcher_page_event_dispatcher_views.cc:41:
main_view_->contents_view()->pagination_model()->AddObserver(this);
It might be worth adding a note here about why all the complexity later is
needed.

// Observe the app list's ContentsView's PaginationModel. Note that the
ContentsView can be destroyed and a new one can be created in its place. We
observe these events and remove/add ourselves as an observer of the
PaginationModel as appropriate.

(Or rephrase in your own words.)

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/launcher_page_event_dispatcher_views.cc:46:
main_view_->contents_view()->pagination_model()->RemoveObserver(this);
Should there be null checks on contents_view() everywhere in this file? (Since
you could be in between OnContentsViewDestroying and OnContentsViewCreated
events, I assume there are therefore times where there is no ContentsView in
existence.)

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/launcher_page_event_dispatcher_views.cc:76:
ContentsView* contents_view = main_view_->contents_view();
Null check here? (Maybe not necessary as in the destructor, because you wouldn't
get a TransitionChanged event unless there was a PaginationModel... but at least
DCHECK.)

https://codereview.chromium.org/683703002/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/683703002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/_api_features.json:466: "launcherPagePrivate": {
I thought we agreed to just call this launcherPage to avoid any churn later if
we decide to make launcher pages public. (It is effectively a non-whitelisted
API that can only be used by whitelisted extensions.)

https://codereview.chromium.org/683703002/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/launcher_page_private.idl (right):

https://codereview.chromium.org/683703002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/launcher_page_private.idl:9: static void
onTransitionChanged(double progress);
Where do you introduce the ability for JS code to subscribe to this event? Is it
all automatic?

I think all of this API stuff should also be split up into a separate CL (so you
introduce all the IDL and the API itself, but the event never receives any calls
until this follow-up CL).

https://codereview.chromium.org/683703002/diff/100001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/custom_launcher_page/main.js
(right):

https://codereview.chromium.org/683703002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/custom_launcher_page/main.js:5:
chrome.launcherPagePrivate.onTransitionChanged.addListener(function(progress) {
Same here... doesn't seem updated to the new API.

https://codereview.chromium.org/683703002/diff/100001/ui/app_list/app_list_mo...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/683703002/diff/100001/ui/app_list/app_list_mo...
ui/app_list/app_list_model.h:46: STATE_CUSTOM_LAUNCHER_PAGE,
I think you can move this change, and the changes to ContentsView (introducing
the CUSTOM_LAUNCHER_PAGE state) to a separate CL at least.

https://codereview.chromium.org/683703002/diff/100001/ui/app_list/app_list_vi...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/683703002/diff/100001/ui/app_list/app_list_vi...
ui/app_list/app_list_view_delegate.h:113: virtual void
ViewReinitializedForProfileChange() = 0;
Drop the "ForProfileChange". Just "ViewReinitialized", it's cleaner. (No need to
say *why* it was called because in the future it could be called for another
reason.)

https://codereview.chromium.org/683703002/diff/100001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/683703002/diff/100001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:426: 
Why did this go away?

[Matt Giuca, 2014-11-03 02:34:37]

not lgtm, because I haven't given an lg yet for the rewrite since my last
review.

[calamity, 2014-11-06 02:20:56]

Patchset #7 (id:140001) has been deleted

[calamity, 2014-11-06 02:24:09]

Patchset #7 (id:160001) has been deleted

[calamity, 2014-11-06 02:36:30]

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

[calamity, 2014-11-06 02:36:32]

+tapted for design review.

3rd rewrite. The animation information is now forwarded directly to the ALVD.

The lifetime of the LPED is the same as the custom launcher pages.

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/apps/cus...
File chrome/browser/apps/custom_launcher_page_browsertest_views.cc (right):

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/apps/cus...
chrome/browser/apps/custom_launcher_page_browsertest_views.cc:81:
ExtensionTestMessageListener listener("onPageExpanded", false);
On 2014/11/03 00:57:41, Matt Giuca wrote:
> Looks like this hasn't been updated with the new API.
> 
> (Does it still pass? If so, that's troubling.)

It has been? It listens to the new event. It's just called this because these
are the semantic changes that occurred.

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:470:
controller_->GetAppListWindow());
On 2014/11/03 00:31:20, tapted wrote:
> (overridden by later comments, but worth noting: 
> GetAppListWindow can return NULL, which I guess will work here, but it's
yucky.
> There is AppListControllerDelegate::ForceNativeDesktop which returns a bool.
You
> can probably change that to return a desktop type (it's a bool to be
consistent
> with the app_list::AppListViewDelegate interface, which doesn't know about
> chrome::DesktopType).

Acknowledged.

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:484:
launcher_page_event_dispatcher_.reset();
On 2014/11/03 00:31:20, tapted (OOO until Nov 1st) wrote:
> This lifetime is really confusing. Why can't the event dispatcher lifetime be
> coupled with the WebContents?

Done.

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/launcher_page_event_dispatcher_views.cc (right):

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/launcher_page_event_dispatcher_views.cc:32:
app_list_view = ash::Shell::GetInstance()->GetAppListView();
On 2014/11/03 00:31:20, tapted wrote:
> ash::Shell shouldn't be used here - Mac will need to compile this code. It
looks
> like you could maybe pass in the AppListControllerDelegate instance, and have
it
> return a gfx::NativeView which must be guaranteed to be non-null here. That
will
> get rid of the ash dependency.
> 
> But I think the lifetime of the event dispatcher overall needs to be improved.

Done.

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/launcher_page_event_dispatcher_views.cc:41:
main_view_->contents_view()->pagination_model()->AddObserver(this);
On 2014/11/03 00:57:41, Matt Giuca wrote:
> It might be worth adding a note here about why all the complexity later is
> needed.
> 
> // Observe the app list's ContentsView's PaginationModel. Note that the
> ContentsView can be destroyed and a new one can be created in its place. We
> observe these events and remove/add ourselves as an observer of the
> PaginationModel as appropriate.
> 
> (Or rephrase in your own words.)

Code is gone.

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/launcher_page_event_dispatcher_views.cc:46:
main_view_->contents_view()->pagination_model()->RemoveObserver(this);
On 2014/11/03 00:57:41, Matt Giuca wrote:
> Should there be null checks on contents_view() everywhere in this file? (Since
> you could be in between OnContentsViewDestroying and OnContentsViewCreated
> events, I assume there are therefore times where there is no ContentsView in
> existence.)

Code is gone.

https://codereview.chromium.org/683703002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/launcher_page_event_dispatcher_views.cc:76:
ContentsView* contents_view = main_view_->contents_view();
On 2014/11/03 00:57:42, Matt Giuca wrote:
> Null check here? (Maybe not necessary as in the destructor, because you
wouldn't
> get a TransitionChanged event unless there was a PaginationModel... but at
least
> DCHECK.)

Code is gone.

https://codereview.chromium.org/683703002/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/683703002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/_api_features.json:466: "launcherPagePrivate": {
On 2014/11/03 00:57:42, Matt Giuca wrote:
> I thought we agreed to just call this launcherPage to avoid any churn later if
> we decide to make launcher pages public. (It is effectively a non-whitelisted
> API that can only be used by whitelisted extensions.)

Done.

https://codereview.chromium.org/683703002/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/launcher_page_private.idl (right):

https://codereview.chromium.org/683703002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/launcher_page_private.idl:9: static void
onTransitionChanged(double progress);
On 2014/11/03 00:31:20, tapted wrote:
> What's the long-term plan for `progress`? In the current form, it looks like
you
> could just observe via `document.addEventListener('visibilitychange',
function()
> {..}` (and if that's not working properly for the custom launcher pages then
it
> should be fixed).

progress is used for touch events which will require constant updates.

https://codereview.chromium.org/683703002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/launcher_page_private.idl:9: static void
onTransitionChanged(double progress);
On 2014/11/03 00:57:42, Matt Giuca wrote:
> Where do you introduce the ability for JS code to subscribe to this event? Is
it
> all automatic?
> 
> I think all of this API stuff should also be split up into a separate CL (so
you
> introduce all the IDL and the API itself, but the event never receives any
calls
> until this follow-up CL).

Yes. It's all automatic. I'll split it after another review round.

https://codereview.chromium.org/683703002/diff/100001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/custom_launcher_page/main.js
(right):

https://codereview.chromium.org/683703002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/custom_launcher_page/main.js:5:
chrome.launcherPagePrivate.onTransitionChanged.addListener(function(progress) {
On 2014/11/03 00:57:42, Matt Giuca wrote:
> Same here... doesn't seem updated to the new API.

It has. onTransitionChanged is the new API. The code below then sends a message
on 0 and on 1 which represent the fully collapsed and fully expanded states.

https://codereview.chromium.org/683703002/diff/100001/ui/app_list/app_list_mo...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/683703002/diff/100001/ui/app_list/app_list_mo...
ui/app_list/app_list_model.h:46: STATE_CUSTOM_LAUNCHER_PAGE,
On 2014/11/03 00:57:42, Matt Giuca wrote:
> I think you can move this change, and the changes to ContentsView (introducing
> the CUSTOM_LAUNCHER_PAGE state) to a separate CL at least.

The var would be unused. Is there a point to this?

https://codereview.chromium.org/683703002/diff/100001/ui/app_list/app_list_vi...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/683703002/diff/100001/ui/app_list/app_list_vi...
ui/app_list/app_list_view_delegate.h:113: virtual void
ViewReinitializedForProfileChange() = 0;
On 2014/11/03 00:57:42, Matt Giuca wrote:
> Drop the "ForProfileChange". Just "ViewReinitialized", it's cleaner. (No need
to
> say *why* it was called because in the future it could be called for another
> reason.)

Code is gone.

https://codereview.chromium.org/683703002/diff/100001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/683703002/diff/100001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:426: 
On 2014/11/03 00:57:42, Matt Giuca wrote:
> Why did this go away?

It's back. From outer space.

[tapted, 2014-11-06 06:05:45]

I like it!

(sorry for the lag - make sure you prod me in case I'm buried in compositor
debugging again)

https://codereview.chromium.org/683703002/diff/220001/athena/home/app_list_vi...
File athena/home/app_list_view_delegate.cc (right):

https://codereview.chromium.org/683703002/diff/220001/athena/home/app_list_vi...
athena/home/app_list_view_delegate.cc:163:
launcher_page_event_dispatcher_->ProgressChanged(progress);
remove? (not in athena)

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:215:
custom_page_contents_.clear();
launcher_page_event_dispatcher_.reset(); before this.

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:290: for
(std::vector<GURL>::const_iterator it = custom_launcher_page_urls.begin();
nit: I'd probably go for 

if (custom_launcher_page_urls.empty())
  return;

up here, rather than the !empty check below.

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:645:
launcher_page_event_dispatcher_->ProgressChanged(progress);
if (launcher_page_event_dispatcher_) ?

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/launcher_page_event_dispatcher.h (right):

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/launcher_page_event_dispatcher.h:8: #include
"base/basictypes.h"
nit: I think base/macros.h is preferred

you should have <string> too

https://codereview.chromium.org/683703002/diff/220001/ui/app_list/views/conte...
File ui/app_list/views/contents_view.h (right):

https://codereview.chromium.org/683703002/diff/220001/ui/app_list/views/conte...
ui/app_list/views/contents_view.h:168: AppListViewDelegate* view_delegate_;
needs to be initialized.

But I'd prefer exposing the AppListMainView::delegate_, and getting the ALVD
with

app_list_main_view_->delegate(); (or writing a helper function that does that).

Then you could remove the delegate argument from ContentsView::Init as well.

[Matt Giuca, 2014-11-06 06:09:37]

Since I've done 6 reviews for you in the past 24 hours, I think I'll take a
break for today and wait till you respond to Trent's comments. Will try to get
to this tomorrow.

(I assume this isn't required for M40.)

[tapted, 2014-11-06 11:54:18]

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.h:100: void
CustomLauncherPageAnimationChanged(double progress) override;
also, don't forget about applisttestviewdelegate and exampleapplistviewdelegate
orwhateveritis

[calamity, 2014-11-07 03:39:00]

Patchset #10 (id:240001) has been deleted

[calamity, 2014-11-07 03:43:30]

https://codereview.chromium.org/683703002/diff/220001/athena/home/app_list_vi...
File athena/home/app_list_view_delegate.cc (right):

https://codereview.chromium.org/683703002/diff/220001/athena/home/app_list_vi...
athena/home/app_list_view_delegate.cc:163:
launcher_page_event_dispatcher_->ProgressChanged(progress);
On 2014/11/06 06:05:45, tapted wrote:
> remove? (not in athena)

Done.

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:215:
custom_page_contents_.clear();
On 2014/11/06 06:05:45, tapted wrote:
> launcher_page_event_dispatcher_.reset(); before this.

Done.

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:290: for
(std::vector<GURL>::const_iterator it = custom_launcher_page_urls.begin();
On 2014/11/06 06:05:45, tapted wrote:
> nit: I'd probably go for 
> 
> if (custom_launcher_page_urls.empty())
>   return;
> 
> up here, rather than the !empty check below.

Done.

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:645:
launcher_page_event_dispatcher_->ProgressChanged(progress);
On 2014/11/06 06:05:45, tapted wrote:
> if (launcher_page_event_dispatcher_) ?

Done.

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.h:100: void
CustomLauncherPageAnimationChanged(double progress) override;
On 2014/11/06 11:54:18, tapted wrote:
> also, don't forget about applisttestviewdelegate and
exampleapplistviewdelegate
> orwhateveritis

Done.

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/launcher_page_event_dispatcher.h (right):

https://codereview.chromium.org/683703002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/launcher_page_event_dispatcher.h:8: #include
"base/basictypes.h"
On 2014/11/06 06:05:45, tapted wrote:
> nit: I think base/macros.h is preferred
> 
> you should have <string> too

Done.

https://codereview.chromium.org/683703002/diff/220001/ui/app_list/views/conte...
File ui/app_list/views/contents_view.h (right):

https://codereview.chromium.org/683703002/diff/220001/ui/app_list/views/conte...
ui/app_list/views/contents_view.h:168: AppListViewDelegate* view_delegate_;
On 2014/11/06 06:05:45, tapted wrote:
> needs to be initialized.
> 
> But I'd prefer exposing the AppListMainView::delegate_, and getting the ALVD
> with
> 
> app_list_main_view_->delegate(); (or writing a helper function that does
that).
> 
> Then you could remove the delegate argument from ContentsView::Init as well.

Done.

[tapted, 2014-11-07 05:34:56]

The release code lgtm, but I'm worried about the test... If you can make it
flake locally, I'd probably disable it in this CL, then fix it in a follow-up.

https://codereview.chromium.org/683703002/diff/280001/chrome/browser/apps/cus...
File chrome/browser/apps/custom_launcher_page_browsertest_views.cc (right):

https://codereview.chromium.org/683703002/diff/280001/chrome/browser/apps/cus...
chrome/browser/apps/custom_launcher_page_browsertest_views.cc:88:
contents_view->SetActivePage(contents_view->GetPageIndexForState(
You might run into a problem here on ChromeOS that could make this test flaky.
There's a run loop spun while doing listener.WaitUntilSatisfied(), during which
another window could take focus, which will dismiss the applist and destroy
contents_view, leading to an invalid access here.

You might also have a problem earlier, depending whether dismissing the app list
mid-animation ends up in the onPageExpanded state. I think in that case the test
will hang (on any platform).

Suff like this keeps coming up... I think we just need a
ScopedKeepAppListOpenForTest that works on all platforms.

[Matt Giuca, 2014-11-07 06:46:29]

Just have a few questions.

lgtm here, because I don't think the answer to those questions will change my
verdict, but please address them.

https://codereview.chromium.org/683703002/diff/280001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/custom_launcher_page/main.js
(right):

https://codereview.chromium.org/683703002/diff/280001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/custom_launcher_page/main.js:7:
chrome.test.sendMessage('onPageCollapsed');
Ah I think I understand now... onPageCollapsed and onPageExpanded are not part
of the API; they are test messages.

Can you rename them to progressAt0Percent and progressAt100Percent or something?
That would make it seem less like an actual API message (especially if we later
decide to add onPageCollapsed and onPageExpanded messages to the API; then it
could get really confusing).

https://codereview.chromium.org/683703002/diff/280001/ui/app_list/views/conte...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/683703002/diff/280001/ui/app_list/views/conte...
ui/app_list/views/contents_view.cc:236: void ContentsView::UpdatePageBounds() {
I'm confused... I thought you were moving PaginationModel up into the
AppListMainView. Is that coming in another CL or were you able to avoid that?

I'm also confused because I thought the reason you had to move PaginationModel
to AppListMainView is that you would otherwise have to do a lot of work to
detach and re-attach the ContentsView when the profiles switch. I don't see any
such work here (nor does it seem necessary, if all you are doing is sending a
message to the AppListViewDelegate).

So is there no longer any need for such big refactorings, or are they coming
later? (If the former, I am pleasantly surprised.)

[calamity, 2014-11-10 03:38:30]

https://codereview.chromium.org/683703002/diff/280001/chrome/browser/apps/cus...
File chrome/browser/apps/custom_launcher_page_browsertest_views.cc (right):

https://codereview.chromium.org/683703002/diff/280001/chrome/browser/apps/cus...
chrome/browser/apps/custom_launcher_page_browsertest_views.cc:88:
contents_view->SetActivePage(contents_view->GetPageIndexForState(
On 2014/11/07 05:34:56, tapted wrote:
> You might run into a problem here on ChromeOS that could make this test flaky.
> There's a run loop spun while doing listener.WaitUntilSatisfied(), during
which
> another window could take focus, which will dismiss the applist and destroy
> contents_view, leading to an invalid access here.
> 
> You might also have a problem earlier, depending whether dismissing the app
list
> mid-animation ends up in the onPageExpanded state. I think in that case the
test
> will hang (on any platform).
> 
> Suff like this keeps coming up... I think we just need a
> ScopedKeepAppListOpenForTest that works on all platforms.

Added and implemented the disable-app-list-on-blur command line flag for now.

https://codereview.chromium.org/683703002/diff/280001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/custom_launcher_page/main.js
(right):

https://codereview.chromium.org/683703002/diff/280001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/custom_launcher_page/main.js:7:
chrome.test.sendMessage('onPageCollapsed');
On 2014/11/07 06:46:29, Matt Giuca wrote:
> Ah I think I understand now... onPageCollapsed and onPageExpanded are not part
> of the API; they are test messages.
> 
> Can you rename them to progressAt0Percent and progressAt100Percent or
something?
> That would make it seem less like an actual API message (especially if we
later
> decide to add onPageCollapsed and onPageExpanded messages to the API; then it
> could get really confusing).

Done.

https://codereview.chromium.org/683703002/diff/280001/ui/app_list/views/conte...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/683703002/diff/280001/ui/app_list/views/conte...
ui/app_list/views/contents_view.cc:236: void ContentsView::UpdatePageBounds() {
On 2014/11/07 06:46:29, Matt Giuca wrote:
> I'm confused... I thought you were moving PaginationModel up into the
> AppListMainView. Is that coming in another CL or were you able to avoid that?
> 

Avoided for now. This doesn't support observing anything other than the custom
launcher page animation. If we ever want that, we can either expand this or
revisit the refactor.

[benwells, 2014-11-10 03:42:01]

extensions lgtm

[calamity, 2014-11-10 03:43:33]

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

[calamity, 2014-11-10 03:43:34]

TBR-ing oshima for ash and athena new AppListViewDelegate methods.

[calamity, 2014-11-10 04:21:01]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-11-10 04:21:43]

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

[commit-bot: I haz the power, 2014-11-10 04:42:34]

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

[commit-bot: I haz the power, 2014-11-10 04:42:35]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[commit-bot: I haz the power, 2014-11-10 04:42:51]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[calamity, 2014-11-10 05:25:21]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-11-10 05:25:42]

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

[commit-bot: I haz the power, 2014-11-10 06:24:25]

Committed patchset #13 (id:320001)

[commit-bot: I haz the power, 2014-11-10 06:25:00]

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