Use OffTheRecord profile in case of GuestLogin for AppList.

Use OffTheRecord profile in case of GuestLogin for AppList.


BUG=117915
TEST=manually

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

[sky, 2012-03-13 16:04:11]

http://codereview.chromium.org/9691032/diff/1/chrome/browser/ui/views/ash/app...
File chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc (right):

http://codereview.chromium.org/9691032/diff/1/chrome/browser/ui/views/ash/app...
chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc:26: if
(chromeos::UserManager::Get()->IsLoggedInAsGuest())
I think you need the same logic as is in ChromeLauncherDelegate and other
places. That said, this code is easy to get wrong and we need a better solution
that doesn't require people remembering to special case this. Let me talk with
Dave to see if there is a better way to fix this more centrally.

[xiyuan, 2012-03-13 17:22:02]

http://codereview.chromium.org/9691032/diff/1/chrome/browser/ui/views/ash/app...
File chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc (right):

http://codereview.chromium.org/9691032/diff/1/chrome/browser/ui/views/ash/app...
chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc:26: if
(chromeos::UserManager::Get()->IsLoggedInAsGuest())
On 2012/03/13 16:04:12, sky wrote:
> I think you need the same logic as is in ChromeLauncherDelegate and other
> places. That said, this code is easy to get wrong and we need a better
solution
> that doesn't require people remembering to special case this. Let me talk with
> Dave to see if there is a better way to fix this more centrally.

ProfileManager::GetDefaultProfile already has code to return correct profile
regarding whether user is logged in or not. Maybe patch that up to return the
incognito one for guest user?

[sky, 2012-03-13 17:43:18]

On Tue, Mar 13, 2012 at 10:22 AM,  <xiyuan@chromium.org> wrote:
>
>
http://codereview.chromium.org/9691032/diff/1/chrome/browser/ui/views/ash/app...
> File chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc
> (right):
>
>
http://codereview.chromium.org/9691032/diff/1/chrome/browser/ui/views/ash/app...
> chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc:26: if
> (chromeos::UserManager::Get()->IsLoggedInAsGuest())
> On 2012/03/13 16:04:12, sky wrote:
>>
>> I think you need the same logic as is in ChromeLauncherDelegate and
>
> other
>>
>> places. That said, this code is easy to get wrong and we need a better
>
> solution
>>
>> that doesn't require people remembering to special case this. Let me
>
> talk with
>>
>> Dave to see if there is a better way to fix this more centrally.
>
>
> ProfileManager::GetDefaultProfile already has code to return correct
> profile regarding whether user is logged in or not. Maybe patch that up
> to return the incognito one for guest user?
>
> http://codereview.chromium.org/9691032/

I prefer that too, but I'm not sure if there's a reason Dave didn't do
this. I'm going to speak with Dave about this later today.

  -Scott

[Nikita (slow), 2012-03-15 10:39:12]

On 2012/03/13 17:43:18, sky wrote:
> On Tue, Mar 13, 2012 at 10:22 AM,  <mailto:xiyuan@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/9691032/diff/1/chrome/browser/ui/views/ash/app...
> > File chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc
> > (right):
> >
> >
>
http://codereview.chromium.org/9691032/diff/1/chrome/browser/ui/views/ash/app...
> > chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc:26: if
> > (chromeos::UserManager::Get()->IsLoggedInAsGuest())
> > On 2012/03/13 16:04:12, sky wrote:
> >>
> >> I think you need the same logic as is in ChromeLauncherDelegate and
> >
> > other
> >>
> >> places. That said, this code is easy to get wrong and we need a better
> >
> > solution
> >>
> >> that doesn't require people remembering to special case this. Let me
> >
> > talk with
> >>
> >> Dave to see if there is a better way to fix this more centrally.
> >
> >
> > ProfileManager::GetDefaultProfile already has code to return correct
> > profile regarding whether user is logged in or not. Maybe patch that up
> > to return the incognito one for guest user?
> >
> > http://codereview.chromium.org/9691032/
> 
> I prefer that too, but I'm not sure if there's a reason Dave didn't do
> this. I'm going to speak with Dave about this later today.

I don't see issues with moving this code into GetDefaultProfile instead.

Dave?

[Jun Mukai, 2012-03-15 11:08:51]

