Move signin status and current user information into AppListModel.

Move signin status and current user information into AppListModel.

For increased separation between model and view, the AppListViewDelegate
now updates the model on profile and signin status change which then
notifies the AppListView of changes so it can update.

BUG=
TEST=None
TBR=sky@chromium.org

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

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

lgtm

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

https://codereview.chromium.org/22268009/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.cc:221: void
AppListViewDelegate::UpdateModelWithCurrentProfiles() {
I think this would be better named OnProfileChanged().

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/app_list_model.cc
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/app_list_model...
ui/app_list/app_list_model.cc:49: OnAppListModelProfilesChanged());
OnCurrentUserChanged()?

[tapted, 2013-08-06 09:27:50]

Are some mac changes required?

Make sure you add a BUG= and TEST= line (even if it's just TEST=None or "No
functional changes")

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

https://codereview.chromium.org/22268009/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.cc:67:
signin_delegate_->RemoveObserver(this);
SetModel has a check: `if (signin_delegate_.get())`. Is that needed here as
well?

https://codereview.chromium.org/22268009/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.cc:72: registrar_.RemoveAll();
It looks like this (private function) is only called from the constructor, so
this line is not needed. And, in the current form, it looks like the rest can
just go inline in the constructor, which is the typical pattern for registrar_
foo

https://codereview.chromium.org/22268009/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.cc:221: void
AppListViewDelegate::UpdateModelWithCurrentProfiles() {
nit: it's usually best to follow declaration order -- i.e. put this function up
around line 82.

https://codereview.chromium.org/22268009/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.cc:230: return;
nit: blank line after early return

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/app_list_model.cc
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/app_list_model...
ui/app_list/app_list_model.cc:19: signed_in_(false) {
This should be initialized before `status_` to match declaration order..
(there's probably a compiler warning floating around about this)

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/app_list_model...
File ui/app_list/app_list_model_observer.h (right):

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/app_list_model...
ui/app_list/app_list_model_observer.h:15: virtual void
OnAppListModelStatusChanged() {}
Observers are usually all pure-virtual, otherwise it raises questions about
multiple-inheritance, which is typically avoided in Chrome. I think these should
stay pure virtual.

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/app_list_model...
ui/app_list/app_list_model_observer.h:20: protected:
nit: blank line before protected:

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/views/app_list...
File ui/app_list/views/app_list_menu_views.cc (right):

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/views/app_list...
ui/app_list/views/app_list_menu_views.cc:124: AppListModel* app_list_model_; //
Weak. Owned by AppListView.
nit: two spaces before // comment

[xiyuan, 2013-08-06 17:04:53]

+skuhne fyi

I'd like to take another look after comments from tapted and koz are addressed.

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/views/search_b...
File ui/app_list/views/search_box_view.h (right):

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/views/search_b...
ui/app_list/views/search_box_view.h:25: class AppListModel;
nit: keep the list sorted.

[calamity, 2013-08-08 01:34:57]

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

https://codereview.chromium.org/22268009/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.cc:67:
signin_delegate_->RemoveObserver(this);
On 2013/08/06 09:27:50, tapted wrote:
> SetModel has a check: `if (signin_delegate_.get())`. Is that needed here as
> well?

Done.

https://codereview.chromium.org/22268009/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.cc:72: registrar_.RemoveAll();
On 2013/08/06 09:27:50, tapted wrote:
> It looks like this (private function) is only called from the constructor, so
> this line is not needed. And, in the current form, it looks like the rest can
> just go inline in the constructor, which is the typical pattern for registrar_
> foo

Done.

https://codereview.chromium.org/22268009/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.cc:221: void
AppListViewDelegate::UpdateModelWithCurrentProfiles() {
On 2013/08/06 09:27:50, tapted wrote:
> nit: it's usually best to follow declaration order -- i.e. put this function
up
> around line 82.

Done.

https://codereview.chromium.org/22268009/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.cc:221: void
AppListViewDelegate::UpdateModelWithCurrentProfiles() {
On 2013/08/06 08:14:42, koz wrote:
> I think this would be better named OnProfileChanged().

Done.

https://codereview.chromium.org/22268009/diff/6001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_view_delegate.cc:230: return;
On 2013/08/06 09:27:50, tapted wrote:
> nit: blank line after early return

Done.

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/app_list_model.cc
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/app_list_model...
ui/app_list/app_list_model.cc:19: signed_in_(false) {
On 2013/08/06 09:27:50, tapted wrote:
> This should be initialized before `status_` to match declaration order..
> (there's probably a compiler warning floating around about this)

Done.

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/app_list_model...
ui/app_list/app_list_model.cc:49: OnAppListModelProfilesChanged());
On 2013/08/06 08:14:42, koz wrote:
> OnCurrentUserChanged()?

Done.

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/app_list_model...
File ui/app_list/app_list_model_observer.h (right):

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/app_list_model...
ui/app_list/app_list_model_observer.h:15: virtual void
OnAppListModelStatusChanged() {}
On 2013/08/06 09:27:50, tapted wrote:
> Observers are usually all pure-virtual, otherwise it raises questions about
> multiple-inheritance, which is typically avoided in Chrome. I think these
should
> stay pure virtual.

This was discussed in https://codereview.chromium.org/16035013/. It seems to
come down to convenience vs purity. My preference is to avoid unnecessary empty
methods in inheritors but I'm willing to have it either way.

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/app_list_model...
ui/app_list/app_list_model_observer.h:20: protected:
On 2013/08/06 09:27:50, tapted wrote:
> nit: blank line before protected:

Done.

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/views/app_list...
File ui/app_list/views/app_list_menu_views.cc (right):

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/views/app_list...
ui/app_list/views/app_list_menu_views.cc:124: AppListModel* app_list_model_; //
Weak. Owned by AppListView.
On 2013/08/06 09:27:50, tapted wrote:
> nit: two spaces before // comment

Done.

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/views/search_b...
File ui/app_list/views/search_box_view.h (right):

https://codereview.chromium.org/22268009/diff/6001/ui/app_list/views/search_b...
ui/app_list/views/search_box_view.h:25: class AppListModel;
On 2013/08/06 17:04:54, xiyuan wrote:
> nit: keep the list sorted.

Done.

[xiyuan, 2013-08-08 01:52:05]

LGTM with nits

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

https://codereview.chromium.org/22268009/diff/17001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.cc:87: if (profile_index ==
std::string::npos)
Do you need to reset the current user info when this happens?

https://codereview.chromium.org/22268009/diff/17001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.cc:232: void
AppListViewDelegate::OnSigninSuccess() {
What's the difference between OnSigninSuccess and
NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL? Do we need both?

https://codereview.chromium.org/22268009/diff/17001/ui/app_list/app_list_mode...
File ui/app_list/app_list_model_observer.h (right):

https://codereview.chromium.org/22268009/diff/17001/ui/app_list/app_list_mode...
ui/app_list/app_list_model_observer.h:16: // Invoked when AppListModel's current
user details have changed.
nit: insert an empty line between the methods.

https://codereview.chromium.org/22268009/diff/17001/ui/app_list/app_list_mode...
ui/app_list/app_list_model_observer.h:18: // Invoked when AppListModel's current
user's signin status has changed.
nit: insert empty line here too

[calamity, 2013-08-08 06:29:40]

Also removed OnSigninStatusChanged from AppListServiceImpl since the signin
status is now managed by AppListViewDelegate.

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

https://chromiumcodereview.appspot.com/22268009/diff/17001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:87: if (profile_index ==
std::string::npos)
On 2013/08/08 01:52:06, xiyuan wrote:
> Do you need to reset the current user info when this happens?

Nope. When the app list is next shown, the last used profile is loaded which
will repopulate the current user details.

https://chromiumcodereview.appspot.com/22268009/diff/17001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:232: void
AppListViewDelegate::OnSigninSuccess() {
On 2013/08/08 01:52:06, xiyuan wrote:
> What's the difference between OnSigninSuccess and
> NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL? Do we need both?

Removed the notifications.

https://chromiumcodereview.appspot.com/22268009/diff/17001/ui/app_list/app_li...
File ui/app_list/app_list_model_observer.h (right):

https://chromiumcodereview.appspot.com/22268009/diff/17001/ui/app_list/app_li...
ui/app_list/app_list_model_observer.h:16: // Invoked when AppListModel's current
user details have changed.
On 2013/08/08 01:52:06, xiyuan wrote:
> nit: insert an empty line between the methods.

Done.

https://chromiumcodereview.appspot.com/22268009/diff/17001/ui/app_list/app_li...
ui/app_list/app_list_model_observer.h:18: // Invoked when AppListModel's current
user's signin status has changed.
On 2013/08/08 01:52:06, xiyuan wrote:
> nit: insert empty line here too

Done.

[calamity, 2013-08-13 08:16:41]

Implemented this for mac, also added early return on setting model properties to
avoid unnecessary notifications.

[tapted, 2013-08-14 03:53:11]

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

https://codereview.chromium.org/22268009/diff/60001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.cc:27: #include
"content/public/browser/notification_details.h"
nit: content::NotificationSource and content::NotificationDetails look like they
can just be forward-decs (probably in the header file, since they're used there
first).

https://codereview.chromium.org/22268009/diff/60001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.cc:71: if
(signin_delegate_.get())
nit: the `.get()` is usually dropped when used as a condition. Another in
SetModel.

https://codereview.chromium.org/22268009/diff/60001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.cc:111: } else {
I think while this else is still around, it also needs a
signin_delegate_.reset()

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

https://codereview.chromium.org/22268009/diff/60001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.h:49: void
RegisterForNotifications();
this function isn't defined any more

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/app_list_mode...
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/app_list_mode...
ui/app_list/app_list_model.cc:58: return;
nit: blank line after early return

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/cocoa/app_lis...
File ui/app_list/cocoa/app_list_view_controller.mm (right):

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/cocoa/app_lis...
ui/app_list/cocoa/app_list_view_controller.mm:78: -
(app_list::SigninDelegate*)signinDelegate;
Remove this function?

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/cocoa/app_lis...
ui/app_list/cocoa/app_list_view_controller.mm:95: AppListModel* model_;
prevailing style on the Cocoa side is to not cache things you can infer from
other members. You can get to the model reliably with

[[parent_ appsGridController] model]

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/cocoa/app_lis...
ui/app_list/cocoa/app_list_view_controller.mm:357: - (const
base::string16&)currentUserName {
nit: These should be grouped with the other protocol methods - i.e. after
modelTextDidChange

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/cocoa/apps_se...
File ui/app_list/cocoa/apps_search_box_controller.h (right):

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/cocoa/apps_se...
ui/app_list/cocoa/apps_search_box_controller.h:12: #include
"base/strings/sys_string_conversions.h"
this shouldn't be in the header, but you'll probably need it in
app_list_view_controller.mm with the comment below

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/cocoa/apps_se...
ui/app_list/cocoa/apps_search_box_controller.h:31: - (const
base::string16&)currentUserName;
Objective-C functions "can't" return references.. (they kinda can, but it breaks
some assumptions). Probably nicest is if these return an NSString* instead, and
the conversion is done in the AppListViewController

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/cocoa/apps_se...
File ui/app_list/cocoa/apps_search_box_controller.mm (right):

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/cocoa/apps_se...
ui/app_list/cocoa/apps_search_box_controller.mm:13: #include
"ui/app_list/app_list_model.h"
I think you can get by without changing #includes

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/cocoa/current...
File ui/app_list/cocoa/current_user_menu_item_view.h (right):

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.h:21: -
(id)initWithCurrentUser:(const base::string16&)userName
(so this will take NSString* arguments with earlier comments)

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/cocoa/current...
File ui/app_list/cocoa/current_user_menu_item_view.mm (right):

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.mm:7: #include "base/logging.h"
nit: I think this can go too, with the DCHECK being deleted

https://codereview.chromium.org/22268009/diff/60001/ui/app_list/cocoa/current...
ui/app_list/cocoa/current_user_menu_item_view.mm:64: labelText:(const
base::string16&)labelText {
NSString* for everyone!

[tfarina, 2013-08-14 03:56:59]

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

https://codereview.chromium.org/22268009/diff/60001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_view_delegate.cc:64: registrar_.Add(this,
chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
do you need to use notifications? we are moving away from it. see recent thread
on chromium-dev about this.

[calamity, 2013-08-14 09:18:59]

ProfileInfoCacheObserver change split into
https://chromiumcodereview.appspot.com/23179002/ .

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

https://chromiumcodereview.appspot.com/22268009/diff/60001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:27: #include
"content/public/browser/notification_details.h"
On 2013/08/14 03:53:11, tapted wrote:
> nit: content::NotificationSource and content::NotificationDetails look like
they
> can just be forward-decs (probably in the header file, since they're used
there
> first).

Done. notification_source.h is needed for content::Source.

https://chromiumcodereview.appspot.com/22268009/diff/60001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:64: registrar_.Add(this,
chrome::NOTIFICATION_PROFILE_CACHED_INFO_CHANGED,
On 2013/08/14 03:57:00, tfarina wrote:
> do you need to use notifications? we are moving away from it. see recent
thread
> on chromium-dev about this.

I'll split out the NOTIFICATION_PROFILE_CACHED_INFO_CHANGED change into another
CL since it's actually an orthogonal issue anyway, and use
ProfileInfoCacheObserver there.

After looking at this again, it looks like the other signin notifications are
still needed here. There's no observer equivalent for these events.

https://chromiumcodereview.appspot.com/22268009/diff/60001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:71: if
(signin_delegate_.get())
On 2013/08/14 03:53:11, tapted wrote:
> nit: the `.get()` is usually dropped when used as a condition. Another in
> SetModel.

Done.

https://chromiumcodereview.appspot.com/22268009/diff/60001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_view_delegate.cc:111: } else {
On 2013/08/14 03:53:11, tapted wrote:
> I think while this else is still around, it also needs a
> signin_delegate_.reset()

Done.

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

https://chromiumcodereview.appspot.com/22268009/diff/60001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_view_delegate.h:49: void
RegisterForNotifications();
On 2013/08/14 03:53:11, tapted wrote:
> this function isn't defined any more

Done.

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/app_li...
File ui/app_list/app_list_model.cc (right):

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/app_li...
ui/app_list/app_list_model.cc:58: return;
On 2013/08/14 03:53:11, tapted wrote:
> nit: blank line after early return

Done.

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/cocoa/...
File ui/app_list/cocoa/app_list_view_controller.mm (right):

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/cocoa/...
ui/app_list/cocoa/app_list_view_controller.mm:78: -
(app_list::SigninDelegate*)signinDelegate;
On 2013/08/14 03:53:11, tapted wrote:
> Remove this function?

Done.

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/cocoa/...
ui/app_list/cocoa/app_list_view_controller.mm:95: AppListModel* model_;
On 2013/08/14 03:53:11, tapted wrote:
> prevailing style on the Cocoa side is to not cache things you can infer from
> other members. You can get to the model reliably with
> 
> [[parent_ appsGridController] model]

Done.

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/cocoa/...
ui/app_list/cocoa/app_list_view_controller.mm:357: - (const
base::string16&)currentUserName {
On 2013/08/14 03:53:11, tapted wrote:
> nit: These should be grouped with the other protocol methods - i.e. after
> modelTextDidChange

Done.

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/cocoa/...
File ui/app_list/cocoa/apps_search_box_controller.h (right):

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/cocoa/...
ui/app_list/cocoa/apps_search_box_controller.h:12: #include
"base/strings/sys_string_conversions.h"
On 2013/08/14 03:53:11, tapted wrote:
> this shouldn't be in the header, but you'll probably need it in
> app_list_view_controller.mm with the comment below

Done.

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/cocoa/...
ui/app_list/cocoa/apps_search_box_controller.h:31: - (const
base::string16&)currentUserName;
On 2013/08/14 03:53:11, tapted wrote:
> Objective-C functions "can't" return references.. (they kinda can, but it
breaks
> some assumptions). Probably nicest is if these return an NSString* instead,
and
> the conversion is done in the AppListViewController

Done.

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/cocoa/...
File ui/app_list/cocoa/apps_search_box_controller.mm (right):

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/cocoa/...
ui/app_list/cocoa/apps_search_box_controller.mm:13: #include
"ui/app_list/app_list_model.h"
On 2013/08/14 03:53:11, tapted wrote:
> I think you can get by without changing #includes

Done.

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/cocoa/...
File ui/app_list/cocoa/current_user_menu_item_view.h (right):

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/cocoa/...
ui/app_list/cocoa/current_user_menu_item_view.h:21: -
(id)initWithCurrentUser:(const base::string16&)userName
On 2013/08/14 03:53:11, tapted wrote:
> (so this will take NSString* arguments with earlier comments)

Done.

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/cocoa/...
File ui/app_list/cocoa/current_user_menu_item_view.mm (right):

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/cocoa/...
ui/app_list/cocoa/current_user_menu_item_view.mm:7: #include "base/logging.h"
On 2013/08/14 03:53:11, tapted wrote:
> nit: I think this can go too, with the DCHECK being deleted

Done.

https://chromiumcodereview.appspot.com/22268009/diff/60001/ui/app_list/cocoa/...
ui/app_list/cocoa/current_user_menu_item_view.mm:64: labelText:(const
base::string16&)labelText {
On 2013/08/14 03:53:11, tapted wrote:
> NSString* for everyone!

Done.

[tapted, 2013-08-14 10:57:59]

nice! I missed one (sorry!) but could you also rebase? Then we can run some bots
with `git cl try -bmac{,_rel,_asan}{,:app_list_unittests}`. I think it all looks
good, but some tests might need updating...

https://chromiumcodereview.appspot.com/22268009/diff/86001/ui/app_list/cocoa/...
File ui/app_list/cocoa/current_user_menu_item_view.h (right):

https://chromiumcodereview.appspot.com/22268009/diff/86001/ui/app_list/cocoa/...
ui/app_list/cocoa/current_user_menu_item_view.h:13: class AppListViewDelegate;
nit: this forward dec isn't needed any more.

[calamity, 2013-08-16 01:44:16]

Fixed tests.

https://chromiumcodereview.appspot.com/22268009/diff/86001/ui/app_list/cocoa/...
File ui/app_list/cocoa/current_user_menu_item_view.h (right):

https://chromiumcodereview.appspot.com/22268009/diff/86001/ui/app_list/cocoa/...
ui/app_list/cocoa/current_user_menu_item_view.h:13: class AppListViewDelegate;
On 2013/08/14 10:57:59, tapted wrote:
> nit: this forward dec isn't needed any more.

Done.

[tapted, 2013-08-16 05:09:13]

https://chromiumcodereview.appspot.com/22268009/diff/122001/ui/app_list/cocoa...
File ui/app_list/cocoa/app_list_view_controller_unittest.mm (right):

https://chromiumcodereview.appspot.com/22268009/diff/122001/ui/app_list/cocoa...
ui/app_list/cocoa/app_list_view_controller_unittest.mm:118:
model->SetSignedIn(true);
I think this needs to be the default. Otherwise, things like the
AppListViewControllerTest.PagerChangingPage test above will just be showing a
signin page during the test - I checked ;). The problem is that Cocoa is smart
enough to optimize out all the calls to draw parts of the UI that are covered up
by opaque views. So, even though the unit test still passes, its coverage has
been reduced.

I think all you need is

AppListTestModel::AppListTestModel() {
  SetSignedIn(true);
}

in app_list_test_model.cc

https://chromiumcodereview.appspot.com/22268009/diff/122001/ui/app_list/cocoa...
ui/app_list/cocoa/app_list_view_controller_unittest.mm:132:
NotifySigninSuccess();
Thanks to your refactoring, I think this line is now unnecessary :)

[calamity, 2013-08-16 07:24:04]

Also fixed a bug that was introduced which was causing multiple signin views to
get created by onSigninStatusChanged.

https://chromiumcodereview.appspot.com/22268009/diff/122001/ui/app_list/cocoa...
File ui/app_list/cocoa/app_list_view_controller_unittest.mm (right):

https://chromiumcodereview.appspot.com/22268009/diff/122001/ui/app_list/cocoa...
ui/app_list/cocoa/app_list_view_controller_unittest.mm:118:
model->SetSignedIn(true);
On 2013/08/16 05:09:14, tapted wrote:
> I think this needs to be the default. Otherwise, things like the
> AppListViewControllerTest.PagerChangingPage test above will just be showing a
> signin page during the test - I checked ;). The problem is that Cocoa is smart
> enough to optimize out all the calls to draw parts of the UI that are covered
up
> by opaque views. So, even though the unit test still passes, its coverage has
> been reduced.
> 
> I think all you need is
> 
> AppListTestModel::AppListTestModel() {
>   SetSignedIn(true);
> }
> 
> in app_list_test_model.cc

Done.

https://chromiumcodereview.appspot.com/22268009/diff/122001/ui/app_list/cocoa...
ui/app_list/cocoa/app_list_view_controller_unittest.mm:132:
NotifySigninSuccess();
On 2013/08/16 05:09:14, tapted wrote:
> Thanks to your refactoring, I think this line is now unnecessary :)

Done.

[tapted, 2013-08-16 08:00:24]

lgtm

https://chromiumcodereview.appspot.com/22268009/diff/161001/ui/app_list/cocoa...
File ui/app_list/cocoa/app_list_view_controller.mm (right):

https://chromiumcodereview.appspot.com/22268009/diff/161001/ui/app_list/cocoa...
ui/app_list/cocoa/app_list_view_controller.mm:347: ![appsGridController_
model]->signed_in() && signinDelegate;
nit: might as well check signinDelegate first, since it's a lighter-weight check

https://chromiumcodereview.appspot.com/22268009/diff/161001/ui/app_list/cocoa...
ui/app_list/cocoa/app_list_view_controller.mm:348: if
(static_cast<bool>(signinViewController_) == show_signin_view)
nit: I think instead of static_cast, I see this done like

if (!!signinViewController_ == show_signin_view)

[calamity, 2013-08-16 09:12:46]

https://chromiumcodereview.appspot.com/22268009/diff/161001/ui/app_list/cocoa...
File ui/app_list/cocoa/app_list_view_controller.mm (right):

https://chromiumcodereview.appspot.com/22268009/diff/161001/ui/app_list/cocoa...
ui/app_list/cocoa/app_list_view_controller.mm:347: ![appsGridController_
model]->signed_in() && signinDelegate;
On 2013/08/16 08:00:25, tapted wrote:
> nit: might as well check signinDelegate first, since it's a lighter-weight
check

Done.

https://chromiumcodereview.appspot.com/22268009/diff/161001/ui/app_list/cocoa...
ui/app_list/cocoa/app_list_view_controller.mm:348: if
(static_cast<bool>(signinViewController_) == show_signin_view)
On 2013/08/16 08:00:25, tapted wrote:
> nit: I think instead of static_cast, I see this done like
> 
> if (!!signinViewController_ == show_signin_view)

Done.

[commit-bot: I haz the power, 2013-08-16 09:37:07]

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

[commit-bot: I haz the power, 2013-08-16 10:12:52]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[calamity, 2013-08-19 05:59:57]

TBRing sky@ for ash/shell/app_list.cc .

[commit-bot: I haz the power, 2013-08-19 06:00:19]

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

[commit-bot: I haz the power, 2013-08-19 06:21:38]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[calamity, 2013-08-19 07:14:28]

Actually add sky@ to review.

sky@: TBR for ash/shell/app_list.cc

[commit-bot: I haz the power, 2013-08-19 07:14:56]

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

[commit-bot: I haz the power, 2013-08-19 10:33:13]

Change committed as 218251

[sky, 2013-08-19 15:54:18]

LGTM