Issue 13633003: Part of multiprofile implementation.

Issue 13633003: Part of multiprofile implementation. (Closed)

Created:
7 years, 8 months ago by dzhioev (left Google)

Modified:
7 years, 8 months ago

Reviewers:
Nikita (slow), sail

CC:
chromium-reviews, nkostylev+watch_chromium.org, sail+watch_chromium.org, rginda+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, Joao da Silva

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Part of multiprofile implementation. Created method for obtaining signin profile. Signin profile used instead of "DefaultProfile" during login phase. BUG=225116 R=nkostylev@chromium.org TBR=brettw Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194774

Patch Set 1 #

Total comments: 13

Patch Set 2 : Destroying signin profile after session start. #

Total comments: 2

Patch Set 3 : Created ProfileHelper for ChromeOS. #

Patch Set 4 : Little cleanup. #

Total comments: 6

Patch Set 5 : Nits fixed. #

Total comments: 8

Patch Set 6 : #

Patch Set 7 : merge #

Patch Set 8 : #

Patch Set 9 : Tests not fail anymore. #

Patch Set 10 : m #

Created: 7 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+91 lines, -115 lines)			Patch
M	chrome/browser/chromeos/login/base_login_display_host.cc	View	1 2 3 4 5 6	1 chunk	+2 lines, -4 lines	0 comments	Download
M	chrome/browser/chromeos/login/captive_portal_window_proxy.cc	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/chromeos/login/existing_user_controller.cc	View	1 2	2 chunks	+6 lines, -6 lines	0 comments	Download
M	chrome/browser/chromeos/login/help_app_launcher.cc	View	1 2	2 chunks	+2 lines, -3 lines	0 comments	Download
M	chrome/browser/chromeos/login/login_performer.cc	View	1 2	3 chunks	+3 lines, -11 lines	0 comments	Download
M	chrome/browser/chromeos/login/login_utils.cc	View	1 2 3 4 5 6 7 8	3 chunks	+7 lines, -3 lines	0 comments	Download
M	chrome/browser/chromeos/login/login_web_dialog.cc	View	1 2	2 chunks	+3 lines, -2 lines	0 comments	Download
M	chrome/browser/chromeos/login/proxy_settings_dialog.cc	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/chromeos/login/screens/terms_of_service_screen.cc	View	1 2 3 4 5 6	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/chromeos/login/user_manager_impl.cc	View	1 2 3 4 5 6 7 8 9	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/webui_login_view.cc	View	1 2 3 4 5 6 7 8 9	3 chunks	+5 lines, -3 lines	0 comments	Download
D	chrome/browser/chromeos/profile_startup.h	View	1 2 3	1 chunk	+0 lines, -20 lines	0 comments	Download
D	chrome/browser/chromeos/profile_startup.cc	View	1 2 3	1 chunk	+0 lines, -38 lines	0 comments	Download
A	chrome/browser/chromeos/profiles/profile_helper.h	View	1 2 3 4 5	1 chunk	+30 lines, -0 lines	0 comments	Download
A +	chrome/browser/chromeos/profiles/profile_helper.cc	View	1 2 3 4 5 6	2 chunks	+20 lines, -10 lines	0 comments	Download
M	chrome/browser/chromeos/sms_observer.h	View	1 2 3 4	2 chunks	+1 line, -2 lines	0 comments	Download
M	chrome/browser/chromeos/sms_observer.cc	View	1 2 3	1 chunk	+1 line, -3 lines	0 comments	Download
M	chrome/browser/ui/startup/startup_browser_creator.cc	View	1 2 3	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/chrome_browser_chromeos.gypi	View	1 2 3 4 5 6 7 8 9	1 chunk	+2 lines, -2 lines	0 comments	Download

Messages

Total messages: 19 (0 generated)

Expand Messages | Collapse Messages

Nikita (slow)

chromeos/login lgtm with nits https://codereview.chromium.org/13633003/diff/1/chrome/browser/chromeos/login/existing_user_controller.cc File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/13633003/diff/1/chrome/browser/chromeos/login/existing_user_controller.cc#newcode278 chrome/browser/chromeos/login/existing_user_controller.cc:278: Profile* signing_profile = ProfileManager::GetSigninProfile(); nit: ...

7 years, 8 months ago (2013-04-04 12:59:13 UTC) #1

sail

https://codereview.chromium.org/13633003/diff/1/chrome/browser/profiles/avatar_menu_model.cc File chrome/browser/profiles/avatar_menu_model.cc (right): https://codereview.chromium.org/13633003/diff/1/chrome/browser/profiles/avatar_menu_model.cc#newcode195 chrome/browser/profiles/avatar_menu_model.cc:195: #if defined(OS_CHROMEOS) Could you just delete this part entirely? ...