On 2012/03/15 10:39:12, Nikita Kostylev wrote:
> On 2012/03/13 17:43:18, sky wrote:
> > On Tue, Mar 13, 2012 at 10:22 AM,  <mailto:xiyuan@chromium.org> wrote:
> > >
> > >
> >
>
http://codereview.chromium.org/9691032/diff/1/chrome/browser/ui/views/ash/app...
> > > File chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc
> > > (right):
> > >
> > >
> >
>
http://codereview.chromium.org/9691032/diff/1/chrome/browser/ui/views/ash/app...
> > > chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc:26: if
> > > (chromeos::UserManager::Get()->IsLoggedInAsGuest())
> > > On 2012/03/13 16:04:12, sky wrote:
> > >>
> > >> I think you need the same logic as is in ChromeLauncherDelegate and
> > >
> > > other
> > >>
> > >> places. That said, this code is easy to get wrong and we need a better
> > >
> > > solution
> > >>
> > >> that doesn't require people remembering to special case this. Let me
> > >
> > > talk with
> > >>
> > >> Dave to see if there is a better way to fix this more centrally.
> > >
> > >
> > > ProfileManager::GetDefaultProfile already has code to return correct
> > > profile regarding whether user is logged in or not. Maybe patch that up
> > > to return the incognito one for guest user?
> > >
> > > http://codereview.chromium.org/9691032/
> > 
> > I prefer that too, but I'm not sure if there's a reason Dave didn't do
> > this. I'm going to speak with Dave about this later today.
> 
> I don't see issues with moving this code into GetDefaultProfile instead.
> 
> Dave?

Moved to profile_manager.

[Nikita (slow), 2012-03-15 11:13:50]

lgtm but please run trybots
Is Guest mode working fine with this fix?

[Jun Mukai, 2012-03-15 11:33:17]

On 2012/03/15 11:13:50, Nikita Kostylev wrote:
> lgtm but please run trybots
> Is Guest mode working fine with this fix?

Ouch, it's not working actually.  Please ignore patchset 3-4.

[Nikita (slow), 2012-03-15 11:42:51]

On 2012/03/15 11:33:17, mukai wrote:
> On 2012/03/15 11:13:50, Nikita Kostylev wrote:
> > lgtm but please run trybots
> > Is Guest mode working fine with this fix?
> 
> Ouch, it's not working actually.  Please ignore patchset 3-4.

Yes, we've also noticed that it's not correct way to do this.
What happens is that with Webstore / Filebrowser in Guest normal window is
opened (you could see by UI) which shouldn't happen at all for Guest mode.

This should be fixed.
If you see how New window in Wrench menu + shortcut works on Guest mode it
always opens incognito tab which will in result have incognito  profile
associated to it.

So more broad change i.e. always return incognito profile for GetDefaultProfile
in guest mode has yet to be evaluated whether or not it breaks things.

[Jun Mukai, 2012-03-15 11:53:33]

On 2012/03/15 11:42:51, Nikita Kostylev wrote:
> On 2012/03/15 11:33:17, mukai wrote:
> > On 2012/03/15 11:13:50, Nikita Kostylev wrote:
> > > lgtm but please run trybots
> > > Is Guest mode working fine with this fix?
> > 
> > Ouch, it's not working actually.  Please ignore patchset 3-4.
> 
> Yes, we've also noticed that it's not correct way to do this.
> What happens is that with Webstore / Filebrowser in Guest normal window is
> opened (you could see by UI) which shouldn't happen at all for Guest mode.
> 
> This should be fixed.
> If you see how New window in Wrench menu + shortcut works on Guest mode it
> always opens incognito tab which will in result have incognito  profile
> associated to it.
> 
> So more broad change i.e. always return incognito profile for
GetDefaultProfile
> in guest mode has yet to be evaluated whether or not it breaks things.

Hmmm...  Let me go back to patchset2 (put the code to app_list_view_delegate.cc)
for now.  It sounds the right thing, but is it essentially a part of Denis's
clean-up task you mentioned?

[Jun Mukai, 2012-03-15 12:08:16]

On 2012/03/15 11:53:33, mukai wrote:
> On 2012/03/15 11:42:51, Nikita Kostylev wrote:
> > On 2012/03/15 11:33:17, mukai wrote:
> > > On 2012/03/15 11:13:50, Nikita Kostylev wrote:
> > > > lgtm but please run trybots
> > > > Is Guest mode working fine with this fix?
> > > 
> > > Ouch, it's not working actually.  Please ignore patchset 3-4.
> > 
> > Yes, we've also noticed that it's not correct way to do this.
> > What happens is that with Webstore / Filebrowser in Guest normal window is
> > opened (you could see by UI) which shouldn't happen at all for Guest mode.
> > 
> > This should be fixed.
> > If you see how New window in Wrench menu + shortcut works on Guest mode it
> > always opens incognito tab which will in result have incognito  profile
> > associated to it.
> > 
> > So more broad change i.e. always return incognito profile for
> GetDefaultProfile
> > in guest mode has yet to be evaluated whether or not it breaks things.
> 
> Hmmm...  Let me go back to patchset2 (put the code to
app_list_view_delegate.cc)
> for now.  It sounds the right thing, but is it essentially a part of Denis's
> clean-up task you mentioned?

