[cros] user_manager component - move UserManagerBase and UserManager

[cros] user_manager component - move UserManagerBase and UserManager

* Move UserManager test helper classes in their own files
* Move RemoveUserDelegate to user_manager component
* Move UserManager ownership to BrowserProcessPlatformPart
* UserManager::Get() still stays but instance is explicitly initialized/cleared with instance->Initialize()|Destroy()
* ScopedUserManagerEnabler / ScopedTestUserManager classes still work

* Introduce ChromeUserManager base class (impl: ChromeUserManagerImpl)
* ChromeUserManager adds getters for MultiProfileUserController, UserImageManager, SupervisedUserManager.
All clients that were using these methods now access ChromeUserManager instance through ChromeUserManager::Get().
* FakeUserManager/MockUserManager implement ChromeUserManager
* Move *UserFlow() methods out of UserManager to ChromeUserManager

BUG=387614
TBR=thestig,benwells

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

[Nikita (slow), 2014-08-11 13:16:44]

Dmitry, please take a look.

[Nikita (slow), 2014-08-11 19:43:49]

OWNERS of dependencies added to componenents/user_manager/DEPS:

oshima:
'+chromeos/cryptohome/async_method_caller.h'

creis:
'+content/public/browser/browser_thread.h'

OWNERS:
bauerb:
  chrome/browser/browsing_data/*
  chrome/browser/prefs/browser_prefs.cc
  chrome/browser/supervised_user/*

thestig:
  chrome/browser/DEPS
  chrome/browser/background/background_mode_manager_unittest.cc
  chrome/browser/browser_process_platform_part_chromeos.cc
  chrome/browser/browser_process_platform_part_chromeos.h
  chrome/browser/chrome_browser_field_trials_desktop.cc
  chrome/browser/invalidation/*
  chrome/browser/io_thread.cc
  chrome/browser/lifetime/application_lifetime.cc
  chrome/browser/media/media_stream_devices_controller.cc
  chrome/browser/media_galleries/*
  chrome/browser/metrics/*
  chrome/browser/net/nss_context_chromeos_browsertest.cc
  chrome/browser/notifications/*
  chrome/browser/performance_monitor/performance_monitor_browsertest.cc
  chrome/browser/policy/*
  chrome/browser/profiles/*
  chrome/browser/signin/*
  chrome/browser/sync/*
  chrome/browser/sync_file_system/*
  chrome/browser/themes/theme_syncable_service_unittest.cc
  chrome/browser/ui/*
  chrome/test/base/browser_with_test_window_test.h

benwells:
  apps/DEPS
  apps/launcher.cc
  chrome/browser/apps/app_browsertest.cc
  chrome/browser/extensions/*
  extensions/shell/browser/shell_browser_main_parts.cc

oshima:
  chrome/browser/ui/ash/*

[Charlie Reis, 2014-08-11 20:23:58]

In general, please wait until you've gotten approval on the CL before adding all
the owners, since the CL could change before it's ready for us.

[Dmitry Polukhin, 2014-08-12 09:03:13]

https://codereview.chromium.org/444903002/diff/380001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/first_run.cc (right):

https://codereview.chromium.org/444903002/diff/380001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/first_run.cc:99: new
DialogLauncher(ProfileHelper::Get()->GetProfileByUser(
I think it is better to replace with  ProfileManager::GetActiveUserProfile()

https://codereview.chromium.org/444903002/diff/380001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/444903002/diff/380001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:1564: if
(logged_in_user) {
This check was added in this CL. Why it didn't require before?

https://codereview.chromium.org/444903002/diff/380001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/terms_of_service_screen_handler.cc
(right):

https://codereview.chromium.org/444903002/diff/380001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/terms_of_service_screen_handler.cc:87:
->GetProfileByUser(user_manager::UserManager::Get()->GetActiveUser())
ProfileManager::GetActiveUserProfile()?

https://codereview.chromium.org/444903002/diff/380001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/terms_of_service_screen_handler.cc:154:
->GetProfileByUser(user_manager::UserManager::Get()->GetActiveUser())
Use ProfileManager::GetActiveUserProfile()?

https://codereview.chromium.org/444903002/diff/380001/components/user_manager...
File components/user_manager/user_manager_base.cc (left):

https://codereview.chromium.org/444903002/diff/380001/components/user_manager...
components/user_manager/user_manager_base.cc:119:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Why did you remove it?

[Nikita (slow), 2014-08-12 09:34:42]

https://codereview.chromium.org/444903002/diff/380001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/first_run.cc (right):

https://codereview.chromium.org/444903002/diff/380001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/first_run.cc:99: new
DialogLauncher(ProfileHelper::Get()->GetProfileByUser(
On 2014/08/12 09:03:12, Dmitry Polukhin wrote:
> I think it is better to replace with  ProfileManager::GetActiveUserProfile()

Done.

https://codereview.chromium.org/444903002/diff/380001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/444903002/diff/380001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:1564: if
(logged_in_user) {
On 2014/08/12 09:03:12, Dmitry Polukhin wrote:
> This check was added in this CL. Why it didn't require before?

I've only hit this check in one of my intermediate patches during tests.
So what was happening is that UserManager was being initialized and there was no
logged in user yet.

I've fixed that issue but decided to leave this check just in case this ever
happens again.

https://codereview.chromium.org/444903002/diff/380001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/terms_of_service_screen_handler.cc
(right):

https://codereview.chromium.org/444903002/diff/380001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/terms_of_service_screen_handler.cc:87:
->GetProfileByUser(user_manager::UserManager::Get()->GetActiveUser())
On 2014/08/12 09:03:13, Dmitry Polukhin wrote:
> ProfileManager::GetActiveUserProfile()?

Done.

https://codereview.chromium.org/444903002/diff/380001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/terms_of_service_screen_handler.cc:154:
->GetProfileByUser(user_manager::UserManager::Get()->GetActiveUser())
On 2014/08/12 09:03:13, Dmitry Polukhin wrote:
> Use ProfileManager::GetActiveUserProfile()?

Done.

https://codereview.chromium.org/444903002/diff/380001/components/user_manager...
File components/user_manager/user_manager_base.cc (left):

https://codereview.chromium.org/444903002/diff/380001/components/user_manager...
components/user_manager/user_manager_base.cc:119:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/08/12 09:03:13, Dmitry Polukhin wrote:
> Why did you remove it?

unit_tests now use this class through

FakeUserManager|MockUserManager > ChromeUserManager > UserManagerBase >
UserManager

instead of previous

FakeUserManager|MockUserManager > UserManager.

unit_tests don't have browser threads creating so this check fails. We do have
these checks in ChromeUserManager methods though.

[Dmitry Polukhin, 2014-08-12 09:50:56]

lgtm

[Nikita (slow), 2014-08-12 11:01:23]

OWNERS of dependencies added to componenents/user_manager/DEPS:

oshima:
'+chromeos/cryptohome/async_method_caller.h'

creis:
'+content/public/browser/browser_thread.h'

OWNERS:
bauerb:
  chrome/browser/browsing_data/*
  chrome/browser/prefs/browser_prefs.cc
  chrome/browser/supervised_user/*

thestig:
  chrome/browser/DEPS
  chrome/browser/background/background_mode_manager_unittest.cc
  chrome/browser/browser_process_platform_part_chromeos.cc
  chrome/browser/browser_process_platform_part_chromeos.h
  chrome/browser/chrome_browser_field_trials_desktop.cc
  chrome/browser/invalidation/*
  chrome/browser/io_thread.cc
  chrome/browser/lifetime/application_lifetime.cc
  chrome/browser/media/media_stream_devices_controller.cc
  chrome/browser/media_galleries/*
  chrome/browser/metrics/*
  chrome/browser/net/nss_context_chromeos_browsertest.cc
  chrome/browser/notifications/*
  chrome/browser/performance_monitor/performance_monitor_browsertest.cc
  chrome/browser/policy/*
  chrome/browser/profiles/*
  chrome/browser/signin/*
  chrome/browser/sync/*
  chrome/browser/sync_file_system/*
  chrome/browser/themes/theme_syncable_service_unittest.cc
  chrome/browser/ui/*
  chrome/test/base/browser_with_test_window_test.h

benwells:
  apps/DEPS
  apps/launcher.cc
  chrome/browser/apps/app_browsertest.cc
  chrome/browser/extensions/*
  extensions/shell/browser/shell_browser_main_parts.cc

oshima:
  chrome/browser/ui/ash/*

[oshima, 2014-08-12 12:45:33]

c/b/ui/ash lgtm

[Charlie Reis, 2014-08-12 16:41:58]

[-creis,+jam]

Oh, I see-- you wanted a review of a line in components/user_manager/DEPS.  I
don't have a good sense of the DEPS guidelines for components, so John's a
better reviewer for whether it's ok to depend on browser_thread.h.  (Seems ok to
me.)

[Bernhard Bauer, 2014-08-12 20:47:40]

chrome/browser/{browsing_data,prefs/browser_prefs.cc,supervised_user} LGTM. Feel
free to TBR changes like this if the core change has been LGTM'd.

[jam, 2014-08-12 23:56:00]

On 2014/08/12 16:41:58, Charlie Reis wrote:
> [-creis,+jam]
> 
> Oh, I see-- you wanted a review of a line in components/user_manager/DEPS.  I
> don't have a good sense of the DEPS guidelines for components, so John's a
> better reviewer for whether it's ok to depend on browser_thread.h.  (Seems ok
to
> me.)

can you use dependency injection to avoid the include on content? i.e. pass in a
single threaded task runner pointer. it's nice to avoid a dependency on content
if you can avoid it.

[jam, 2014-08-12 23:56:05]

On 2014/08/12 16:41:58, Charlie Reis wrote:
> [-creis,+jam]
> 
> Oh, I see-- you wanted a review of a line in components/user_manager/DEPS.  I
> don't have a good sense of the DEPS guidelines for components, so John's a
> better reviewer for whether it's ok to depend on browser_thread.h.  (Seems ok
to
> me.)

can you use dependency injection to avoid the include on content? i.e. pass in a
single threaded task runner pointer. it's nice to avoid a dependency on content
if you can avoid it.

[Nikita (slow), 2014-08-13 09:06:03]

On 2014/08/12 23:56:05, jam wrote:
> On 2014/08/12 16:41:58, Charlie Reis wrote:
> > [-creis,+jam]
> > 
> > Oh, I see-- you wanted a review of a line in components/user_manager/DEPS. 
I
> > don't have a good sense of the DEPS guidelines for components, so John's a
> > better reviewer for whether it's ok to depend on browser_thread.h.  (Seems
ok
> to
> > me.)
> 
> can you use dependency injection to avoid the include on content? i.e. pass in
a
> single threaded task runner pointer. it's nice to avoid a dependency on
content
> if you can avoid it.

I can inject task runner pointer but what about checking for correct thread?
user_manager_base.cc has multiple checks like that.

user_manager_base.cc:  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
user_manager_base.cc:  BrowserThread::PostTask(BrowserThread::UI,
user_manager_base.cc:    BrowserThread::PostBlockingPoolTask(

[Bernhard Bauer, 2014-08-13 09:12:39]

On 2014/08/13 09:06:03, Nikita Kostylev wrote:
> On 2014/08/12 23:56:05, jam wrote:
> > On 2014/08/12 16:41:58, Charlie Reis wrote:
> > > [-creis,+jam]
> > > 
> > > Oh, I see-- you wanted a review of a line in components/user_manager/DEPS.

> I
> > > don't have a good sense of the DEPS guidelines for components, so John's a
> > > better reviewer for whether it's ok to depend on browser_thread.h.  (Seems
> ok
> > to
> > > me.)
> > 
> > can you use dependency injection to avoid the include on content? i.e. pass
in
> a
> > single threaded task runner pointer. it's nice to avoid a dependency on
> content
> > if you can avoid it.
> 
> I can inject task runner pointer but what about checking for correct thread?
> user_manager_base.cc has multiple checks like that.

SingleThreadTaskRunner has a method BelongsToCurrentThread().

> user_manager_base.cc:  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
> user_manager_base.cc:  BrowserThread::PostTask(BrowserThread::UI,
> user_manager_base.cc:    BrowserThread::PostBlockingPoolTask(

You'll probably need to inject a second TaskRunner for the blocking pool.

[Nikita (slow), 2014-08-13 16:43:01]

On 2014/08/12 23:56:05, jam wrote:
> On 2014/08/12 16:41:58, Charlie Reis wrote:
> > [-creis,+jam]
> > 
> > Oh, I see-- you wanted a review of a line in components/user_manager/DEPS. 
I
> > don't have a good sense of the DEPS guidelines for components, so John's a
> > better reviewer for whether it's ok to depend on browser_thread.h.  (Seems
ok
> to
> > me.)
> 
> can you use dependency injection to avoid the include on content? i.e. pass in
a
> single threaded task runner pointer. it's nice to avoid a dependency on
content
> if you can avoid it.

Done. Please see patchsets 16..17 diff.

[Nikita (slow), 2014-08-14 09:20:49]

TBR'ing thestig@ and benwells@

On 2014/08/12 11:01:23, Nikita Kostylev wrote:
> OWNERS:
> 
> thestig:
>   chrome/browser/DEPS
>   chrome/browser/background/background_mode_manager_unittest.cc
>   chrome/browser/browser_process_platform_part_chromeos.cc
>   chrome/browser/browser_process_platform_part_chromeos.h
>   chrome/browser/chrome_browser_field_trials_desktop.cc
>   chrome/browser/invalidation/*
>   chrome/browser/io_thread.cc
>   chrome/browser/lifetime/application_lifetime.cc
>   chrome/browser/media/media_stream_devices_controller.cc
>   chrome/browser/media_galleries/*
>   chrome/browser/metrics/*
>   chrome/browser/net/nss_context_chromeos_browsertest.cc
>   chrome/browser/notifications/*
>   chrome/browser/performance_monitor/performance_monitor_browsertest.cc
>   chrome/browser/policy/*
>   chrome/browser/profiles/*
>   chrome/browser/signin/*
>   chrome/browser/sync/*
>   chrome/browser/sync_file_system/*
>   chrome/browser/themes/theme_syncable_service_unittest.cc
>   chrome/browser/ui/*
>   chrome/test/base/browser_with_test_window_test.h
> 
> benwells:
>   apps/DEPS
>   apps/launcher.cc
>   chrome/browser/apps/app_browsertest.cc
>   chrome/browser/extensions/*
>   extensions/shell/browser/shell_browser_main_parts.cc

[Nikita (slow), 2014-08-14 11:07:08]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-08-14 11:09:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/444903002/500001

[commit-bot: I haz the power, 2014-08-14 17:44:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[commit-bot: I haz the power, 2014-08-14 17:52:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-14 17:52:32]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[Nikita (slow), 2014-08-14 18:00:08]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-08-14 18:02:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/444903002/500001

[commit-bot: I haz the power, 2014-08-14 18:54:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[commit-bot: I haz the power, 2014-08-14 19:44:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-14 19:44:42]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[Lei Zhang, 2014-08-14 21:10:38]

lgtm

[benwells, 2014-08-14 22:49:41]

lgtm

[Nikita (slow), 2014-08-15 03:59:39]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-08-15 04:01:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/444903002/500001

[commit-bot: I haz the power, 2014-08-15 04:11:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[commit-bot: I haz the power, 2014-08-15 04:23:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-15 04:23:52]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[Nikita (slow), 2014-08-15 04:24:44]

The CQ bit was checked by nkostylev@chromium.org

[Nikita (slow), 2014-08-15 04:24:45]

The CQ bit was unchecked by nkostylev@chromium.org

[Nikita (slow), 2014-08-15 04:24:47]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-08-15 04:30:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/444903002/500001

[commit-bot: I haz the power, 2014-08-15 04:47:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[commit-bot: I haz the power, 2014-08-15 04:53:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-15 04:53:46]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[Nikita (slow), 2014-08-15 05:05:20]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-08-15 05:11:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/444903002/520001

[Nikita (slow), 2014-08-15 05:28:31]

The CQ bit was unchecked by nkostylev@chromium.org

[Nikita (slow), 2014-08-15 05:28:51]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-08-15 05:29:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/42142)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/47354)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-15 05:31:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/444903002/540001

[commit-bot: I haz the power, 2014-08-15 09:23:09]

Committed patchset #20 (540001) as 289827