7 years, 8 months ago (2013-04-04 14:38:37 UTC) #2

Nikita (slow)

Sailesh, please take a look at CrOS multi-profiles design doc that describes changes to ProfileManager: ...

7 years, 8 months ago (2013-04-04 14:50:04 UTC) #3

sail

7 years, 8 months ago (2013-04-04 15:09:34 UTC) #4

dzhioev (left Google)

7 years, 8 months ago (2013-04-04 15:54:41 UTC) #5

https://codereview.chromium.org/13633003/diff/1/chrome/browser/profiles/avata...
File chrome/browser/profiles/avatar_menu_model.cc (right):

https://codereview.chromium.org/13633003/diff/1/chrome/browser/profiles/avata...
chrome/browser/profiles/avatar_menu_model.cc:195: #if defined(OS_CHROMEOS)
On 2013/04/04 15:09:34, sail wrote:
> On 2013/04/04 14:50:05, Nikita Kostylev wrote:
> > On 2013/04/04 14:38:37, sail wrote:
> > > Could you just delete this part entirely?
> > 
> > This part should not be deleted as eventually this code will be present in
the
> > final version i.e. Chrome OS has separate multi-profiles UI.
> > 
> > The reason why this code is commented out is to have a short-term prototype
> that
> > works with --multi-profiles switch. 
> 
> Makes sense. Could you add a comment / TODO then?
I've uncommented it back as long as we have no Chrome OS specific
implementation.

https://codereview.chromium.org/13633003/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/13633003/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_manager.h:68: Profile* GetSigninProfile(const
base::FilePath& user_data_dir);
On 2013/04/04 15:09:34, sail wrote:
> On 2013/04/04 14:50:05, Nikita Kostylev wrote:
> > On 2013/04/04 14:38:37, sail wrote:
> > > Can we keep this out of ProfileManager? Maybe put this in sign-in specific
> > code?
> > 
> > I think that we could  not get this code out of ProfileManager.
> > 
> > [user_data_dir]/Default is a special type of profile only on Chrome OS - it
is
> > used as a part of sign in flow.
> > 
> > On the other hand it should be part of ProfileManager bookkeeping logic like
> > loading profile, deleting it etc.
> 
> Currently profile manager is a very very large class and it's hard to
maintain.
> There's nothing that this function needs to do that requires any private data.
> If you really can't find a better place to put it then how about doing
something
> like this:
>   profile_signin_util.h
>     namespace profile_signin_util {
>       GetSigninProfile()
>     }
* I'm not sure that we can implement GetSigninProfile using only public methods
of ProfileManager.
* We want to eliminate GetDefaultProfile completely while making multiprofiles
and I think Chrome OS related logic should become much simpler.

Nikita (slow)

https://codereview.chromium.org/13633003/diff/7002/chrome/browser/chromeos/login/user_manager_impl.cc File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/13633003/diff/7002/chrome/browser/chromeos/login/user_manager_impl.cc#newcode658 chrome/browser/chromeos/login/user_manager_impl.cc:658: // TODO(nkostylev): Mayby we should observe all logged in ...

7 years, 8 months ago (2013-04-08 13:09:44 UTC) #6

dzhioev (left Google)

On 2013/04/04 15:09:34, sail wrote: > https://codereview.chromium.org/13633003/diff/1/chrome/browser/profiles/avatar_menu_model.cc > File chrome/browser/profiles/avatar_menu_model.cc (right): > > https://codereview.chromium.org/13633003/diff/1/chrome/browser/profiles/avatar_menu_model.cc#newcode195 > ...

7 years, 8 months ago (2013-04-11 15:25:06 UTC) #7