Note:
when I add the code to return OffTheRecord profile if logged-in as a guest (like
patchset 3-4), chrome will crash during chrome_browser_main_chromeos.  I didn't
look into this more, but I'm feeling that chrome is not ready to do so.  Some
modules seem to depend on profile is not OTR.

[Nikita (slow), 2012-03-15 12:11:29]

http://codereview.chromium.org/9691032/diff/11001/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc (right):

http://codereview.chromium.org/9691032/diff/11001/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc:30:
AppListModelBuilder builder(profile, model);
This results in calling BrowserList::GetLastActiveWithProfile().

Seems that for now till starting guest mode with OTR profile as default one is
works w/o crashes you should move this code to
BrowserList::GetLastActiveWithProfile so that we accidentally don't repeat this
bug.

[xiyuan, 2012-03-15 16:41:10]

On 2012/03/15 12:08:16, mukai wrote:
> On 2012/03/15 11:53:33, mukai wrote:
> > On 2012/03/15 11:42:51, Nikita Kostylev wrote:
> > > On 2012/03/15 11:33:17, mukai wrote:
> > > > On 2012/03/15 11:13:50, Nikita Kostylev wrote:
> > > > > lgtm but please run trybots
> > > > > Is Guest mode working fine with this fix?
> > > > 
> > > > Ouch, it's not working actually.  Please ignore patchset 3-4.
> > > 
> > > Yes, we've also noticed that it's not correct way to do this.
> > > What happens is that with Webstore / Filebrowser in Guest normal window is
> > > opened (you could see by UI) which shouldn't happen at all for Guest mode.
> > > 
> > > This should be fixed.
> > > If you see how New window in Wrench menu + shortcut works on Guest mode it
> > > always opens incognito tab which will in result have incognito  profile
> > > associated to it.
> > > 
> > > So more broad change i.e. always return incognito profile for
> > GetDefaultProfile
> > > in guest mode has yet to be evaluated whether or not it breaks things.
> > 
> > Hmmm...  Let me go back to patchset2 (put the code to
> app_list_view_delegate.cc)
> > for now.  It sounds the right thing, but is it essentially a part of Denis's
> > clean-up task you mentioned?
> 
> Note:
> when I add the code to return OffTheRecord profile if logged-in as a guest
(like
> patchset 3-4), chrome will crash during chrome_browser_main_chromeos.  I
didn't
> look into this more, but I'm feeling that chrome is not ready to do so.  Some
> modules seem to depend on profile is not OTR.

Patch #3 and #4 seems not correct. The code is added in a "if (!logged_in_)"
branch which is only executed for login manager (i.e. before login). We probably
should do the guest logic in a "else" branch of that "if".

[sky, 2012-03-15 16:49:45]

http://codereview.chromium.org/9691032/diff/11001/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc (right):

http://codereview.chromium.org/9691032/diff/11001/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc:27: if
(chromeos::UserManager::Get()->IsLoggedInAsGuest())
Can you use the same code as is in ChromeLauncherDelegate? That way you don't
need the ifdef. I suggest pulling the code out of ChromeLauncherDelegate putting
it into a static on ProfileManager.

[Jun Mukai, 2012-03-15 16:50:14]

On 2012/03/15 16:41:10, xiyuan wrote:
> On 2012/03/15 12:08:16, mukai wrote:
> > On 2012/03/15 11:53:33, mukai wrote:
> > > On 2012/03/15 11:42:51, Nikita Kostylev wrote:
> > > > On 2012/03/15 11:33:17, mukai wrote:
> > > > > On 2012/03/15 11:13:50, Nikita Kostylev wrote:
> > > > > > lgtm but please run trybots
> > > > > > Is Guest mode working fine with this fix?
> > > > > 
> > > > > Ouch, it's not working actually.  Please ignore patchset 3-4.
> > > > 
> > > > Yes, we've also noticed that it's not correct way to do this.
> > > > What happens is that with Webstore / Filebrowser in Guest normal window
is
> > > > opened (you could see by UI) which shouldn't happen at all for Guest
mode.
> > > > 
> > > > This should be fixed.
> > > > If you see how New window in Wrench menu + shortcut works on Guest mode
it
> > > > always opens incognito tab which will in result have incognito  profile
> > > > associated to it.
> > > > 
> > > > So more broad change i.e. always return incognito profile for
> > > GetDefaultProfile
> > > > in guest mode has yet to be evaluated whether or not it breaks things.
> > > 
> > > Hmmm...  Let me go back to patchset2 (put the code to
> > app_list_view_delegate.cc)
> > > for now.  It sounds the right thing, but is it essentially a part of
Denis's
> > > clean-up task you mentioned?
> > 
> > Note:
> > when I add the code to return OffTheRecord profile if logged-in as a guest
> (like
> > patchset 3-4), chrome will crash during chrome_browser_main_chromeos.  I
> didn't
> > look into this more, but I'm feeling that chrome is not ready to do so. 
Some
> > modules seem to depend on profile is not OTR.
> 
> Patch #3 and #4 seems not correct. The code is added in a "if (!logged_in_)"
> branch which is only executed for login manager (i.e. before login). We
probably
> should do the guest logic in a "else" branch of that "if".

