Add profile selector menu to app list.

Add profile selector menu to app list.

Adding a profile selector in the app list menu because currently the only way
to switch the profile is to go into the app list settings which is hard to
find. This CL covers both the mac and windows app list.

The app list switches the profile in place without respawning the app list.

* Added AppListModel::User struct to represent a User in the app list.
* Added a vector of Users to AppListModel.
* AppListMenu builds it model with respect to AppListModel::Users.
* The AppListViewDelegate profile is now switchable via SetProfilePath(). Calling this rebuilds the AppListModel which notifies the view to update.
* Removed custom menu items and views from app_list_menu_views.cc and deleted current_user_menu_item_view on mac.
* Changed ui assets that indicate the current user of the app list.
* Tweaked PaginationModel to handle an app list with 0 apps.
* Mac app list now sets a new delegate to switch the app list profile rather than
dismissing and reshowing.

BUG=254143, 258729, 289872, 290048
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224330

[calamity, 2013-07-26 04:53:28]

This is a first attempt at doing a profile switching menu in the app list.
Mostly looking for design feedback at this point but if you think that's fine,
style review etc would be good.

The menu will NOT get updated on profile change at the moment but that can be
fixed once https://codereview.chromium.org/19675010/ lands.

[koz (OOO until 15th September), 2013-07-26 06:30:18]

This design seems fine to me.

https://codereview.chromium.org/20656002/diff/2001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/20656002/diff/2001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.cc:58: avatar_menu_model_(
Better to pass this in.

https://codereview.chromium.org/20656002/diff/2001/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/20656002/diff/2001/ui/ui.gyp#newcode263
ui/ui.gyp:263: 'base/models/avatar_menu_item_model.h',
This file seems to be missing.

[calamity, 2013-07-26 09:13:29]

https://codereview.chromium.org/20656002/diff/2001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/20656002/diff/2001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.cc:58: avatar_menu_model_(
On 2013/07/26 06:30:18, koz wrote:
> Better to pass this in.

Done.

https://codereview.chromium.org/20656002/diff/2001/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/20656002/diff/2001/ui/ui.gyp#newcode263
ui/ui.gyp:263: 'base/models/avatar_menu_item_model.h',
On 2013/07/26 06:30:18, koz wrote:
> This file seems to be missing.

Done.

[koz (OOO until 15th September), 2013-07-29 00:34:34]

lgtm with nits

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
File ui/base/models/avatar_menu_item_model.h (right):

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
ui/base/models/avatar_menu_item_model.h:6: #define
UI_BASE_AVATAR_MENU_ITEM_MODEL_H_
should be UI_BASE_MODELS_AVATAR_MENU_ITEM_MODEL_H_

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
ui/base/models/avatar_menu_item_model.h:13: AvatarMenuItemModel(size_t
model_index, const gfx::Image& icon)
This all needs to be indented two spaces.

[tapted, 2013-07-29 01:59:09]

Can you post a screenshot on the bug? I'm interested to see what it looks like
with a submenu (assuming that's how it's drawn)... Maybe we don't want a
submenu, but just a list of profiles added to the start/end?

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/profiles/av...
File chrome/browser/profiles/avatar_menu_model.h (left):

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/profiles/av...
chrome/browser/profiles/avatar_menu_model.h:18: #include "ui/gfx/image/image.h"
nit: you probably keep a forward dec for GetManagedUserIcon(), and move the
#include to the .cc

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_controller_delegate.h (right):

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_controller_delegate.h:73: // Show the app
list for the profile specified by |index|.
nit: might be good to say what's being indexed (on first read, I'm not sure if
it's the ProfileInfoCache or the AvatarMenuModel or something inside the app
list)

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.h:40: // The delegate will
take ownership of the controller.
nit: the comment needs to be updated to say it takes ownership of
avatar_menu_model as well.

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.h:41:
AppListViewDelegate(AppListControllerDelegate* controller,
Aand... this should probably be updated to use scoped_ptr arguments. See
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/9Q2pc9jqxMQ/94AZw...

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:995: void
AppListController::ShowForProfileAtIndex(size_t index) {
Can this be moved to AppListServiceImpl?

https://codereview.chromium.org/20656002/diff/8001/ui/app_list/app_list_menu.h
File ui/app_list/app_list_menu.h (right):

https://codereview.chromium.org/20656002/diff/8001/ui/app_list/app_list_menu....
ui/app_list/app_list_menu.h:24: // |The received command id| - |SELECT_PROFILE|
is the profile index
nit: this comment's a bit unclear, since this seems a bit unconventional. e.g. I
think you need to say that this must be the last enum item, and describe why
it's done this way.

https://codereview.chromium.org/20656002/diff/8001/ui/app_list/app_list_view_...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/8001/ui/app_list/app_list_view_...
ui/app_list/app_list_view_delegate.h:98: virtual
std::vector<AvatarMenuItemModel> GetAvatarMenuItems() = 0;
nit: #include vector?

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
File ui/base/models/avatar_menu_item_model.h (right):

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
ui/base/models/avatar_menu_item_model.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit: Year can be 2013 (and no "(c)" )

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
ui/base/models/avatar_menu_item_model.h:12: struct AvatarMenuItemModel {
This guy should go in namespace ui - that seems to be the Way. Also you need to
declare it with UI_EXPORT, i.e.

class UI_EXPORT AvatarMenuItemModel { .. }

and #include "ui/base/ui_export.h"

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
ui/base/models/avatar_menu_item_model.h:19: ~AvatarMenuItemModel() {}
I think clang will warn at you for having a non-trivial destructor in a header
file... The constructor & destructor should be moved to a .cc

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
ui/base/models/avatar_menu_item_model.h:28: string16 name;
nit: #include "base/strings/string16.h" for string16

[tapted, 2013-07-29 02:37:06]

Once there are some green bots, I'd like to try it out on Mac, too, in case
there's a UI regression. git cl try -bmac_rel:app_list_unittests might pick up
something too.

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_service.h:58: virtual void
ShowForProfileAtIndex(size_t index) = 0;
btw - I don't think anything calls this via AppListService. You can probably
just declare it non-virtual in AppListServiceImpl. Then you don't have to fix
the compile errors for not defining this in, e.g, AppListServiceDisabled and
elsewhere :p

[calamity, 2013-07-30 08:42:34]

Rework + refactor.

The most "interesting" changes are AppListView::SetModel and
AppListControllerWin::ShowForProfile.

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/profiles/av...
File chrome/browser/profiles/avatar_menu_model.h (left):

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/profiles/av...
chrome/browser/profiles/avatar_menu_model.h:18: #include "ui/gfx/image/image.h"
On 2013/07/29 01:59:10, tapted wrote:
> nit: you probably keep a forward dec for GetManagedUserIcon(), and move the
> #include to the .cc

Done.

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_controller_delegate.h (right):

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_controller_delegate.h:73: // Show the app
list for the profile specified by |index|.
On 2013/07/29 01:59:10, tapted wrote:
> nit: might be good to say what's being indexed (on first read, I'm not sure if
> it's the ProfileInfoCache or the AvatarMenuModel or something inside the app
> list)

Done.

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_service.h:58: virtual void
ShowForProfileAtIndex(size_t index) = 0;
On 2013/07/29 02:37:06, tapted wrote:
> btw - I don't think anything calls this via AppListService. You can probably
> just declare it non-virtual in AppListServiceImpl. Then you don't have to fix
> the compile errors for not defining this in, e.g, AppListServiceDisabled and
> elsewhere :p

Done.

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.h:40: // The delegate will
take ownership of the controller.
On 2013/07/29 01:59:10, tapted wrote:
> nit: the comment needs to be updated to say it takes ownership of
> avatar_menu_model as well. 

N/A.

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/20656002/diff/8001/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:995: void
AppListController::ShowForProfileAtIndex(size_t index) {
On 2013/07/29 01:59:10, tapted wrote:
> Can this be moved to AppListServiceImpl?

Done.

https://codereview.chromium.org/20656002/diff/8001/ui/app_list/app_list_menu.h
File ui/app_list/app_list_menu.h (right):

https://codereview.chromium.org/20656002/diff/8001/ui/app_list/app_list_menu....
ui/app_list/app_list_menu.h:24: // |The received command id| - |SELECT_PROFILE|
is the profile index
On 2013/07/29 01:59:10, tapted wrote:
> nit: this comment's a bit unclear, since this seems a bit unconventional. e.g.
I
> think you need to say that this must be the last enum item, and describe why
> it's done this way.

Done.

https://codereview.chromium.org/20656002/diff/8001/ui/app_list/app_list_view_...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/8001/ui/app_list/app_list_view_...
ui/app_list/app_list_view_delegate.h:98: virtual
std::vector<AvatarMenuItemModel> GetAvatarMenuItems() = 0;
On 2013/07/29 01:59:10, tapted wrote:
> nit: #include vector?

Done.

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
File ui/base/models/avatar_menu_item_model.h (right):

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
ui/base/models/avatar_menu_item_model.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/07/29 01:59:10, tapted wrote:
> nit: Year can be 2013 (and no "(c)" )

Done.

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
ui/base/models/avatar_menu_item_model.h:6: #define
UI_BASE_AVATAR_MENU_ITEM_MODEL_H_
On 2013/07/29 00:34:34, koz wrote:
> should be UI_BASE_MODELS_AVATAR_MENU_ITEM_MODEL_H_

Done.

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
ui/base/models/avatar_menu_item_model.h:12: struct AvatarMenuItemModel {
On 2013/07/29 01:59:10, tapted wrote:
> This guy should go in namespace ui - that seems to be the Way. Also you need
to
> declare it with UI_EXPORT, i.e.
> 
> class UI_EXPORT AvatarMenuItemModel { .. }
> 
> and #include "ui/base/ui_export.h"

Done.

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
ui/base/models/avatar_menu_item_model.h:13: AvatarMenuItemModel(size_t
model_index, const gfx::Image& icon)
On 2013/07/29 00:34:34, koz wrote:
> This all needs to be indented two spaces.

Done.

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
ui/base/models/avatar_menu_item_model.h:19: ~AvatarMenuItemModel() {}
On 2013/07/29 01:59:10, tapted wrote:
> I think clang will warn at you for having a non-trivial destructor in a header
> file... The constructor & destructor should be moved to a .cc 

Done.

https://codereview.chromium.org/20656002/diff/8001/ui/base/models/avatar_menu...
ui/base/models/avatar_menu_item_model.h:28: string16 name;
On 2013/07/29 01:59:10, tapted wrote:
> nit: #include "base/strings/string16.h" for string16

Done.

[koz (OOO until 15th September), 2013-07-31 03:01:42]

As discussed, let's see how keeping the model alive in-place looks.

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_impl.h (right):

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_impl.h:55: // not been loaded, this
will load the profile and then show the app list.
This comment has become detached from its function :-)

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:543: if
(current_view_ && view_delegate_) {
current_view_ being not NULL implies view_delegate_ is not NULL so just check
current_view_ here.

[tapted, 2013-07-31 05:43:53]

We should think about adding tests too.....

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/profiles/a...
File chrome/browser/profiles/avatar_menu_model.cc (right):

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/profiles/a...
chrome/browser/profiles/avatar_menu_model.cc:294:
browser_->profile()->GetPath());
The old code had an `if (browser_)` - is that still needed?

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_info_util.cc (right):

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_info_util.cc:137: base::FilePath path =
cache->GetPathOfProfileAtIndex(i);
this variable seems unused

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_info_util.h (right):

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_info_util.h:54: const base::FilePath
active_profile_path);
nit: const-reference?

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_impl.h (right):

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_impl.h:54: // Shows the app list for
the profile at the given index. If the profile has
comment has become detached from the function. (and nit, perhaps: index ->
|index|)

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.cc:77: NOTREACHED();
maybe DCHECK_NE(std::string::npos, profile_index), assuming it "can't" happen

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.cc:96: model_));
`model_` with an underscore might be an error?

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.cc:98: } else {
Does something need to be done for the `else` case. E.g. clear items && notify?

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.cc:222:
STLDeleteElements(&model->avatar_menu_items());
it's a bit odd to expose a non-const reference to a data member this way.. I'd
prefer a local vector to be populated, which is then passed to a setter, like

AppListMode::SetMenuItems(const std::vector<..>& new_items) {
  STLDeleteElements(avatar_menu_items_);
  avatar_menu_items_ = new_items;
}

An efficient alternative might be the following, but I think the above is more
Chromey.

AppListMode::SetMenuItems(std::vector<..>* new_items) {
  new_items->swap(avatar_menu_items_);
  STLDeleteElements(*new_items);
}

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.h:43: Profile* profile);
nit: looks like the dec it will fit on one line

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/avatar_menu_bubble_view.h (right):

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/views/a...
chrome/browser/ui/views/avatar_menu_bubble_view.h:18: class Browser;
hm - what happened to the AvatarMenuModel forward-dec. It looks used..

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_menu.h
File ui/app_list/app_list_menu.h (right):

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_menu...
ui/app_list/app_list_menu.h:35: explicit AppListMenu(
nit: explicit not needed

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_menu...
ui/app_list/app_list_menu.h:37: std::vector<ui::AvatarMenuItemModel*>&
avatar_menu_items);
can't pass a non-const reference in Chrome

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_menu...
ui/app_list/app_list_menu.h:37: std::vector<ui::AvatarMenuItemModel*>&
avatar_menu_items);
I think it's convention in the current app list models to typedef a collection
of item-models (see e.g. app_list::SearchResult, app_list::AppListModel). You
could do (in AppListMenu), 

