[win] Initialize the app list menu lazily, and allow invalidation.

[win] Initialize the app list menu lazily, and allow invalidation.

Currently the app launcher will show the old profile name, and no login
identity, after signing in for the first time, until the browser is
restarted (or the app list profile is switched).

This CL makes populating of the app list menu lazy, and allows the items
to be invalidated when the signin status changes. Simply setting the
CURRENT_USER menu item to be 'dynamic' is insufficient, because the menu
item uses a custom view which needs to be released and re-appended.

BUG=262639
TEST=Have Chrome not signed in, open a browser. Open app launcher, see
signin page, and sign in, leave browser window open. Reopen app
launcher, open menu: it should show email of the just-signed-in user.

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

[tapted, 2013-07-23 01:06:41]

Ben, could you take a look? I think you are most familiar with these waters..

[benwells, 2013-07-23 01:10:43]

https://codereview.chromium.org/19675010/diff/7001/ui/app_list/views/app_list...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/19675010/diff/7001/ui/app_list/views/app_list...
ui/app_list/views/app_list_main_view.cc:93: search_box_view_ = new
SearchBoxView(this);
Should you update the menu here?

[tapted, 2013-07-23 01:16:47]

https://codereview.chromium.org/19675010/diff/7001/ui/app_list/views/app_list...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/19675010/diff/7001/ui/app_list/views/app_list...
ui/app_list/views/app_list_main_view.cc:93: search_box_view_ = new
SearchBoxView(this);
On 2013/07/23 01:10:43, benwells wrote:
> Should you update the menu here?

I was doing this in patchset 1, but realised it would always be reset again,
because AppListView::InitAsBubble(..) calls OnSigninStatusChanged()

[benwells, 2013-07-23 01:23:26]

https://codereview.chromium.org/19675010/diff/7001/ui/app_list/views/app_list...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/19675010/diff/7001/ui/app_list/views/app_list...
ui/app_list/views/app_list_main_view.cc:93: search_box_view_ = new
SearchBoxView(this);
On 2013/07/23 01:16:47, tapted wrote:
> On 2013/07/23 01:10:43, benwells wrote:
> > Should you update the menu here?
> 
> I was doing this in patchset 1, but realised it would always be reset again,
> because AppListView::InitAsBubble(..) calls OnSigninStatusChanged()

I'd prefer not to have distant parts of the code rely on each other like that,
it's the kind of thing that can change easily and cause unexpected problems.

[tapted, 2013-07-23 03:37:57]

PTAL - this makes the menu population lazy, making its initialization more akin
to per-item context menus. I thought, "well, rather than create the
CurrentUserMenuItem twice during Init(), let's create it 0 times!". It has the
following advantages:
 - Work to create the menu is not required unless the menu is actually clicked
 - CrOS does not get saddled with a populated menu it can never show
 - Partial mitigation of http://crbug.com/262863 -- i.e. so long as the profile
rename (e.g. via first-time Sync) ocurrs before the menu is first shown, the
displayed profile name will be correct.
 - Makes the action of invalidating very lightweight, if the menu has never been
shown.

[benwells, 2013-07-23 03:58:57]

I like the laziness, but I'm not sure about the change to the delegate
structure.

Not that I don't like it, just not sure. I find the delegation confusing and
it's been a while since I looked at it, I'm worried adding more indirection here
might make things a little cleaner (i.e. one argument instead of two for the
constructor) but more confusing overall. Adding koz to get his opinion on that.

[tapted, 2013-07-23 04:13:47]

On 2013/07/23 03:58:57, benwells wrote:
> I like the laziness, but I'm not sure about the change to the delegate
> structure.
> 
> Not that I don't like it, just not sure. I find the delegation confusing and
> it's been a while since I looked at it, I'm worried adding more indirection
here
> might make things a little cleaner (i.e. one argument instead of two for the
> constructor) but more confusing overall. Adding koz to get his opinion on
that.

As another data point, it is also closer to the structure for the OSX app list
:D --
https://code.google.com/p/chromium/codesearch#chromium/src/ui/app_list/cocoa/...

However, although it was in an early CL, the menu on OSX is not lazy, because:
reasons.

[tapted, 2013-07-24 01:50:48]

PTAL - this now keeps both delegates in SearchBoxView

[koz (OOO until 15th September), 2013-07-24 02:57:36]

lgtm

[benwells, 2013-07-25 00:59:10]

lgtm

[commit-bot: I haz the power, 2013-07-25 01:17:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/19675010/43001

[commit-bot: I haz the power, 2013-07-25 16:34:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/19675010/58001

[commit-bot: I haz the power, 2013-07-26 00:45:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/19675010/58001

[commit-bot: I haz the power, 2013-07-26 08:49:00]

Change committed as 213807