On 2013/04/04 15:09:34, sail wrote:
>
https://codereview.chromium.org/13633003/diff/1/chrome/browser/profiles/avata...
> File chrome/browser/profiles/avatar_menu_model.cc (right):
> 
>
https://codereview.chromium.org/13633003/diff/1/chrome/browser/profiles/avata...
> chrome/browser/profiles/avatar_menu_model.cc:195: #if defined(OS_CHROMEOS)
> On 2013/04/04 14:50:05, Nikita Kostylev wrote:
> > On 2013/04/04 14:38:37, sail wrote:
> > > Could you just delete this part entirely?
> > 
> > This part should not be deleted as eventually this code will be present in
the
> > final version i.e. Chrome OS has separate multi-profiles UI.
> > 
> > The reason why this code is commented out is to have a short-term prototype
> that
> > works with --multi-profiles switch. 
> 
> Makes sense. Could you add a comment / TODO then?
> 
>
https://codereview.chromium.org/13633003/diff/1/chrome/browser/profiles/profi...
> File chrome/browser/profiles/profile_manager.h (right):
> 
>
https://codereview.chromium.org/13633003/diff/1/chrome/browser/profiles/profi...
> chrome/browser/profiles/profile_manager.h:68: Profile* GetSigninProfile(const
> base::FilePath& user_data_dir);
> On 2013/04/04 14:50:05, Nikita Kostylev wrote:
> > On 2013/04/04 14:38:37, sail wrote:
> > > Can we keep this out of ProfileManager? Maybe put this in sign-in specific
> > code?
> > 
> > I think that we could  not get this code out of ProfileManager.
> > 
> > [user_data_dir]/Default is a special type of profile only on Chrome OS - it
is
> > used as a part of sign in flow.
> > 
> > On the other hand it should be part of ProfileManager bookkeeping logic like
> > loading profile, deleting it etc.
> 
> Currently profile manager is a very very large class and it's hard to
maintain.
> There's nothing that this function needs to do that requires any private data.
> If you really can't find a better place to put it then how about doing
something
> like this:
>   profile_signin_util.h
>     namespace profile_signin_util {
>       GetSigninProfile()
>     }

I've created chromeos::ProfileHelper. Currently it has only GetSigninProfile
method, but we want to implement some other ChromeOS specific methods later.

dzhioev (left Google)

7 years, 8 months ago (2013-04-11 15:26:47 UTC) #8

Nikita (slow)

lgtm https://codereview.chromium.org/13633003/diff/26001/chrome/browser/chromeos/login/base_login_display_host.cc File chrome/browser/chromeos/login/base_login_display_host.cc (right): https://codereview.chromium.org/13633003/diff/26001/chrome/browser/chromeos/login/base_login_display_host.cc#newcode36 chrome/browser/chromeos/login/base_login_display_host.cc:36: #include "chrome/browser/chromeos/profiles/profile_helper.h" nit: not used https://codereview.chromium.org/13633003/diff/26001/chrome/browser/chromeos/profiles/profile_helper.cc File chrome/browser/chromeos/profiles/profile_helper.cc ...

7 years, 8 months ago (2013-04-12 16:03:17 UTC) #9

dzhioev (left Google)

https://codereview.chromium.org/13633003/diff/26001/chrome/browser/chromeos/login/base_login_display_host.cc File chrome/browser/chromeos/login/base_login_display_host.cc (right): https://codereview.chromium.org/13633003/diff/26001/chrome/browser/chromeos/login/base_login_display_host.cc#newcode36 chrome/browser/chromeos/login/base_login_display_host.cc:36: #include "chrome/browser/chromeos/profiles/profile_helper.h" On 2013/04/12 16:03:17, Nikita Kostylev wrote: > ...

7 years, 8 months ago (2013-04-15 07:20:03 UTC) #11

Nikita (slow)

just FYI, another user of signin profile is being added here https://codereview.chromium.org/13872010/

7 years, 8 months ago (2013-04-15 14:46:53 UTC) #13

sail

LGTM I think this is a really good approach https://codereview.chromium.org/13633003/diff/37001/chrome/browser/chromeos/profiles/profile_helper.cc File chrome/browser/chromeos/profiles/profile_helper.cc (right): https://codereview.chromium.org/13633003/diff/37001/chrome/browser/chromeos/profiles/profile_helper.cc#newcode30 chrome/browser/chromeos/profiles/profile_helper.cc:30: ...

7 years, 8 months ago (2013-04-15 15:42:53 UTC) #14

dzhioev (left Google)

https://codereview.chromium.org/13633003/diff/37001/chrome/browser/chromeos/profiles/profile_helper.cc File chrome/browser/chromeos/profiles/profile_helper.cc (right): https://codereview.chromium.org/13633003/diff/37001/chrome/browser/chromeos/profiles/profile_helper.cc#newcode30 chrome/browser/chromeos/profiles/profile_helper.cc:30: // TODO(beng): It is very fishy that any of ...

7 years, 8 months ago (2013-04-15 19:48:11 UTC) #15

Nikita (slow)

Committed patchset #7 manually as r194348 (presubmit successful).

7 years, 8 months ago (2013-04-16 12:13:02 UTC) #16

Nikita (slow)

7 years, 8 months ago (2013-04-18 05:05:28 UTC) #19

Message was sent while issue was closed.

Committed patchset #10 manually as r194774 (presubmit successful).

Expand Messages | Collapse Messages