typedef std::vector<ui::AvatarMenuItemModel*> AvatarMenuModel;

referred to as AppListMenu::AvatarMenuModel

But there might be a better name for it (just AvatarModel, or AvatarsModel, or
ProfilesModel..). A typedef in AppListModel might make more sense, too (just
noticed it's used there..)

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_model.h
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_mode...
ui/app_list/app_list_model.h:17: namespace ui{
nit: space before {

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_mode...
ui/app_list/app_list_model.h:49: void SetCurrentUser(string16 user_name,
string16 user_email);
nit: const-reference args for strings

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_mode...
ui/app_list/app_list_model.h:56: string16 current_user_name() { return
current_user_name_; }
I've seen new code using base::string16 a lot more - I think there's an effort
to disallow access to string16 without using the base:: namespace.

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_mode...
ui/app_list/app_list_model.h:57: string16 current_user_email() { return
current_user_email_; }
Also, most app list code returns const-references for accessors like these. See,
e.g., app_list::SearchResult. The exception is if the "accessor" is a virtual
function.

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_mode...
ui/app_list/app_list_model.h:71: std::vector<ui::AvatarMenuItemModel*>
avatar_menu_items_;
a thought: you could also use a ScopedVector - not sure what the prevailing
style is though, or if it will conflict with the way it's currently used in
AvatarMenuModel. Perhaps due to changes there it would be better to investigate
orthogonal to this CL anyway.

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_view...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_view...
ui/app_list/app_list_view_delegate.h:13: class Profile;
src/ui shouldn't care about Profile* - I don't think this belongs here. E.g.
`git grep 'Profile\*'` in src/ui turns up no results.

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/views/app_lis...
File ui/app_list/views/app_list_menu_views.cc (right):

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/views/app_lis...
ui/app_list/views/app_list_menu_views.cc:127: AppListModel* app_list_model_;
nit: This should have a comment that it is a weak reference, and who the owner
is.

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/views/app_lis...
File ui/app_list/views/app_list_menu_views.h (right):

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/views/app_lis...
ui/app_list/views/app_list_menu_views.h:29: explicit
AppListMenuViews(AppListViewDelegate* delegate,
nit: explicit not needed

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/views/app_lis...
File ui/app_list/views/app_list_view.h (right):

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/views/app_lis...
ui/app_list/views/app_list_view.h:62: void SetModel(AppListModel* model);
this should take a scoped_ptr, to make the ownership transfer obvious

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/views/signin_...
File ui/app_list/views/signin_view.h (right):

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/views/signin_...
ui/app_list/views/signin_view.h:31: void SetDelegate(SigninDelegate* delegate) {
delegate_ = delegate; }
nit: SetDelegate -> set_delegate

https://codereview.chromium.org/20656002/diff/36001/ui/base/models/avatar_men...
File ui/base/models/avatar_menu_item_model.cc (right):

https://codereview.chromium.org/20656002/diff/36001/ui/base/models/avatar_men...
ui/base/models/avatar_menu_item_model.cc:5: #include
"ui/base/models/avatar_menu_item_model.h"
nit: blank line after a .cc's "own" header

https://codereview.chromium.org/20656002/diff/36001/ui/base/models/avatar_men...
File ui/base/models/avatar_menu_item_model.h (right):

https://codereview.chromium.org/20656002/diff/36001/ui/base/models/avatar_men...
ui/base/models/avatar_menu_item_model.h:16: struct UI_EXPORT AvatarMenuItemModel
{
Feeling around a bit more..... there's not really any precedent for a ui model,
nor an app list model, to be a struct. Convention is class + accessors. Any
thoughts?

https://codereview.chromium.org/20656002/diff/36001/ui/base/models/avatar_men...
ui/base/models/avatar_menu_item_model.h:17: AvatarMenuItemModel(size_t
model_index, const gfx::Image& icon);
Looks like you got carried away with the indent here :p. 4 spaces -> 2 spaces.

https://codereview.chromium.org/20656002/diff/36001/ui/base/models/avatar_men...
ui/base/models/avatar_menu_item_model.h:27: string16 name;
base::string16 for these I think

[benwells, 2013-07-31 05:47:13]

drive by. I've only really looked at the interface changes. Please loop xiyuan
in on this once the Sydney folks reach consensus.

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_delegate.h (right):

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_delegate.h:73: // Show the app
list for the profile specified by |index|.
Index into what? It is alright here layer-wise to have this be an index to
profile info cache, but it would be better to be a path (less coupled to profile
info cache).

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.cc:66: profile_ = profile;
Shouldn't this rebuild models and suchlike? Feels strange that it doesn't,
although the name is accurate.

https://codereview.chromium.org/20656002/diff/36001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.cc:69: void
AppListViewDelegate::SetModel(app_list::AppListModel* model) {
This name is really bad. It rebuilds the whole model, not just setting it.

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_view...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_view...
ui/app_list/app_list_view_delegate.h:35: virtual void SetProfile(Profile*
profile) = 0;
I don't think we want to add profile here. Do you need this?

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_view...
ui/app_list/app_list_view_delegate.h:94: // ProfileInfoCache.
ProfileInfoCache is a chrome concept. Having indexes into it in this API is not
right.

I guess you could just change the comment to be '.. in the menu built by the
delegate in RebuildAvatarMenuItems' but I think it would be better to just make
this a path instead of an index.

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/app_list_view...
ui/app_list/app_list_view_delegate.h:97: // Rebuilds the avatar menu items from
the profile info cache.
Again, profile info cache is a chrome concept and shouldn't be referred to from
here.

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/views/signin_...
File ui/app_list/views/signin_view.h (right):

https://codereview.chromium.org/20656002/diff/36001/ui/app_list/views/signin_...
ui/app_list/views/signin_view.h:31: void SetDelegate(SigninDelegate* delegate) {
delegate_ = delegate; }
use set_delegate for trivial setters like this.

[calamity, 2013-08-01 08:35:45]

Model now changes in-place rather than creating a new one. AppListView::SetModel
has been removed and the repopulation work is pushed into
AppListViewDelegate::SetProfile.

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/pro...
File chrome/browser/profiles/avatar_menu_model.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/pro...
chrome/browser/profiles/avatar_menu_model.cc:294:
browser_->profile()->GetPath());
On 2013/07/31 05:43:53, tapted wrote:
> The old code had an `if (browser_)` - is that still needed?

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/pro...
File chrome/browser/profiles/profile_info_util.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/pro...
chrome/browser/profiles/profile_info_util.cc:137: base::FilePath path =
cache->GetPathOfProfileAtIndex(i);
On 2013/07/31 05:43:53, tapted wrote:
> this variable seems unused

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/pro...
File chrome/browser/profiles/profile_info_util.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/pro...
chrome/browser/profiles/profile_info_util.h:54: const base::FilePath
active_profile_path);
On 2013/07/31 05:43:53, tapted wrote:
> nit: const-reference?

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
File chrome/browser/ui/app_list/app_list_controller_delegate.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_controller_delegate.h:73: // Show the app
list for the profile specified by |index|.
On 2013/07/31 05:47:13, benwells wrote:
> Index into what? It is alright here layer-wise to have this be an index to
> profile info cache, but it would be better to be a path (less coupled to
profile
> info cache).

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
File chrome/browser/ui/app_list/app_list_service_impl.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_service_impl.h:54: // Shows the app list for
the profile at the given index. If the profile has
On 2013/07/31 05:43:53, tapted wrote:
> comment has become detached from the function. (and nit, perhaps: index ->
> |index|)

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:66: profile_ = profile;
On 2013/07/31 05:47:13, benwells wrote:
> Shouldn't this rebuild models and suchlike? Feels strange that it doesn't,
> although the name is accurate.

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:69: void
AppListViewDelegate::SetModel(app_list::AppListModel* model) {
On 2013/07/31 05:47:13, benwells wrote:
> This name is really bad. It rebuilds the whole model, not just setting it.

Done. Switched to InitModel.

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:77: NOTREACHED();
On 2013/07/31 05:43:53, tapted wrote:
> maybe DCHECK_NE(std::string::npos, profile_index), assuming it "can't" happen

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:96: model_));
On 2013/07/31 05:43:53, tapted wrote:
> `model_` with an underscore might be an error?

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:98: } else {
On 2013/07/31 05:43:53, tapted wrote:
> Does something need to be done for the `else` case. E.g. clear items &&
notify?

I removed the else case because it doesn't seem to be used anywhere...

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:222:
STLDeleteElements(&model->avatar_menu_items());
On 2013/07/31 05:43:53, tapted wrote:
> it's a bit odd to expose a non-const reference to a data member this way.. I'd
> prefer a local vector to be populated, which is then passed to a setter, like
> 
> AppListMode::SetMenuItems(const std::vector<..>& new_items) {
>   STLDeleteElements(avatar_menu_items_);
>   avatar_menu_items_ = new_items;
> }
> 
> An efficient alternative might be the following, but I think the above is more
> Chromey.
> 
> AppListMode::SetMenuItems(std::vector<..>* new_items) {
>   new_items->swap(avatar_menu_items_);
>   STLDeleteElements(*new_items);
> }
> 

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_view_delegate.h:43: Profile* profile);
On 2013/07/31 05:43:53, tapted wrote:
> nit: looks like the dec it will fit on one line

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:543: if
(current_view_ && view_delegate_) {
On 2013/07/31 03:01:42, koz wrote:
> current_view_ being not NULL implies view_delegate_ is not NULL so just check
> current_view_ here.

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
File chrome/browser/ui/views/avatar_menu_bubble_view.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/chrome/browser/ui/...
chrome/browser/ui/views/avatar_menu_bubble_view.h:18: class Browser;
On 2013/07/31 05:43:53, tapted wrote:
> hm - what happened to the AvatarMenuModel forward-dec. It looks used..

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
File ui/app_list/app_list_menu.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
ui/app_list/app_list_menu.h:35: explicit AppListMenu(
On 2013/07/31 05:43:53, tapted wrote:
> nit: explicit not needed

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
ui/app_list/app_list_menu.h:37: std::vector<ui::AvatarMenuItemModel*>&
avatar_menu_items);
On 2013/07/31 05:43:53, tapted wrote:
> can't pass a non-const reference in Chrome

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
ui/app_list/app_list_menu.h:37: std::vector<ui::AvatarMenuItemModel*>&
avatar_menu_items);
On 2013/07/31 05:43:53, tapted wrote:
> I think it's convention in the current app list models to typedef a collection
> of item-models (see e.g. app_list::SearchResult, app_list::AppListModel). You
> could do (in AppListMenu), 
> 
> typedef std::vector<ui::AvatarMenuItemModel*> AvatarMenuModel;
> 
> referred to as AppListMenu::AvatarMenuModel
> 
> But there might be a better name for it (just AvatarModel, or AvatarsModel, or
> ProfilesModel..). A typedef in AppListModel might make more sense, too (just
> noticed it's used there..)

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
File ui/app_list/app_list_model.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
ui/app_list/app_list_model.h:17: namespace ui{
On 2013/07/31 05:43:53, tapted wrote:
> nit: space before {

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
ui/app_list/app_list_model.h:49: void SetCurrentUser(string16 user_name,
string16 user_email);
On 2013/07/31 05:43:53, tapted wrote:
> nit: const-reference args for strings

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
ui/app_list/app_list_model.h:56: string16 current_user_name() { return
current_user_name_; }
On 2013/07/31 05:43:53, tapted wrote:
> I've seen new code using base::string16 a lot more - I think there's an effort
> to disallow access to string16 without using the base:: namespace.

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
ui/app_list/app_list_model.h:57: string16 current_user_email() { return
current_user_email_; }
On 2013/07/31 05:43:53, tapted wrote:
> Also, most app list code returns const-references for accessors like these.
See,
> e.g., app_list::SearchResult. The exception is if the "accessor" is a virtual
> function.

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
ui/app_list/app_list_model.h:71: std::vector<ui::AvatarMenuItemModel*>
avatar_menu_items_;
On 2013/07/31 05:43:53, tapted wrote:
> a thought: you could also use a ScopedVector - not sure what the prevailing
> style is though, or if it will conflict with the way it's currently used in
> AvatarMenuModel. Perhaps due to changes there it would be better to
investigate
> orthogonal to this CL anyway.

Also might be worth using a ui::ListModel since it has observer support. I'll
see if it's at all useful to do so...

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
File ui/app_list/app_list_view_delegate.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
ui/app_list/app_list_view_delegate.h:13: class Profile;
On 2013/07/31 05:43:53, tapted wrote:
> src/ui shouldn't care about Profile* - I don't think this belongs here. E.g.
> `git grep 'Profile\*'` in src/ui turns up no results.

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
ui/app_list/app_list_view_delegate.h:35: virtual void SetProfile(Profile*
profile) = 0;
On 2013/07/31 05:47:13, benwells wrote:
> I don't think we want to add profile here. Do you need this?

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
ui/app_list/app_list_view_delegate.h:94: // ProfileInfoCache.
On 2013/07/31 05:47:13, benwells wrote:
> ProfileInfoCache is a chrome concept. Having indexes into it in this API is
not
> right.
> 
> I guess you could just change the comment to be '.. in the menu built by the
> delegate in RebuildAvatarMenuItems' but I think it would be better to just
make
> this a path instead of an index.

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/app_li...
ui/app_list/app_list_view_delegate.h:97: // Rebuilds the avatar menu items from
the profile info cache.
On 2013/07/31 05:47:13, benwells wrote:
> Again, profile info cache is a chrome concept and shouldn't be referred to
from
> here.

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/views/...
File ui/app_list/views/app_list_menu_views.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/views/...
ui/app_list/views/app_list_menu_views.cc:127: AppListModel* app_list_model_;
On 2013/07/31 05:43:53, tapted wrote:
> nit: This should have a comment that it is a weak reference, and who the owner
> is.

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/views/...
File ui/app_list/views/app_list_menu_views.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/views/...
ui/app_list/views/app_list_menu_views.h:29: explicit
AppListMenuViews(AppListViewDelegate* delegate,
On 2013/07/31 05:43:53, tapted wrote:
> nit: explicit not needed

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/views/...
File ui/app_list/views/app_list_view.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/views/...
ui/app_list/views/app_list_view.h:62: void SetModel(AppListModel* model);
On 2013/07/31 05:43:53, tapted wrote:
> this should take a scoped_ptr, to make the ownership transfer obvious

Removed.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/views/...
File ui/app_list/views/signin_view.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/app_list/views/...
ui/app_list/views/signin_view.h:31: void SetDelegate(SigninDelegate* delegate) {
delegate_ = delegate; }
On 2013/07/31 05:47:13, benwells wrote:
> use set_delegate for trivial setters like this.
Removed.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/base/models/ava...
File ui/base/models/avatar_menu_item_model.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/base/models/ava...
ui/base/models/avatar_menu_item_model.cc:5: #include
"ui/base/models/avatar_menu_item_model.h"
On 2013/07/31 05:43:53, tapted wrote:
> nit: blank line after a .cc's "own" header

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/base/models/ava...
File ui/base/models/avatar_menu_item_model.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/base/models/ava...
ui/base/models/avatar_menu_item_model.h:16: struct UI_EXPORT AvatarMenuItemModel
{
On 2013/07/31 05:43:53, tapted wrote:
> Feeling around a bit more..... there's not really any precedent for a ui
model,
> nor an app list model, to be a struct. Convention is class + accessors. Any
> thoughts?

Hmm. I don't think this even belongs in this folder... The models here are
highly abstracted UI components rather than actual ui.

I think it's worth moving this to its own folder ui/base/profile_selector given
that we will be needing more shared assets for the new profile selector.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/base/models/ava...
ui/base/models/avatar_menu_item_model.h:17: AvatarMenuItemModel(size_t
model_index, const gfx::Image& icon);
On 2013/07/31 05:43:53, tapted wrote:
> Looks like you got carried away with the indent here :p. 4 spaces -> 2 spaces.

Done.

https://chromiumcodereview.appspot.com/20656002/diff/36001/ui/base/models/ava...
ui/base/models/avatar_menu_item_model.h:27: string16 name;
On 2013/07/31 05:43:53, tapted wrote:
> base::string16 for these I think

Done.

[tapted, 2013-08-01 09:25:55]

An initial batch, will do another pass.

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
File chrome/browser/ui/app_list/app_list_controller_delegate.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_controller_delegate.h:78: virtual void
ShowForProfileByPath(base::FilePath profile_path) = 0;
const-reference?

And.. this doesn't need to be pure virtual, and would then avoid duplicating
identical logic for win and mac [which currently won't compile because of this].
But it would involve #including app_list_service_impl.h in
c/b/ui/app_list/app_list_controller_delegate.cc . This would be my preference,
but you might get other opinions.

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
File chrome/browser/ui/app_list/app_list_service_impl.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_service_impl.h:42: void
ShowForProfileByPath(base::FilePath profile_path);
nit: const-reference

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_view_delegate.cc:225:
controller_->ShowForProfileByPath(profile_path);
Instead of this, could you just do what manage_profile_handler.cc does, and

  SetProfilePath(profile_path);
  Show();

Then you don't need to have AppListServiceImpl::ShowForProfileByPath at all --
it's already written.

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_view_delegate.cc:233:
DCHECK_NE(profile_index, std::string::npos);
nit: the "more constant" parameter should come first in these.

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_view_delegate.h:42: virtual void
SetProfileByPath(base::FilePath profile_path) OVERRIDE;
nit: const-reference

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_view_delegate.h:75: app_list::AppListModel*
model_;
Should have an ownership comment

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
File chrome/browser/ui/app_list/apps_model_builder.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
chrome/browser/ui/app_list/apps_model_builder.cc:178: if (tracker_) {
nit: doesn't need curlies

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
chrome/browser/ui/app_list/apps_model_builder.cc:181: Build();
You may find doing it this way to be surprisingly inefficient... Currently, the
model is built for a profile while there is nothing observing it, and then the
UI code build objects for the entire model. It then listens for gradual changes
on it -- stuff installing or syncing.

I.. think this will change that, so the view code is observing it while
PopulateApps runs, meaning that ui::ListModel will notify observers for each
thing added.

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
File chrome/browser/ui/app_list/chrome_signin_delegate.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
chrome/browser/ui/app_list/chrome_signin_delegate.h:34: virtual void
SetProfileByPath(base::FilePath profile_path) OVERRIDE;
const-reference?

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/123001/chrome/browser/ui...
chrome/browser/ui/ash/app_list/app_list_controller_ash.h:33: virtual void
ShowForProfileAtIndex(size_t index) OVERRIDE;
This one is still using indexes..

https://chromiumcodereview.appspot.com/20656002/diff/123001/ui/app_list/app_l...
File ui/app_list/app_list_view_delegate.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/123001/ui/app_list/app_l...
ui/app_list/app_list_view_delegate.h:33: virtual void
SetProfileByPath(base::FilePath profile_path) = 0;
const-reference?

https://chromiumcodereview.appspot.com/20656002/diff/123001/ui/app_list/pagin...
File ui/app_list/pagination_model.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/123001/ui/app_list/pagin...
ui/app_list/pagination_model.cc:32: if (selected_page_ < 0) {
don't need curlies?

[tapted, 2013-08-01 23:09:01]

It looks like you deleted the patchset with my comments - I didn't think they
were that bad :'(

[koz (OOO until 15th September), 2013-08-02 00:59:08]

On 2013/08/01 23:09:01, tapted wrote:
> It looks like you deleted the patchset with my comments - I didn't think they
> were that bad :'(

> You may find doing it this way to be surprisingly inefficient...

I think the right answer to that (if it does turn out to be really inefficient)
is to have a different event like "ReplaceAllItems(std::vector<Item> items);"
which handles that case.

[tapted, 2013-08-02 01:23:43]

On 2013/08/02 00:59:08, koz wrote:
> On 2013/08/01 23:09:01, tapted wrote:
> > It looks like you deleted the patchset with my comments - I didn't think
they
> > were that bad :'(
> 
> > You may find doing it this way to be surprisingly inefficient...
> 
> I think the right answer to that (if it does turn out to be really
inefficient)
> is to have a different event like "ReplaceAllItems(std::vector<Item> items);"
> which handles that case.

Yep - it would need an update to ui::ListModel. It's pretty strange that the
ListItemsAdded observer function is passed a `count` even though there is no way
for it to be called with anything other than count=1 the way ListModel is
currently written.... Only `DeleteAll` ever uses a count != 1. Hopefully the
change doesn't break any assumptions that existing observers currently make.

I know OSX won't handle ListItemsAdded(index, 1) being called more than ~40
times without its autorelease pool being drained (which usually needs a visit
back to the runloop 'root'). I tried this in unit tests and I ran out of memory
-- there's just too many UI tasks getting queued up.

[koz (OOO until 15th September), 2013-08-02 01:51:15]

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/pr...
File chrome/browser/profiles/profile_info_util.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/pr...
chrome/browser/profiles/profile_info_util.h:52:
std::vector<ui::AvatarMenuItemModel*>& items,
Use a pointer here - we only use const references in Chrome.

Also, out params go last in the parameter list.

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/ui...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_view_delegate.cc:94: DCHECK(model);
DCHECK(!model_), too.

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/ui...
File chrome/browser/ui/app_list/apps_model_builder.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/ui...
chrome/browser/ui/app_list/apps_model_builder.cc:178: if (tracker_) {
nit: no curlies

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/ui...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/ui...
chrome/browser/ui/ash/app_list/app_list_controller_ash.h:33: virtual void
ShowForProfileAtIndex(size_t index) OVERRIDE;
This needs to be changed to SetProfileByPath().

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/pagin...
File ui/app_list/pagination_model.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/pagin...
ui/app_list/pagination_model.cc:32: if (selected_page_ < 0) {
nit: one line ifs shouldn't have curlies.

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/views...
File ui/app_list/views/app_list_main_view.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/views...
ui/app_list/views/app_list_main_view.h:51: void SetModel(AppListModel* model);
This is no longer needed, right?

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/views...
File ui/app_list/views/app_list_view.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/views...
ui/app_list/views/app_list_view.cc:178:
delegate_->UpdateModelWithCurrentProfiles();
This would be better if the delegate listened to the signin delegate itself as
it would then have 100% responsibility for knowing when changes to the profile
occur.

If you do that then AppListView can just listen to the model and update itself
when the profile changes and this function just becomes
OnAppListodelProfilesChanged() below.

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/views...
File ui/app_list/views/search_box_view.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/views...
ui/app_list/views/search_box_view.h:43: void SetModel(AppListModel* model);
Can we pass this in through the constructor now, and require it to be non-NULL?

[koz (OOO until 15th September), 2013-08-02 01:53:59]

On 2013/08/02 01:23:43, tapted wrote:
> On 2013/08/02 00:59:08, koz wrote:
> > On 2013/08/01 23:09:01, tapted wrote:
> > > It looks like you deleted the patchset with my comments - I didn't think
> they
> > > were that bad :'(
> > 
> > > You may find doing it this way to be surprisingly inefficient...
> > 
> > I think the right answer to that (if it does turn out to be really
> inefficient)
> > is to have a different event like "ReplaceAllItems(std::vector<Item>
items);"
> > which handles that case.
> 
> Yep - it would need an update to ui::ListModel. It's pretty strange that the
> ListItemsAdded observer function is passed a `count` even though there is no
way
> for it to be called with anything other than count=1 the way ListModel is
> currently written.... Only `DeleteAll` ever uses a count != 1. Hopefully the
> change doesn't break any assumptions that existing observers currently make.
> 
> I know OSX won't handle ListItemsAdded(index, 1) being called more than ~40
> times without its autorelease pool being drained (which usually needs a visit
> back to the runloop 'root'). I tried this in unit tests and I ran out of
memory
> -- there's just too many UI tasks getting queued up.

::mind blown:: That is crazy!

[calamity, 2013-08-02 09:59:53]

Did tapted's comments too, despite deleting the patchset. Oops.

-sign in status now in the model
-list_model now provides an AddAll
-install tracker is updated when the profile is changed
-removed DCHECK that was causing a crash on profile deletion

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/pr...
File chrome/browser/profiles/profile_info_util.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/pr...
chrome/browser/profiles/profile_info_util.h:52:
std::vector<ui::AvatarMenuItemModel*>& items,
On 2013/08/02 01:51:16, koz wrote:
> Use a pointer here - we only use const references in Chrome.
> 
> Also, out params go last in the parameter list.

Done.

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/ui...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_view_delegate.cc:94: DCHECK(model);
On 2013/08/02 01:51:16, koz wrote:
> DCHECK(!model_), too.

Done.

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/ui...
File chrome/browser/ui/app_list/apps_model_builder.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/ui...
chrome/browser/ui/app_list/apps_model_builder.cc:178: if (tracker_) {
On 2013/08/02 01:51:16, koz wrote:
> nit: no curlies

Done.

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/ui...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/chrome/browser/ui...
chrome/browser/ui/ash/app_list/app_list_controller_ash.h:33: virtual void
ShowForProfileAtIndex(size_t index) OVERRIDE;
On 2013/08/02 01:51:16, koz wrote:
> This needs to be changed to SetProfileByPath().

Done.

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/pagin...
File ui/app_list/pagination_model.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/pagin...
ui/app_list/pagination_model.cc:32: if (selected_page_ < 0) {
On 2013/08/02 01:51:16, koz wrote:
> nit: one line ifs shouldn't have curlies.

Done.

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/views...
File ui/app_list/views/app_list_main_view.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/views...
ui/app_list/views/app_list_main_view.h:51: void SetModel(AppListModel* model);
On 2013/08/02 01:51:16, koz wrote:
> This is no longer needed, right?

Done.

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/views...
File ui/app_list/views/app_list_view.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/views...
ui/app_list/views/app_list_view.cc:178:
delegate_->UpdateModelWithCurrentProfiles();
On 2013/08/02 01:51:16, koz wrote:
> This would be better if the delegate listened to the signin delegate itself as
> it would then have 100% responsibility for knowing when changes to the profile
> occur.
> 
> If you do that then AppListView can just listen to the model and update itself
> when the profile changes and this function just becomes
> OnAppListodelProfilesChanged() below.

Done.

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/views...
File ui/app_list/views/search_box_view.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/143001/ui/app_list/views...
ui/app_list/views/search_box_view.h:43: void SetModel(AppListModel* model);
On 2013/08/02 01:51:16, koz wrote:
> Can we pass this in through the constructor now, and require it to be
non-NULL?

Done.

[koz (OOO until 15th September), 2013-08-05 01:09:11]

Code's looking good. How about some tests?

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/pagin...
File ui/app_list/pagination_model.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/pagin...
ui/app_list/pagination_model.cc:34: if (selected_page_ >= total_pages_) {
nit: no curlies

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/pagin...
ui/app_list/pagination_model.cc:86: DCHECK(page == 0 || (page > 0 && page <
total_pages_));
This change seems unnecessary.

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/signi...
File ui/app_list/signin_delegate.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/signi...
ui/app_list/signin_delegate.h:37: virtual void SetProfileByPath(const
base::FilePath& profile_path) = 0;
This should take a Profile*, seeing as implementors simply look up a Profile*
from profile_path anyway.

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/views...
File ui/app_list/views/app_list_view.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/views...
ui/app_list/views/app_list_view.h:92: virtual void
OnAppListModelSigninStatusChanged() OVERRIDE;
nit: This is also overidden from AppListModelObserver so put it beneath that
comment below.

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/views...
File ui/app_list/views/signin_view.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/views...
ui/app_list/views/signin_view.h:31: void set_delegate(SigninDelegate* delegate)
{ delegate_ = delegate; }
Is this used?

[tapted, 2013-08-05 03:01:38]

This CL is getting a bit wild -- it is very hard to review. Tests will make it
`wilder`, so.... I think it needs to be split up.

I think the refactoring that decouples AppListMenu from AppListViewDelegate
could be made separate.

And I think the ui::ListModel changes need to be a separate CL. You can cite
this CL as the motivator. Note that you definitely need to update
list_model_unittest.cc. It might be worth checking with ui/base owners first,
too, in case there is a good reason for there being no AddAll.

I'd like to see some green mac bots before I review again, too -- there might be
some changes required there that influence the design.

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/pr...
File chrome/browser/profiles/profile_info_util.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/pr...
chrome/browser/profiles/profile_info_util.cc:124: // old avatar menu uses
resized-small images
nit: This comment should be turned into a sentence, since it's moving.

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/pr...
File chrome/browser/profiles/profile_info_util.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/pr...
chrome/browser/profiles/profile_info_util.h:16: namespace base{
nit: space before {

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/ui...
File chrome/browser/ui/app_list/app_list_service_impl.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_service_impl.h:30: public
ProfileInfoCacheObserver{
space before {

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_service_impl.h:39: virtual void Show()
OVERRIDE;
nit: there should be a comment saying these are overloaded from AppListService.
It would be nice to keep all the functions grouped together - I'll see if I can
find the place in this patch which convinced you to make them public...

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/ui...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_view_delegate.cc:262: return;
nit: blank line after early return

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_view_delegate.cc:266: // Populate the
profile menu items;
semicolon should be a .

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/ui...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_view_delegate.h:42: public
content::NotificationObserver  {
too many spaces before {

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_view_delegate.h:47: virtual void
SetProfileByPath(const base::FilePath& profile_path) OVERRIDE;
nit: should say where it's overridden from

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_view_delegate.h:79: // Overriden from
app_list::SigninDelegateObserver:
Overriden -> overridden (spelling)

https://chromiumcodereview.appspot.com/20656002/diff/207001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_view_delegate.h:82: //
content::NotificationObserver
nit: Overridden from.., to be consistent

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
File ui/app_list/app_list_menu.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
ui/app_list/app_list_menu.h:8: #include <vector>
I guess the header doesn't need this any more, since it's now a typedef?

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
ui/app_list/app_list_menu.h:38: const app_list::AppListModel::ProfileMenuItems&
profile_menu_items);
nit: doesn't need app_list::

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
ui/app_list/app_list_menu.h:56: const app_list::AppListModel::ProfileMenuItems&
profile_menu_items_;
I don't usually see things declared this way in Chrome.. Maybe a weak pointer?
Either way, it should have a comment saying who owns it. The app_list:: is also
redundant.

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
File ui/app_list/app_list_model.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
ui/app_list/app_list_model.h:61: const string16& current_user_name() const {
return current_user_name_; }
nit: base:: prefix on string16

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
ui/app_list/app_list_model.h:64: const ProfileMenuItems& profile_menu_items() {
nit: const qualifier on member function?

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
File ui/app_list/app_list_model_observer.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
ui/app_list/app_list_model_observer.h:18: // Invoked when AppListModel's current
user or avatar menu items have changed.
This comment doesn't look right.

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
File ui/app_list/app_list_view_delegate.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
ui/app_list/app_list_view_delegate.h:35: // Invoked to initialize the model that
AppListView uses. This binds the given
Incomplete sentence: "This binds the given.."

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
ui/app_list/app_list_view_delegate.h:94: // Sets the |model|'s current user to
this delegate's profile and rebuilds the
|model| has no meaning here -- perhaps refer to it by the appropriate class name

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
ui/app_list/app_list_view_delegate.h:96: virtual void
UpdateModelWithCurrentProfiles() = 0;
Probably shouldn't be plural Profiles. Maybe call this
UpdateMenuWithCurrentProfile ? (or UpdateMenuModelWithCurrentProfile?).

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/views...
File ui/app_list/views/search_box_view.cc (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/views...
ui/app_list/views/search_box_view.cc:70: if (model_)
This `if` should be redundant now -- model_ is always non-NULL

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/base/models/li...
File ui/base/models/list_model.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/base/models/li...
ui/base/models/list_model.h:40: size_t count = item_count();
I think this needs either a DCHECK(!new_items.empty()), or an early return.

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/base/models/li...
ui/base/models/list_model.h:46: NotifyItemsAdded(count, items_.size());
items_.size() isn't right -- it should be new_items_released.size(). Or
items_.size() - count. (Maybe DCHECK_EQ for those two anyway).

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/base/profile_s...
File ui/base/profile_selector/avatar_menu_item_model.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/base/profile_s...
ui/base/profile_selector/avatar_menu_item_model.h:5: #ifndef
UI_BASE_MODELS_AVATAR_MENU_ITEM_MODEL_H_
This include guard needs updating

[koz (OOO until 15th September), 2013-08-05 03:52:13]

Yes, splitting this up would be great.

Another potentially isolated change could be moving the profile related delegate
methods into the AppListModel.

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
File ui/app_list/app_list_menu.h (right):

https://chromiumcodereview.appspot.com/20656002/diff/207001/ui/app_list/app_l...
ui/app_list/app_list_menu.h:56: const app_list::AppListModel::ProfileMenuItems&
profile_menu_items_;
On 2013/08/05 03:01:39, tapted wrote:
> I don't usually see things declared this way in Chrome.. Maybe a weak pointer?
> Either way, it should have a comment saying who owns it. The app_list:: is
also
> redundant.

Right - this should probably be a value.

[benwells, 2013-08-06 01:37:28]

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/signin_deleg...
File ui/app_list/signin_delegate.h (right):

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/signin_deleg...
ui/app_list/signin_delegate.h:37: virtual void SetProfileByPath(const
base::FilePath& profile_path) = 0;
On 2013/08/05 01:09:11, koz wrote:
> This should take a Profile*, seeing as implementors simply look up a Profile*
> from profile_path anyway.

I don't think ui/app_list shouldn't depend on Profile.

[koz (OOO until 15th September), 2013-08-06 04:51:33]

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/signin_deleg...
File ui/app_list/signin_delegate.h (right):

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/signin_deleg...
ui/app_list/signin_delegate.h:37: virtual void SetProfileByPath(const
base::FilePath& profile_path) = 0;
On 2013/08/06 01:37:29, benwells wrote:
> On 2013/08/05 01:09:11, koz wrote:
> > This should take a Profile*, seeing as implementors simply look up a
Profile*
> > from profile_path anyway.
> 
> I don't think ui/app_list shouldn't depend on Profile.

Good point. This method should probably not be in this interface at all. It
would go better in the implementation (ie: ChromeSigninDelegate) which deals
with Profile*s.

[calamity, 2013-08-08 04:52:12]

+xiyuan

This cl has been split into https://codereview.chromium.org/22268009/ ,
https://codereview.chromium.org/22339004/ , and
https://codereview.chromium.org/22354002/ .

This CL should now only contain the work to add an app list profile selector to
the app list menu.

The profile selector items are now inline with the app list menu rather than
being a submenu.

https://codereview.chromium.org/20656002/diff/207001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_info_util.cc (right):

https://codereview.chromium.org/20656002/diff/207001/chrome/browser/profiles/...
chrome/browser/profiles/profile_info_util.cc:124: // old avatar menu uses
resized-small images
On 2013/08/05 03:01:39, tapted wrote:
> nit: This comment should be turned into a sentence, since it's moving.

Done.

https://codereview.chromium.org/20656002/diff/207001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_impl.h (right):

https://codereview.chromium.org/20656002/diff/207001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_impl.h:30: public
ProfileInfoCacheObserver{
On 2013/08/05 03:01:39, tapted wrote:
> space before {

Done.

https://codereview.chromium.org/20656002/diff/207001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_impl.h:39: virtual void Show()
OVERRIDE;
On 2013/08/05 03:01:39, tapted wrote:
> nit: there should be a comment saying these are overloaded from
AppListService.
> It would be nice to keep all the functions grouped together - I'll see if I
can
> find the place in this patch which convinced you to make them public...

AppListControllerDelegateWin calls Show and SetProfilePath.

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

https://codereview.chromium.org/20656002/diff/207001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:262: return;
On 2013/08/05 03:01:39, tapted wrote:
> nit: blank line after early return

Done.

https://codereview.chromium.org/20656002/diff/207001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:266: // Populate the
profile menu items;
On 2013/08/05 03:01:39, tapted wrote:
> semicolon should be a .

Done.

https://codereview.chromium.org/20656002/diff/207001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/207001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.h:42: public
content::NotificationObserver  {
On 2013/08/05 03:01:39, tapted wrote:
> too many spaces before {

Done.

https://codereview.chromium.org/20656002/diff/207001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.h:47: virtual void
SetProfileByPath(const base::FilePath& profile_path) OVERRIDE;
On 2013/08/05 03:01:39, tapted wrote:
> nit: should say where it's overridden from

Done.

https://codereview.chromium.org/20656002/diff/207001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.h:79: // Overriden from
app_list::SigninDelegateObserver:
On 2013/08/05 03:01:39, tapted wrote:
> Overriden -> overridden (spelling)

Done.

https://codereview.chromium.org/20656002/diff/207001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.h:82: //
content::NotificationObserver
On 2013/08/05 03:01:39, tapted wrote:
> nit: Overridden from.., to be consistent

Done.

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/app_list_menu.h
File ui/app_list/app_list_menu.h (right):

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.h:8: #include <vector>
On 2013/08/05 03:01:39, tapted wrote:
> I guess the header doesn't need this any more, since it's now a typedef?

Done.

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.h:38: const app_list::AppListModel::ProfileMenuItems&
profile_menu_items);
On 2013/08/05 03:01:39, tapted wrote:
> nit: doesn't need app_list::

Done.

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.h:38: const app_list::AppListModel::ProfileMenuItems&
profile_menu_items);
On 2013/08/05 03:01:39, tapted wrote:
> nit: doesn't need app_list::

Done.

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.h:56: const app_list::AppListModel::ProfileMenuItems&
profile_menu_items_;
On 2013/08/05 03:52:13, koz wrote:
> On 2013/08/05 03:01:39, tapted wrote:
> > I don't usually see things declared this way in Chrome.. Maybe a weak
pointer?
> > Either way, it should have a comment saying who owns it. The app_list:: is
> also
> > redundant.
> 
> Right - this should probably be a value.

Done.

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:61: const string16& current_user_name() const {
return current_user_name_; }
On 2013/08/05 03:01:39, tapted wrote:
> nit: base:: prefix on string16

Done.

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:64: const ProfileMenuItems& profile_menu_items() {
On 2013/08/05 03:01:39, tapted wrote:
> nit: const qualifier on member function?

Done.

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model_observer.h (right):

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/app_list_mod...
ui/app_list/app_list_model_observer.h:18: // Invoked when AppListModel's current
user or avatar menu items have changed.
On 2013/08/05 03:01:39, tapted wrote:
> This comment doesn't look right.

Done.

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/app_list_vie...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/app_list_vie...
ui/app_list/app_list_view_delegate.h:35: // Invoked to initialize the model that
AppListView uses. This binds the given
On 2013/08/05 03:01:39, tapted wrote:
> Incomplete sentence: "This binds the given.."

Done.

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/app_list_vie...
ui/app_list/app_list_view_delegate.h:96: virtual void
UpdateModelWithCurrentProfiles() = 0;
On 2013/08/05 03:01:39, tapted wrote:
> Probably shouldn't be plural Profiles. Maybe call this
> UpdateMenuWithCurrentProfile ? (or UpdateMenuModelWithCurrentProfile?).

Hmm. Plural is appropriate though because it also rebuilds the ProfileMenuItems.
How about UpdateProfileItemsInModel?

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/pagination_m...
File ui/app_list/pagination_model.cc (right):

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/pagination_m...
ui/app_list/pagination_model.cc:86: DCHECK(page == 0 || (page > 0 && page <
total_pages_));
On 2013/08/05 01:09:11, koz wrote:
> This change seems unnecessary.

Oops. Should be total_pages_ == 0. This needed to be here to avoid DCHECKing
when notifying the pagination model that apps have been added to the app list
when repopulating the model due to profile switch.

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/signin_deleg...
File ui/app_list/signin_delegate.h (right):

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/signin_deleg...
ui/app_list/signin_delegate.h:37: virtual void SetProfileByPath(const
base::FilePath& profile_path) = 0;
On 2013/08/06 04:51:33, koz wrote:
> On 2013/08/06 01:37:29, benwells wrote:
> > On 2013/08/05 01:09:11, koz wrote:
> > > This should take a Profile*, seeing as implementors simply look up a
> Profile*
> > > from profile_path anyway.
> > 
> > I don't think ui/app_list shouldn't depend on Profile.
> 
> Good point. This method should probably not be in this interface at all. It
> would go better in the implementation (ie: ChromeSigninDelegate) which deals
> with Profile*s.

Done.

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view.h (right):

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.h:92: virtual void
OnAppListModelSigninStatusChanged() OVERRIDE;
On 2013/08/05 01:09:11, koz wrote:
> nit: This is also overidden from AppListModelObserver so put it beneath that
> comment below.

Done.

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/views/search...
File ui/app_list/views/search_box_view.cc (right):

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/views/search...
ui/app_list/views/search_box_view.cc:70: if (model_)
On 2013/08/05 03:01:39, tapted wrote:
> This `if` should be redundant now -- model_ is always non-NULL

Done.

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/views/signin...
File ui/app_list/views/signin_view.h (right):

https://codereview.chromium.org/20656002/diff/207001/ui/app_list/views/signin...
ui/app_list/views/signin_view.h:31: void set_delegate(SigninDelegate* delegate)
{ delegate_ = delegate; }
On 2013/08/05 01:09:11, koz wrote:
> Is this used?

Removed.

https://codereview.chromium.org/20656002/diff/207001/ui/base/profile_selector...
File ui/base/profile_selector/avatar_menu_item_model.h (right):

https://codereview.chromium.org/20656002/diff/207001/ui/base/profile_selector...
ui/base/profile_selector/avatar_menu_item_model.h:5: #ifndef
UI_BASE_MODELS_AVATAR_MENU_ITEM_MODEL_H_
On 2013/08/05 03:01:39, tapted wrote:
> This include guard needs updating

Done.

[koz (OOO until 15th September), 2013-08-08 06:25:01]

Another thing you could potentially split out is the AvatarMenuModel stuff.

https://codereview.chromium.org/20656002/diff/261001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_impl.h (right):

https://codereview.chromium.org/20656002/diff/261001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_impl.h:32: virtual void
SetProfilePath(const base::FilePath& profile_path) OVERRIDE;
nit: newline

https://codereview.chromium.org/20656002/diff/261001/ui/app_list/signin_deleg...
File ui/app_list/signin_delegate.h (right):

https://codereview.chromium.org/20656002/diff/261001/ui/app_list/signin_deleg...
ui/app_list/signin_delegate.h:14: class FilePath;
Do you still need to add this?

[calamity, 2013-08-09 09:10:56]

Split avatar menu model changes into https://codereview.chromium.org/22597004/ .

https://codereview.chromium.org/20656002/diff/261001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_impl.h (right):

https://codereview.chromium.org/20656002/diff/261001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_impl.h:32: virtual void
SetProfilePath(const base::FilePath& profile_path) OVERRIDE;
On 2013/08/08 06:25:02, koz wrote:
> nit: newline

Done.

https://codereview.chromium.org/20656002/diff/261001/ui/app_list/signin_deleg...
File ui/app_list/signin_delegate.h (right):

https://codereview.chromium.org/20656002/diff/261001/ui/app_list/signin_deleg...
ui/app_list/signin_delegate.h:14: class FilePath;
On 2013/08/08 06:25:02, koz wrote:
> Do you still need to add this?

Removed.

[calamity, 2013-09-09 20:18:02]

Menu model changes have been landed.

Rebased, added jstark's ui assets and removed app_list_menu_views.cc custom
views that are now implemented by MenuModel through SetSublabel.

[koz (OOO until 15th September), 2013-09-09 22:58:06]

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

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:57: void
PopulateUsers(ProfileInfoCache& profile_info,
Use a const ref or a pointer, non-const refs are against the style guide.

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:62:
app_list::AppListModel::User* user = new app_list::AppListModel::User();
nit: Use a scoped_ptr<> and then .release() it into |users|.

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:152:
app_sync_ui_state_watcher_.reset(new AppSyncUIStateWatcher(profile_,
What does this do?

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/apps_model_builder.cc (right):

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/apps_model_builder.cc:180: tracker_ =
extensions::InstallTrackerFactory::GetForProfile(profile_);
It's a bit tricky that we have tracker_->RemoveObserver() here and
tracker_->AddObserver() in Build(). Perhaps they should be merged into a single
function BuildForProfile(Profile*) or StartTrackingProfile(Profile*).

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:33: // Whether or not the app list is currently
instantiated for this user.
Maybe "Whether or not this user is the current user of the app list." ?

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:53: typedef std::vector<User*> Users;
You should just use User here, not User*. This is a tiny amount of data so the
extra complexity of managing the lifetime is not worth the savings.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_menu_views.cc (left):

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/views/app_li...
ui/app_list/views/app_list_menu_views.cc:25: class CurrentUserView : public
views::View {
Nice!

[tapted, 2013-09-10 00:29:26]

Can you put some screenshots on the bug?

And... you will need to do something for Mac. I played around with this patch in
https://codereview.chromium.org/23766018/ to see if we could get away with
landing mac and windows separately... I don't think we can. But that CL is
pretty close I think -- send me some screenshots/assets/binary-diff and I can
probably get the mac side working for you ;)

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

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:58: base::FilePath
active_profile_path,
nit: const-ref for this guy too ;)

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:124:
signin_delegate_->SetProfile(profile_);
Why couldn't this just be signin_delegate_.reset(new
ChromeSigninDelegate(profile_)); ? It feels odd to still pass the profile to the
constructor, while allowing it to be changed.

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:132: StartSearch();
This feels funny here.. it feels like something the view code should be
responsible for, since there are some assumptions being made about whether the
view is in a state where it is ready to show a search to begin with.

In particular, if someone switches to a profile that is not signed in, it
probably shouldn't start searching.

Maybe moving it is too messy. In which case it probably needs a comment here.
Took me some thinking to figure out the rationale.

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.h:9: #include <vector>
nit: can this go in the .cc?

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/views/...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:896:
AppListController::GetInstance()->SetProfilePath(profile_path);
If you do something like

app_list::AppListService service = AppListController::GetInstance();
service->SetProfilePath(profile_path);
service->Show();

then I don't think you need to make these methods public. I think this is the
preferred way in Chrome (and the code above is arguably nicer besides). Keeping
them private is also an indicator that the methods shouldn't be overridden by a
subclass.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_men...
File ui/app_list/app_list_menu.cc (right):

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.cc:27: // User selector menu section.
nit: perhaps mention that when there is exactly 1 user, no selector items are
added so people don't think you're crazy checking >1 below instead of >0

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.cc:32: menu_model_.SetSublabel(i, users_[i]->email);
You should do

int menu_index = GetIndexOfCommandId(SELECT_PROFILE + i);
menu_model_.SetSublabel(menu_index, users_[i]->email);

etc.

or... maybe a DCHECK_EQ(i, GetIndexOfCommandId(SELECT_PROFILE + i)); But I think
I like the one above better.

Also did you try using GetSublabelAt(..) instead? Not sure it would be better,
just curious.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.cc:33: if (users_[i]->active) {
Also, why not IsCommandIdChecked? Maybe a comment would be good here.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:34: bool active;
You should probably initialize |active| to something. Also llvm will whinge at
you that the constructor/destructor are not trivial and can't be
implicit/inlined.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:37: string16 name;
nit: base::string16 unless there's a prevailing convention for string16 in this
file

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:81: base::string16 current_user_name_;
is this (and current_user_email_) still needed?

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:84: Users users_;
nit: maybe move above signed_in_ -- it's nice to keep primitive types together
(sometimes) to avoid padding for struct alignment.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/pagination_m...
File ui/app_list/pagination_model.cc (right):

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/pagination_m...
ui/app_list/pagination_model.cc:35: SelectPage(std::max(total_pages_ - 1, 0),
false /* animate */);
this change looks orthogonal.. is it a bug? Or, perhaps mention in the CL
description.

https://codereview.chromium.org/20656002/diff/360001/ui/resources/ui_resource...
File ui/resources/ui_resources.grd (right):

https://codereview.chromium.org/20656002/diff/360001/ui/resources/ui_resource...
ui/resources/ui_resources.grd:29: <structure type="chrome_scaled_image"
name="IDR_APP_LIST_USER_INDICATOR" file="common/app_list_user_indicator.png" />
can this be removed too?

[xiyuan, 2013-09-10 01:54:32]

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

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:132: StartSearch();
On 2013/09/10 00:29:26, tapted wrote:
> This feels funny here.. it feels like something the view code should be
> responsible for, since there are some assumptions being made about whether the
> view is in a state where it is ready to show a search to begin with.
> 
> In particular, if someone switches to a profile that is not signed in, it
> probably shouldn't start searching.
> 
> Maybe moving it is too messy. In which case it probably needs a comment here.
> Took me some thinking to figure out the rationale.

It does look strange. I would prefer to clear query when switching profile.

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:152:
app_sync_ui_state_watcher_.reset(new AppSyncUIStateWatcher(profile_,
On 2013/09/09 22:58:07, koz wrote:
> What does this do?

This is for ChromeOS to show pulsing animation when user signs-in on a device
for the first time. If a profile is new,  and when sync is in progress or there
are pending extension installs, it sets AppListModel status to turn on/off the
pulsing animation. It does nothing if the profile is already created.

[koz (OOO until 15th September), 2013-09-10 15:21:36]

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

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:152:
app_sync_ui_state_watcher_.reset(new AppSyncUIStateWatcher(profile_,
On 2013/09/10 01:54:33, xiyuan wrote:
> On 2013/09/09 22:58:07, koz wrote:
> > What does this do?
> 
> This is for ChromeOS to show pulsing animation when user signs-in on a device
> for the first time. If a profile is new,  and when sync is in progress or
there
> are pending extension installs, it sets AppListModel status to turn on/off the
> pulsing animation. It does nothing if the profile is already created.

Cool, thanks for the explanation. Sounds like something it'd be good to expose
in the windows UI, too.

[tapted, 2013-09-10 21:20:02]

https://codereview.chromium.org/20656002/diff/380001/ui/app_list/app_list_men...
File ui/app_list/app_list_menu.cc (right):

https://codereview.chromium.org/20656002/diff/380001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.cc:28: if (users_.size() > 1) {
While fighting jetlag last night, I realised you will also need to check the
Ash-on-Windows8 case - pretty sure it will come up. Profile switching does not
work there, so we shouldn't show the profile selector (see e.g.
http://crbug.com/229704 ). You'll need a runtime check for desktop type
according to the app list window. Maybe here via a delegate interface, or
possibly (maybe nicer) in c/b/ui/app_list/app_list_view_delegate.cc simply by
putting only the active user in the model, in PopulateUsers. But then (since
they *are* a multi-profile user), we might want to show which profile is active,
but first cut could be to just rely on the ash system tray indicating the active
profile (assuming that's still a thing on Windows8).

[calamity, 2013-09-10 22:50:45]

Mac patch on its way.

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

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:57: void
PopulateUsers(ProfileInfoCache& profile_info,
On 2013/09/09 22:58:07, koz wrote:
> Use a const ref or a pointer, non-const refs are against the style guide.

Done.

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:58: base::FilePath
active_profile_path,
On 2013/09/10 00:29:26, tapted wrote:
> nit: const-ref for this guy too ;)

Done.

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:62:
app_list::AppListModel::User* user = new app_list::AppListModel::User();
On 2013/09/09 22:58:07, koz wrote:
> nit: Use a scoped_ptr<> and then .release() it into |users|.

No longer using pointers.

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:124:
signin_delegate_->SetProfile(profile_);
On 2013/09/10 00:29:26, tapted wrote:
> Why couldn't this just be signin_delegate_.reset(new
> ChromeSigninDelegate(profile_)); ? It feels odd to still pass the profile to
the
> constructor, while allowing it to be changed.

SigninView holds onto this delegate when its constructed so the alternative
would be to either give SigninView the AppListViewDelegate so it can retrieve
the signin delegate when its updated or to reconstruct the SigninView on
rebuild.

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:132: StartSearch();
On 2013/09/10 01:54:33, xiyuan wrote:
> On 2013/09/10 00:29:26, tapted wrote:
> > This feels funny here.. it feels like something the view code should be
> > responsible for, since there are some assumptions being made about whether
the
> > view is in a state where it is ready to show a search to begin with.
> > 
> > In particular, if someone switches to a profile that is not signed in, it
> > probably shouldn't start searching.
> > 
> > Maybe moving it is too messy. In which case it probably needs a comment
here.
> > Took me some thinking to figure out the rationale.
> 
> It does look strange. I would prefer to clear query when switching profile.

Clearing the search box means that if a user starts searching in the wrong
profile, they'll need to re-type their query to switch their search context. My
preference is leave the query intact.

The view only knows about the signin state through the signin delegate which we
have access to here (so we can do the signed in check here). The search only
refreshes the results in the search view so it shouldn't do anything funny to
the app launcher state.

WDYT?

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.h:9: #include <vector>
On 2013/09/10 00:29:26, tapted wrote:
> nit: can this go in the .cc?

Done.

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/apps_model_builder.cc (right):

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/apps_model_builder.cc:180: tracker_ =
extensions::InstallTrackerFactory::GetForProfile(profile_);
On 2013/09/09 22:58:07, koz wrote:
> It's a bit tricky that we have tracker_->RemoveObserver() here and
> tracker_->AddObserver() in Build(). Perhaps they should be merged into a
single
> function BuildForProfile(Profile*) or StartTrackingProfile(Profile*).

Removed the Build() method and changed SetProfile() to SwitchProfile() which
will now set up the tracker as well as build the model. SwitchProfile() is
called on construction to build the model for the given profile. Tests and
callsites have been updated.

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/views/...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:896:
AppListController::GetInstance()->SetProfilePath(profile_path);
On 2013/09/10 00:29:26, tapted wrote:
> If you do something like
> 
> app_list::AppListService service = AppListController::GetInstance();
> service->SetProfilePath(profile_path);
> service->Show();
> 
> then I don't think you need to make these methods public. I think this is the
> preferred way in Chrome (and the code above is arguably nicer besides).
Keeping
> them private is also an indicator that the methods shouldn't be overridden by
a
> subclass.

Ah cool. Done.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_men...
File ui/app_list/app_list_menu.cc (right):

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.cc:27: // User selector menu section.
On 2013/09/10 00:29:26, tapted wrote:
> nit: perhaps mention that when there is exactly 1 user, no selector items are
> added so people don't think you're crazy checking >1 below instead of >0

Done.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.cc:32: menu_model_.SetSublabel(i, users_[i]->email);
On 2013/09/10 00:29:26, tapted wrote:
> You should do
> 
> int menu_index = GetIndexOfCommandId(SELECT_PROFILE + i);
> menu_model_.SetSublabel(menu_index, users_[i]->email);
> 
> etc.
> 
> or... maybe a DCHECK_EQ(i, GetIndexOfCommandId(SELECT_PROFILE + i)); But I
think
> I like the one above better.
> 
> Also did you try using GetSublabelAt(..) instead? Not sure it would be better,
> just curious.

Done. What was GetSublabelAt() for?

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.cc:33: if (users_[i]->active) {
On 2013/09/10 00:29:26, tapted wrote:
> Also, why not IsCommandIdChecked? Maybe a comment would be good here.

Done.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:33: // Whether or not the app list is currently
instantiated for this user.
On 2013/09/09 22:58:07, koz wrote:
> Maybe "Whether or not this user is the current user of the app list." ?

Done.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:34: bool active;
On 2013/09/10 00:29:26, tapted wrote:
> You should probably initialize |active| to something. Also llvm will whinge at
> you that the constructor/destructor are not trivial and can't be
> implicit/inlined.

Done.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:37: string16 name;
On 2013/09/10 00:29:26, tapted wrote:
> nit: base::string16 unless there's a prevailing convention for string16 in
this
> file

Done.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:53: typedef std::vector<User*> Users;
On 2013/09/09 22:58:07, koz wrote:
> You should just use User here, not User*. This is a tiny amount of data so the
> extra complexity of managing the lifetime is not worth the savings.

Done.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:81: base::string16 current_user_name_;
On 2013/09/10 00:29:26, tapted wrote:
> is this (and current_user_email_) still needed?

Removed.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:84: Users users_;
On 2013/09/10 00:29:26, tapted wrote:
> nit: maybe move above signed_in_ -- it's nice to keep primitive types together
> (sometimes) to avoid padding for struct alignment.

Done.

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/pagination_m...
File ui/app_list/pagination_model.cc (right):

https://codereview.chromium.org/20656002/diff/360001/ui/app_list/pagination_m...
ui/app_list/pagination_model.cc:35: SelectPage(std::max(total_pages_ - 1, 0),
false /* animate */);
On 2013/09/10 00:29:26, tapted wrote:
> this change looks orthogonal.. is it a bug? Or, perhaps mention in the CL
> description.

It prevents the app list from populating with 0 apps (which wasn't a problem
until now). Added to the CL description.

https://codereview.chromium.org/20656002/diff/360001/ui/resources/ui_resource...
File ui/resources/ui_resources.grd (right):

https://codereview.chromium.org/20656002/diff/360001/ui/resources/ui_resource...
ui/resources/ui_resources.grd:29: <structure type="chrome_scaled_image"
name="IDR_APP_LIST_USER_INDICATOR" file="common/app_list_user_indicator.png" />
On 2013/09/10 00:29:26, tapted wrote:
> can this be removed too?

I was hoping to land windows separately from mac but since this won't be
possible, yes this can be removed.

[xiyuan, 2013-09-11 05:01:48]

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

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:132: StartSearch();
Sounds reasonable.

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (left):

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:234: return;
Why is this not needed?

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/apps_model_builder.cc (right):

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/apps_model_builder.cc:167: void
AppsModelBuilder::SwitchProfile(Profile* profile) {
nit: return early if profile_ == profile

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/ash/ap...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.h (right):

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/ash/ap...
chrome/browser/ui/ash/app_list/app_list_controller_ash.h:13: class FilePath;
nit: not needed since it is used in inherited AppListControllerDelegate
interface, not in a new method

https://codereview.chromium.org/20656002/diff/340001/ui/app_list/app_list_vie...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/340001/ui/app_list/app_list_vie...
ui/app_list/app_list_view_delegate.h:39: virtual void InitModel(AppListModel*
model) = 0;
Would it be better if we make AppListViewDelegate owns the model and having a
GetModel() call here to expose the model to views code?

[tapted, 2013-09-11 05:50:25]

Don't forget to update the CL description at some point ;)

Also, tests? :D You'll hit a Mac app_list_unittest that will need updating --
extending it a bit to test both the 1-profile case and the multi-profile case
would probably give decent coverage for Mac.

You'll get a bit of coverage from the SwitchAppListProfiles browser_test too.
But it could do with some fixing, since last time I saw a glimpse of it it was
just showing the signin page all the time..

I think there should also be a test for the
switching-profiles-with-an-active-search case - it feels a bit special.

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

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:124:
signin_delegate_->SetProfile(profile_);
On 2013/09/10 22:50:46, calamity wrote:
> On 2013/09/10 00:29:26, tapted wrote:
> > Why couldn't this just be signin_delegate_.reset(new
> > ChromeSigninDelegate(profile_)); ? It feels odd to still pass the profile to
> the
> > constructor, while allowing it to be changed.
> 
> SigninView holds onto this delegate when its constructed so the alternative
> would be to either give SigninView the AppListViewDelegate so it can retrieve
> the signin delegate when its updated or to reconstruct the SigninView on
> rebuild.

Hm - so it does. Mac will fetch a new delegate in response to a signin status
change (via OnProfileChanged below).. but currently only if it switches from
true <-> false.

It looks the delegate will work OK with it as it is now, so I guess there's no
reason to change this. But it should be consistent. I suggest moving this line
into OnProfileChanged, along with the search_controller and
AppSyncUIStateWatcher parts here, and in InitModel(). Then remove the
constructor argument from ChromeSigninDelegate, and either .reset() it once in
the constructor, or just make it a regular member rather than a scoped_ptr (I'm
leaning slightly towards ditching scoped_ptr).

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:132: StartSearch();
On 2013/09/10 22:50:46, calamity wrote:
> On 2013/09/10 01:54:33, xiyuan wrote:
> > On 2013/09/10 00:29:26, tapted wrote:
> > > This feels funny here.. it feels like something the view code should be
> > > responsible for, since there are some assumptions being made about whether
> the
> > > view is in a state where it is ready to show a search to begin with.
> > > 
> > > In particular, if someone switches to a profile that is not signed in, it
> > > probably shouldn't start searching.
> > > 
> > > Maybe moving it is too messy. In which case it probably needs a comment
> here.
> > > Took me some thinking to figure out the rationale.
> > 
> > It does look strange. I would prefer to clear query when switching profile.
> 
> Clearing the search box means that if a user starts searching in the wrong
> profile, they'll need to re-type their query to switch their search context.
My
> preference is leave the query intact.
> 
> The view only knows about the signin state through the signin delegate which
we
> have access to here (so we can do the signed in check here). The search only
> refreshes the results in the search view so it shouldn't do anything funny to
> the app launcher state.
> 
> WDYT?

I'm leaning slightly towards clearing results and forcing them to retype - but
I'll defer to xiyuan/UX. My reasoning: just refreshing results is a bit too
subtle. There's a chance it will provide the same results, and the interface
won't look like it switched at all. Showing users' "home" page of apps is a good
way to reassure them that the switch took place.

A new method on AppListModelObserver might be a nicer way to do it though -
there are already reactions to 'is signed in' keyed off that. Also, E.g. is
there more to do? What happens if a profile switch occurs when someone is on
their second page of apps - does pagination reset to the first page? But if it
all Just Works (and there are tests ;) ) I'm not really bothered with it being
here either.

https://codereview.chromium.org/20656002/diff/360001/ui/resources/ui_resource...
File ui/resources/ui_resources.grd (right):

https://codereview.chromium.org/20656002/diff/360001/ui/resources/ui_resource...
ui/resources/ui_resources.grd:29: <structure type="chrome_scaled_image"
name="IDR_APP_LIST_USER_INDICATOR" file="common/app_list_user_indicator.png" />
On 2013/09/10 22:50:46, calamity wrote:
> On 2013/09/10 00:29:26, tapted wrote:
> > can this be removed too?
> 
> I was hoping to land windows separately from mac but since this won't be
> possible, yes this can be removed.

Make sure you delete the binaries too

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/chrome_signin_delegate.cc (right):

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/chrome_signin_delegate.cc:102: void
ChromeSigninDelegate::SetProfile(Profile* profile) {
nit: keep definitions ordered the same as declarations (destructor should move
too).

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/chrome_signin_delegate.h (right):

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/chrome_signin_delegate.h:21: 
nit: no blank line here

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/chrome_signin_delegate.h:22: bool IsActiveSignin();
hm - is this defined/used? maybe delete while you're here.. FinishSignin looks
undefined too

https://codereview.chromium.org/20656002/diff/340001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/20656002/diff/340001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.cc:60: AppListModel::User::User() : active(false) {
nit: move these to the top to match declaration order

[calamity, 2013-09-13 18:02:34]

Mac stuff implemented.

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

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:124:
signin_delegate_->SetProfile(profile_);
On 2013/09/11 05:50:26, tapted wrote:
> On 2013/09/10 22:50:46, calamity wrote:
> > On 2013/09/10 00:29:26, tapted wrote:
> > > Why couldn't this just be signin_delegate_.reset(new
> > > ChromeSigninDelegate(profile_)); ? It feels odd to still pass the profile
to
> > the
> > > constructor, while allowing it to be changed.
> > 
> > SigninView holds onto this delegate when its constructed so the alternative
> > would be to either give SigninView the AppListViewDelegate so it can
retrieve
> > the signin delegate when its updated or to reconstruct the SigninView on
> > rebuild.
> 
> Hm - so it does. Mac will fetch a new delegate in response to a signin status
> change (via OnProfileChanged below).. but currently only if it switches from
> true <-> false.
> 
> It looks the delegate will work OK with it as it is now, so I guess there's no
> reason to change this. But it should be consistent. I suggest moving this line
> into OnProfileChanged, along with the search_controller and
> AppSyncUIStateWatcher parts here, and in InitModel(). Then remove the
> constructor argument from ChromeSigninDelegate, and either .reset() it once in
> the constructor, or just make it a regular member rather than a scoped_ptr
(I'm
> leaning slightly towards ditching scoped_ptr).

Done.

https://codereview.chromium.org/20656002/diff/360001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:132: StartSearch();
On 2013/09/11 05:50:26, tapted wrote:
> On 2013/09/10 22:50:46, calamity wrote:
> > On 2013/09/10 01:54:33, xiyuan wrote:
> > > On 2013/09/10 00:29:26, tapted wrote:
> > > > This feels funny here.. it feels like something the view code should be
> > > > responsible for, since there are some assumptions being made about
whether
> > the
> > > > view is in a state where it is ready to show a search to begin with.
> > > > 
> > > > In particular, if someone switches to a profile that is not signed in,
it
> > > > probably shouldn't start searching.
> > > > 
> > > > Maybe moving it is too messy. In which case it probably needs a comment
> > here.
> > > > Took me some thinking to figure out the rationale.
> > > 
> > > It does look strange. I would prefer to clear query when switching
profile.
> > 
> > Clearing the search box means that if a user starts searching in the wrong
> > profile, they'll need to re-type their query to switch their search context.
> My
> > preference is leave the query intact.
> > 
> > The view only knows about the signin state through the signin delegate which
> we
> > have access to here (so we can do the signed in check here). The search only
> > refreshes the results in the search view so it shouldn't do anything funny
to
> > the app launcher state.
> > 
> > WDYT?
> 
> I'm leaning slightly towards clearing results and forcing them to retype - but
> I'll defer to xiyuan/UX. My reasoning: just refreshing results is a bit too
> subtle. There's a chance it will provide the same results, and the interface
> won't look like it switched at all. Showing users' "home" page of apps is a
good
> way to reassure them that the switch took place.
> 
> A new method on AppListModelObserver might be a nicer way to do it though -
> there are already reactions to 'is signed in' keyed off that. Also, E.g. is
> there more to do? What happens if a profile switch occurs when someone is on
> their second page of apps - does pagination reset to the first page? But if it
> all Just Works (and there are tests ;) ) I'm not really bothered with it being
> here either.

Changed to clear search text on profile change for now. Page does indeed just go
back to page 0.

https://codereview.chromium.org/20656002/diff/360001/ui/resources/ui_resource...
File ui/resources/ui_resources.grd (right):

https://codereview.chromium.org/20656002/diff/360001/ui/resources/ui_resource...
ui/resources/ui_resources.grd:29: <structure type="chrome_scaled_image"
name="IDR_APP_LIST_USER_INDICATOR" file="common/app_list_user_indicator.png" />
On 2013/09/11 05:50:26, tapted wrote:
> On 2013/09/10 22:50:46, calamity wrote:
> > On 2013/09/10 00:29:26, tapted wrote:
> > > can this be removed too?
> > 
> > I was hoping to land windows separately from mac but since this won't be
> > possible, yes this can be removed.
> 
> Make sure you delete the binaries too

Done.

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (left):

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:234: return;
On 2013/09/11 05:01:49, xiyuan wrote:
> Why is this not needed?

We need to remove this so that the menu updates on name change for profiles
other than the current profile. i.e X is active, Y changes name, menu should
update to reflect this.

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/apps_model_builder.cc (right):

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/apps_model_builder.cc:167: void
AppsModelBuilder::SwitchProfile(Profile* profile) {
On 2013/09/11 05:01:49, xiyuan wrote:
> nit: return early if profile_ == profile

Done.

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/chrome_signin_delegate.cc (right):

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/chrome_signin_delegate.cc:102: void
ChromeSigninDelegate::SetProfile(Profile* profile) {
On 2013/09/11 05:50:26, tapted wrote:
> nit: keep definitions ordered the same as declarations (destructor should move
> too).

Done.

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/chrome_signin_delegate.h (right):

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/chrome_signin_delegate.h:21: 
On 2013/09/11 05:50:26, tapted wrote:
> nit: no blank line here

Done.

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/chrome_signin_delegate.h:22: bool IsActiveSignin();
On 2013/09/11 05:50:26, tapted wrote:
> hm - is this defined/used? maybe delete while you're here.. FinishSignin looks
> undefined too

Done.

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/ash/ap...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.h (right):

https://codereview.chromium.org/20656002/diff/340001/chrome/browser/ui/ash/ap...
chrome/browser/ui/ash/app_list/app_list_controller_ash.h:13: class FilePath;
On 2013/09/11 05:01:49, xiyuan wrote:
> nit: not needed since it is used in inherited AppListControllerDelegate
> interface, not in a new method

Done.

https://codereview.chromium.org/20656002/diff/340001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/20656002/diff/340001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.cc:60: AppListModel::User::User() : active(false) {
On 2013/09/11 05:50:26, tapted wrote:
> nit: move these to the top to match declaration order

Done.

https://codereview.chromium.org/20656002/diff/340001/ui/app_list/app_list_vie...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/340001/ui/app_list/app_list_vie...
ui/app_list/app_list_view_delegate.h:39: virtual void InitModel(AppListModel*
model) = 0;
On 2013/09/11 05:01:49, xiyuan wrote:
> Would it be better if we make AppListViewDelegate owns the model and having a
> GetModel() call here to expose the model to views code?

I think this is a design question that needs a bit more discussion and can be
addressed separately from this change.

[tapted, 2013-09-13 23:12:16]

Don't forget:
 - Ash on Windows 8 shouldn't show profile switcher
 - src/tools/resources/optimize-png-files.sh -o2
 - update CL description

https://codereview.chromium.org/20656002/diff/417001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/20656002/diff/417001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:452: // The Objective C
objects might be released at some unknown point in the
I think here, just do

if (window_controller_) {
  // Clear the search box... 
  [[window_controller_ appListViewController]
searchBoxModel]->SetText(base::string16());
} else {
  window_controller_.reset([[AppListWindowController alloc] init]);
}

then you don't need to change ShowForProfile() at all (after the
ShowWindowNearDock change below), and you avoid repeating some logic.

I'm pretty sure you can also just delete the lines:

  // The Objective C objects might...
  [[window_controller_ appListViewController]
        setDelegate:scoped_ptr<app_list::AppListViewDelegate>()];

Since the maybe-non-nil window_controller_ is no longer getting "fake-destroyed"
by Objective-C by the .reset(), there's no danger of it holding on to C++
objects in the case it doesn't actually get destroyed. You're probably relying
on this in the code added to ShowForProfile anyway, since you don't have that
and now in most cases CreateForProfile is called with a nil window_controller_
anyway.

https://codereview.chromium.org/20656002/diff/417001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:477: DismissAppList();
this will always be a no-op with the condition above

https://codereview.chromium.org/20656002/diff/417001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:490: if (!IsAppListVisible())
I think this check can just go at the start of ShowWindowNearDock() (i.e. return
early if visible), and it will fix http://crbug.com/289872 as well. Not sure if
it should re-show if it is in the 200ms window when it is fading out... I'd say
initially "no"

https://codereview.chromium.org/20656002/diff/417001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/apps_model_builder.cc (right):

https://codereview.chromium.org/20656002/diff/417001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/apps_model_builder.cc:169: return;
nit: blank line after early return

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/app_list_men...
File ui/app_list/app_list_menu.cc (right):

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.cc:42: ui::ResourceBundle& rb =
ui::ResourceBundle::GetSharedInstance();
ugh - I guess mac complains that it's unused when the dec is outside the loop.
An option would be to duplicate the loop condition inside the #if - not sure if
that's better though.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.cc:43: menu_model_.SetIcon(i,
gfx::Image(*rb.GetImageSkiaNamed(
..SetIcon(i, ..  --> ..SetIcon(menu_index, ..

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.cc:63: return users_[command_id -
SELECT_PROFILE].active;
maybe

DCHECK_LT(static_cast<unsigned>(command_id) - SELECT_PROFILE, users_.size());

I guess you could add more, like command_id < SELECT_PROFILE, but I'm pretty
sure it's redundant with but unsigned overflow rules

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.cc:14: AppListModel::User::User() : active(false) {
nit: maybe brackets like {}, on the same line. Seems more common, and not adding
inconsistency

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller.h (right):

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller.h:13: #include
"ui/app_list/app_list_model.h"
this can be forward declared i think

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller_unittest.mm (right):

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:35: if ((self = [super
init])) {
you need to indent this function an extra space

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:36:
app_list::AppListModel::Users users;
maybe delete the next two lines and construct like

app_list::AppListModel::Users users(2);

that should create a vector with two default constructed items.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:153: // Test the popup
menu items.
nit: different comments for the two tests.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:155:
app_list::AppListModel::Users users;
users(1);

you could even just do `[delegate_
appListModel]->SetUsers(app_list::AppListModel::Users(1));`

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:166: }
I think this is missing something. We want to ensure, for the single-profile
case, that neither the menu model _nor_ the menu itself have additional items
for any profiles -- it should just have the 3 fixed items, and no separator. I
think just another check against the SELECT_PROFILE constant, since there are
assumptions around how that behaves which can be tested a bit here too.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:189:
EXPECT_FALSE([unchecked_user_item view]);
we can drop the checks for a view, since they're a pretty obscure menu feature
now that we can be rid of them. More below.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:190:
EXPECT_NSEQ(base::SysUTF16ToNSString(menu_model->GetLabelAt(index)),
can this check against the AppListModel::Users collection? I think that's more
interesting, since we also want to verify the logic that if the email is
missing, the profile name is shown instead.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:194: // The second user
item is a checked label.
nit: say why it is checked (i.e. since it is the active profile)

[calamity, 2013-09-16 19:45:26]

https://codereview.chromium.org/20656002/diff/417001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/20656002/diff/417001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:452: // The Objective C
objects might be released at some unknown point in the
On 2013/09/13 23:12:17, tapted wrote:
> I think here, just do
> 
> if (window_controller_) {
>   // Clear the search box... 
>   [[window_controller_ appListViewController]
> searchBoxModel]->SetText(base::string16());
> } else {
>   window_controller_.reset([[AppListWindowController alloc] init]);
> }
> 
> then you don't need to change ShowForProfile() at all (after the
> ShowWindowNearDock change below), and you avoid repeating some logic.
> 
> I'm pretty sure you can also just delete the lines:
> 
>   // The Objective C objects might...
>   [[window_controller_ appListViewController]
>         setDelegate:scoped_ptr<app_list::AppListViewDelegate>()];
> 
> Since the maybe-non-nil window_controller_ is no longer getting
"fake-destroyed"
> by Objective-C by the .reset(), there's no danger of it holding on to C++
> objects in the case it doesn't actually get destroyed. You're probably relying
> on this in the code added to ShowForProfile anyway, since you don't have that
> and now in most cases CreateForProfile is called with a nil window_controller_
> anyway.

Done.

https://codereview.chromium.org/20656002/diff/417001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:477: DismissAppList();
On 2013/09/13 23:12:17, tapted wrote:
> this will always be a no-op with the condition above

Removed.

https://codereview.chromium.org/20656002/diff/417001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:490: if (!IsAppListVisible())
On 2013/09/13 23:12:17, tapted wrote:
> I think this check can just go at the start of ShowWindowNearDock() (i.e.
return
> early if visible), and it will fix http://crbug.com/289872 as well. Not sure
if
> it should re-show if it is in the 200ms window when it is fading out... I'd
say
> initially "no"

Done.

https://codereview.chromium.org/20656002/diff/417001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/apps_model_builder.cc (right):

https://codereview.chromium.org/20656002/diff/417001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/apps_model_builder.cc:169: return;
On 2013/09/13 23:12:17, tapted wrote:
> nit: blank line after early return

Done.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/app_list_men...
File ui/app_list/app_list_menu.cc (right):

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.cc:43: menu_model_.SetIcon(i,
gfx::Image(*rb.GetImageSkiaNamed(
On 2013/09/13 23:12:17, tapted wrote:
> ..SetIcon(i, ..  --> ..SetIcon(menu_index, ..

Done.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.cc:63: return users_[command_id -
SELECT_PROFILE].active;
On 2013/09/13 23:12:17, tapted wrote:
> maybe
> 
> DCHECK_LT(static_cast<unsigned>(command_id) - SELECT_PROFILE, users_.size());
> 
> I guess you could add more, like command_id < SELECT_PROFILE, but I'm pretty
> sure it's redundant with but unsigned overflow rules

Done.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.cc:14: AppListModel::User::User() : active(false) {
On 2013/09/13 23:12:17, tapted wrote:
> nit: maybe brackets like {}, on the same line. Seems more common, and not
adding
> inconsistency

Done.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller.h (right):

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller.h:13: #include
"ui/app_list/app_list_model.h"
On 2013/09/13 23:12:17, tapted wrote:
> this can be forward declared i think

Done.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller_unittest.mm (right):

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:35: if ((self = [super
init])) {
On 2013/09/13 23:12:17, tapted wrote:
> you need to indent this function an extra space

Done.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:36:
app_list::AppListModel::Users users;
On 2013/09/13 23:12:17, tapted wrote:
> maybe delete the next two lines and construct like
> 
> app_list::AppListModel::Users users(2);
> 
> that should create a vector with two default constructed items. 

Done.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:153: // Test the popup
menu items.
On 2013/09/13 23:12:17, tapted wrote:
> nit: different comments for the two tests.

Done.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:155:
app_list::AppListModel::Users users;
On 2013/09/13 23:12:17, tapted wrote:
> users(1);
> 
> you could even just do `[delegate_
> appListModel]->SetUsers(app_list::AppListModel::Users(1));`

Done.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:166: }
On 2013/09/13 23:12:17, tapted wrote:
> I think this is missing something. We want to ensure, for the single-profile
> case, that neither the menu model _nor_ the menu itself have additional items
> for any profiles -- it should just have the 3 fixed items, and no separator. I
> think just another check against the SELECT_PROFILE constant, since there are
> assumptions around how that behaves which can be tested a bit here too.

Done.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:189:
EXPECT_FALSE([unchecked_user_item view]);
On 2013/09/13 23:12:17, tapted wrote:
> we can drop the checks for a view, since they're a pretty obscure menu feature
> now that we can be rid of them. More below.

Done.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:190:
EXPECT_NSEQ(base::SysUTF16ToNSString(menu_model->GetLabelAt(index)),
On 2013/09/13 23:12:17, tapted wrote:
> can this check against the AppListModel::Users collection? I think that's more
> interesting, since we also want to verify the logic that if the email is
> missing, the profile name is shown instead.

Done.

https://codereview.chromium.org/20656002/diff/417001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:194: // The second user
item is a checked label.
On 2013/09/13 23:12:17, tapted wrote:
> nit: say why it is checked (i.e. since it is the active profile)

Done.

[tapted, 2013-09-16 20:46:43]

mac is looking pretty good! next: ash-on-win8 ;)

don't forget your todo list. You'll also need to switch back to the right assets
after running bots.

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:46:
profile_manager->CreateProfileAsync(
Will the path created by this call get deleted when the test ends? tests in
profile_browsertest.cc all use scoped paths. It could be the case that
everything under the user data dir is deleted reliably, but I've never confirmed
this, and we are not allowed to leave junk around..

You can perhaps just leave the data member in and use
temp_profile_dir_.Set(profile_manager->user_data_dir().AppendASCII("Profile
1"));

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:137:
SwitchAppListProfilesDuringSearch) {
This shouldn't be appearing anew in the diff - you might need to fix your
upstream.

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:549: return;
nit: blank line after early return

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.h:22: class
ChromeSigninDelegate;
don't need this forward dec any more

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.h:55: virtual void
SetProfileByPath(const base::FilePath& profile_path) OVERRIDE;
can this go in the private section, with the other overrides? I'm guessing
there's an error, but there might be an elegant implicit cast there, that could
keep the overrides together. (but if the implicit cast is messy, this might be
fine too).

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/chrome_signin_delegate.cc (right):

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/chrome_signin_delegate.cc:7: #include
"chrome/browser/browser_process.h"
should this be here? (maybe from a rebase)

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/chrome_signin_delegate.cc:10: #include
"chrome/browser/profiles/profile_manager.h"
this too

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/app_list_men...
File ui/app_list/app_list_menu.cc (right):

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.cc:35: 0);
nit: 0 /* group_id */);

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller.mm (right):

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller.mm:181: [delegate_
appListModel]->users()));
nit: since it is dereferenced here, app_list_model.h should be in #includes for
the .cc

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller_unittest.mm (right):

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:169: for (int i = 0; i
< menu_model->GetItemCount(); ++i) {
nit: no braces

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:170:
EXPECT_LT(menu_model->GetCommandIdAt(i), AppListMenu::SELECT_PROFILE);
this is cool - I'd keep it. But I'd also add (outside the loop)

// The number of items should match the index that starts profile items.
EXPECT_EQ(menu_model->GetItemCount(), AppListMenu::SELECT_PROFILE);

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:174: // Test the popup
menu items.
nit: ... for the multi-profile case.

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/test/app_lis...
File ui/app_list/test/app_list_test_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/test/app_lis...
ui/app_list/test/app_list_test_view_delegate.h:29: virtual void
SetProfileByPath(const base::FilePath& profile_path) OVERRIDE {};
stray semicolon at the end of this line

[tapted, 2013-09-16 21:02:19]

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller_unittest.mm (right):

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:170:
EXPECT_LT(menu_model->GetCommandIdAt(i), AppListMenu::SELECT_PROFILE);
On 2013/09/16 20:46:44, tapted wrote:
> this is cool - I'd keep it. But I'd also add (outside the loop)
> 
> // The number of items should match the index that starts profile items.
> EXPECT_EQ(menu_model->GetItemCount(), AppListMenu::SELECT_PROFILE);

oops. and to nit my own comment: the "more constant" argument usually comes
first in an EXPECT_EQ

[calamity, 2013-09-17 00:09:25]

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:46:
profile_manager->CreateProfileAsync(
On 2013/09/16 20:46:44, tapted wrote:
> Will the path created by this call get deleted when the test ends? tests in
> profile_browsertest.cc all use scoped paths. It could be the case that
> everything under the user data dir is deleted reliably, but I've never
confirmed
> this, and we are not allowed to leave junk around..
> 
> You can perhaps just leave the data member in and use
> temp_profile_dir_.Set(profile_manager->user_data_dir().AppendASCII("Profile
> 1"));

user_data_dir is a scoped_temp_dir and those are deleted recursively.

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:549: return;
On 2013/09/16 20:46:44, tapted wrote:
> nit: blank line after early return

Done.

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.h:22: class
ChromeSigninDelegate;
On 2013/09/16 20:46:44, tapted wrote:
> don't need this forward dec any more

Done.

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.h:55: virtual void
SetProfileByPath(const base::FilePath& profile_path) OVERRIDE;
On 2013/09/16 20:46:44, tapted wrote:
> can this go in the private section, with the other overrides? I'm guessing
> there's an error, but there might be an elegant implicit cast there, that
could
> keep the overrides together. (but if the implicit cast is messy, this might be
> fine too).

Done.

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/chrome_signin_delegate.cc (right):

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/chrome_signin_delegate.cc:7: #include
"chrome/browser/browser_process.h"
On 2013/09/16 20:46:44, tapted wrote:
> should this be here? (maybe from a rebase)

Removed.

https://codereview.chromium.org/20656002/diff/436001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/chrome_signin_delegate.cc:10: #include
"chrome/browser/profiles/profile_manager.h"
On 2013/09/16 20:46:44, tapted wrote:
> this too

Removed.

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/app_list_men...
File ui/app_list/app_list_menu.cc (right):

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/app_list_men...
ui/app_list/app_list_menu.cc:35: 0);
On 2013/09/16 20:46:44, tapted wrote:
> nit: 0 /* group_id */);

Done.

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller.mm (right):

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller.mm:181: [delegate_
appListModel]->users()));
On 2013/09/16 20:46:44, tapted wrote:
> nit: since it is dereferenced here, app_list_model.h should be in #includes
for
> the .cc

Done.

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/cocoa/apps_s...
File ui/app_list/cocoa/apps_search_box_controller_unittest.mm (right):

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:169: for (int i = 0; i
< menu_model->GetItemCount(); ++i) {
On 2013/09/16 20:46:44, tapted wrote:
> nit: no braces

Done.

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:170:
EXPECT_LT(menu_model->GetCommandIdAt(i), AppListMenu::SELECT_PROFILE);
On 2013/09/16 21:02:20, tapted wrote:
> On 2013/09/16 20:46:44, tapted wrote:
> > this is cool - I'd keep it. But I'd also add (outside the loop)
> > 
> > // The number of items should match the index that starts profile items.
> > EXPECT_EQ(menu_model->GetItemCount(), AppListMenu::SELECT_PROFILE);
> 
> oops. and to nit my own comment: the "more constant" argument usually comes
> first in an EXPECT_EQ

Done.

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/cocoa/apps_s...
ui/app_list/cocoa/apps_search_box_controller_unittest.mm:174: // Test the popup
menu items.
On 2013/09/16 20:46:44, tapted wrote:
> nit: ... for the multi-profile case.

Done.

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/test/app_lis...
File ui/app_list/test/app_list_test_view_delegate.h (right):

https://codereview.chromium.org/20656002/diff/436001/ui/app_list/test/app_lis...
ui/app_list/test/app_list_test_view_delegate.h:29: virtual void
SetProfileByPath(const base::FilePath& profile_path) OVERRIDE {};
On 2013/09/16 20:46:44, tapted wrote:
> stray semicolon at the end of this line

Done.

[calamity, 2013-09-19 04:22:03]

Fixed windows 8 ash. Removed binaries in order to run try jobs.

[tapted, 2013-09-19 06:03:19]

lgtm with the correct assets+grd after bots have run, and the changes below

I think you can add crbugs http://crbug.com/289872 http://crbug.com/258729 and
http://crbug.com/290048 to the list of things "Fixed" but this patch (but check
yourself before adding to BUG=).

http://crbug.com/272321 might need a follow-up after this too.

In the CL description,
> * Removed custom menu items and views from app_list_menu_views.cc 

Also mention that the Mac custom menu item view is being removed.

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

https://codereview.chromium.org/20656002/diff/451001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:114: if
(chrome::GetActiveDesktop() == chrome::HOST_DESKTOP_TYPE_ASH)
GetActiveDesktop is recommended as a last-resort. You should use
`chrome::GetHostDesktopTypeForNativeWindow(controller_->GetAppListWindow())`
like for OpenHelp, etc., below.

https://codereview.chromium.org/20656002/diff/451001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:124: void
AppListViewDelegate::SetProfileByPath(const base::FilePath& profile_path) {
If true (pretty sure it is!), I think this method needs a comment to say it is
only called on non-Ash Windows. Not sure there's a nice way to move it to
app_list_controller_win.cc, but there may be benefit keeping the code here
anyway, to keep it cohesive with the (cross-platform) Init steps. Maybe
something to investigate in a follow-up.

[calamity, 2013-09-19 08:15:24]

+oshima for ash/ and ui/resources/ OWNERS

@tapted: added bugs to CL description.

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

https://codereview.chromium.org/20656002/diff/451001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:114: if
(chrome::GetActiveDesktop() == chrome::HOST_DESKTOP_TYPE_ASH)
On 2013/09/19 06:03:21, tapted wrote:
> GetActiveDesktop is recommended as a last-resort. You should use
> `chrome::GetHostDesktopTypeForNativeWindow(controller_->GetAppListWindow())`
> like for OpenHelp, etc., below.

Done.

https://codereview.chromium.org/20656002/diff/451001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:124: void
AppListViewDelegate::SetProfileByPath(const base::FilePath& profile_path) {
On 2013/09/19 06:03:21, tapted wrote:
> If true (pretty sure it is!), I think this method needs a comment to say it is
> only called on non-Ash Windows. Not sure there's a nice way to move it to
> app_list_controller_win.cc, but there may be benefit keeping the code here
> anyway, to keep it cohesive with the (cross-platform) Init steps. Maybe
> something to investigate in a follow-up.

Added comment to ui/app_list/app_list_view_delegate.h.

[oshima, 2013-09-19 15:29:40]

ash/ ui/resources lgtm

[koz (OOO until 15th September), 2013-09-19 20:48:38]

lgtm

This is awesome, Chris! Thanks for persevering here.

[benwells, 2013-09-20 00:24:33]

lgtm.

Great work Chris!

[commit-bot: I haz the power, 2013-09-20 01:14:57]

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

[commit-bot: I haz the power, 2013-09-20 08:34:30]

Change committed as 224330