You are right and I was so dull.  Please just ignore patchset 3-4.  They're just
wrong.
I rewrote my code locally to move them to else-clause, and tried, and got a
crash during chrome_browser_main_chromeos.  So the crash patch wasn't uploaded.

[Jun Mukai, 2012-03-19 10:38:09]

http://codereview.chromium.org/9691032/diff/11001/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc (right):

http://codereview.chromium.org/9691032/diff/11001/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc:27: if
(chromeos::UserManager::Get()->IsLoggedInAsGuest())
On 2012/03/15 16:49:45, sky wrote:
> Can you use the same code as is in ChromeLauncherDelegate? That way you don't
> need the ifdef. I suggest pulling the code out of ChromeLauncherDelegate
putting
> it into a static on ProfileManager.

Done.

http://codereview.chromium.org/9691032/diff/11001/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/app_list/app_list_view_delegate.cc:30:
AppListModelBuilder builder(profile, model);
On 2012/03/15 12:11:32, Nikita Kostylev wrote:
> This results in calling BrowserList::GetLastActiveWithProfile().
> 
> Seems that for now till starting guest mode with OTR profile as default one is
> works w/o crashes you should move this code to
> BrowserList::GetLastActiveWithProfile so that we accidentally don't repeat
this
> bug.

no... that's also used in ExtensionAppItem, so it would be good to capture in
other place.  Here, or in AppListModelBuilder's constructor would be nice.

[Nikita (slow), 2012-03-19 10:50:46]

lgtm

http://codereview.chromium.org/9691032/diff/13001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/9691032/diff/13001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:187: Profile*
ProfileManager::GetDefaultProfileOrOffTheRecord() {
I see this method as a temporary measure as we could not place this code in the
GetDefaultProfile() for now (Guest mode crashes).

Please leave a TODO (you could leave my ldap). In the long term we should fix
those cases that crash on Guest mode and have only one GetDefaultProfile()
method.

[Jun Mukai, 2012-03-19 11:00:59]

http://codereview.chromium.org/9691032/diff/13001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/9691032/diff/13001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:187: Profile*
ProfileManager::GetDefaultProfileOrOffTheRecord() {
On 2012/03/19 10:50:46, Nikita Kostylev wrote:
> I see this method as a temporary measure as we could not place this code in
the
> GetDefaultProfile() for now (Guest mode crashes).
> 
> Please leave a TODO (you could leave my ldap). In the long term we should fix
> those cases that crash on Guest mode and have only one GetDefaultProfile()
> method.

Done.

[sky, 2012-03-19 15:29:28]

http://codereview.chromium.org/9691032/diff/17002/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc (right):

http://codereview.chromium.org/9691032/diff/17002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc:522: if
(browser_defaults::kAlwaysOpenIncognitoWindow &&
Do we need 522-526 anymore? Isn't it now covered by the check in profilemanager?

[Jun Mukai, 2012-03-20 04:36:07]

http://codereview.chromium.org/9691032/diff/17002/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc (right):

http://codereview.chromium.org/9691032/diff/17002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc:522: if
(browser_defaults::kAlwaysOpenIncognitoWindow &&
On 2012/03/19 15:29:28, sky wrote:
> Do we need 522-526 anymore? Isn't it now covered by the check in
profilemanager?

Removed.

[sky, 2012-03-20 15:32:08]

LGTM

[xiyuan, 2012-03-20 16:07:40]

lgtm

[commit-bot: I haz the power, 2012-03-21 00:49:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/9691032/22002

[commit-bot: I haz the power, 2012-03-21 00:49:21]

Presubmit check for 9691032-22002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/profiles

Presubmit checks took 1.3s to calculate.

[Jun Mukai, 2012-03-21 00:56:54]

davemoore, could you have a look at this CL?  I need your approval...

[DaveMoore, 2012-03-22 00:47:14]

lgtm