chromeos: Do not create the profile too early.

chromeos: Do not create the profile too early for the uber tray.

BUG=chromium-os:28149
TEST=manually

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

[sadrul, 2012-03-21 23:35:23]

Hi! Some details:

It's not a good idea to try to get the default profile at start (see bug). So
instead of that, we listen to the PROFILE_CREATED notification. Once we get the
first notification like this, we stop listening for this notification (this
probably isn't necessary).

We continue to listen to SESESION_STARTED as before.

Does this look like a reasonable approach?

[Ivan Korotkov, 2012-03-22 11:43:04]

LGTM
Is there any test covering this?

[sadrul, 2012-03-22 11:50:58]

On 2012/03/22 11:43:04, Ivan Korotkov wrote:
> LGTM
> Is there any test covering this?

Thanks! The memory bots caught this, I am hoping this would be covered by it.

Having said that, does it make sense to add a check after the call to
PreProfileInit to make sure that no profile has been created yet?

[sadrul, 2012-03-22 12:05:48]

On 2012/03/22 11:50:58, sadrul wrote:
> On 2012/03/22 11:43:04, Ivan Korotkov wrote:
> > LGTM
> > Is there any test covering this?
> 
> Thanks! The memory bots caught this, I am hoping this would be covered by it.
> 
> Having said that, does it make sense to add a check after the call to
> PreProfileInit to make sure that no profile has been created yet?

Turns out there is another bug in the launcher code where it creates the profile
too early. So we cannot add this check just yet. But this change is still good,
we do not want to end up creating a profile here.

[sadrul, 2012-03-22 12:05:59]

+ben for OWNERS

[Nikita (slow), 2012-03-22 12:10:49]

lgtm

http://codereview.chromium.org/9812026/diff/3001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9812026/diff/3001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:127:
registrar_.Add(this,
Ok, so there's one more case when you should better not register for session
started notification.
That's the case of relaunching Chrome on existing user session:
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/chr...

In that case NOTIFICATION_PROFILE_CREATED handler would pick up correct user
profile and in fact SESSION_STARTED won't be handled as it is fired in 
ChromeBrowserMainPartsChromeos::PreProfileInit() i.e. before
ChromeBrowserMainExtraPartsAsh::PreProfileInit() which would create
SystemTrayDelegate.

But still you should register for SESSION_STARTED notification only if
GetUserLoginStatus() == ash::user::LOGGED_IN_NONE.

[sadrul, 2012-03-22 12:17:44]

http://codereview.chromium.org/9812026/diff/3001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9812026/diff/3001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:127:
registrar_.Add(this,
On 2012/03/22 12:10:49, Nikita Kostylev wrote:
> Ok, so there's one more case when you should better not register for session
> started notification.
> That's the case of relaunching Chrome on existing user session:
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/chr...
> 
> In that case NOTIFICATION_PROFILE_CREATED handler would pick up correct user
> profile and in fact SESSION_STARTED won't be handled as it is fired in 
> ChromeBrowserMainPartsChromeos::PreProfileInit() i.e. before
> ChromeBrowserMainExtraPartsAsh::PreProfileInit() which would create
> SystemTrayDelegate.
> 
> But still you should register for SESSION_STARTED notification only if
> GetUserLoginStatus() == ash::user::LOGGED_IN_NONE.

Ah, interesting. Thanks for the detailed explanation. I have made the change to
listen to SESSION_STARTED only when LOGGED_IN_NONE.

[Nikita (slow), 2012-03-22 12:28:03]

On 2012/03/22 12:17:44, sadrul wrote:
>
http://codereview.chromium.org/9812026/diff/3001/chrome/browser/chromeos/syst...
> File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):
> 
>
http://codereview.chromium.org/9812026/diff/3001/chrome/browser/chromeos/syst...
> chrome/browser/chromeos/system/ash_system_tray_delegate.cc:127:
> registrar_.Add(this,
> On 2012/03/22 12:10:49, Nikita Kostylev wrote:
> > Ok, so there's one more case when you should better not register for session
> > started notification.
> > That's the case of relaunching Chrome on existing user session:
> >
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/chr...
> > 
> > In that case NOTIFICATION_PROFILE_CREATED handler would pick up correct user
> > profile and in fact SESSION_STARTED won't be handled as it is fired in 
> > ChromeBrowserMainPartsChromeos::PreProfileInit() i.e. before
> > ChromeBrowserMainExtraPartsAsh::PreProfileInit() which would create
> > SystemTrayDelegate.
> > 
> > But still you should register for SESSION_STARTED notification only if
> > GetUserLoginStatus() == ash::user::LOGGED_IN_NONE.
> 
> Ah, interesting. Thanks for the detailed explanation. I have made the change
to
> listen to SESSION_STARTED only when LOGGED_IN_NONE.

Yes it seems that we may somehome need to cleanup/comment these notification to
avoid confusion.

[Ben Goodger (Google), 2012-03-22 14:56:42]

lgtm