Make FakeChromeUserManager subclass of ChromeUserManager

Make FakeChromeUserManager subclass of ChromeUserManager

This patch does:
- Make FakeChromeUserManager a subclass of ChromeUserManager
- Use FakeChromeUserManager in the tests intead of FakeUserManager

FakeChromeUserManager has to be a subclass of ChromeUserManager in the chromeos-related tests, since ChromeUserManager::Get() does static_cast the user manager instance to ChromeUserManager type.

Previously, an instance of FakeChromeUserManager, which didn't inherit ChromeUserManager, was casted to ChromeUserManager instance wrongly, and it caused a bit-rot and some crashes in some tests. This patch re-enables some of them.

Following tests are still failling due to other reason. We need to fix a cause and reenable them separately.
- UserCloudPolicyManagerChromeOSTest.BlockingFetchTimeout
- UserCloudPolicyManagerChromeOSTest.BlockingRefreshFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingFirstFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingRefreshFetch

BUG=534733
TEST=trybots pass
R=achuith@chromium.org

Committed: https://crrev.com/18898fb63b4079de832cc14a1331a0b0853490f1
Cr-Commit-Position: refs/heads/master@{#433211}

[yoshiki, 2016-11-15 05:05:46]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[yoshiki, 2016-11-15 05:05:53]

Description was changed from

==========
.

BUG=
==========

to

==========
Make FakeChromeUserManager a subclass of ChromeUserManager

This patch does:
- Make FakeChromeUserManager a subclass of ChromeUserManager
- Use FakeChromeUserManager in the tests intead of FakeUserManager

We have to use a subclass of ChromeUserManager in the chromeos-related tests,
since ChromeUserManager::Get() does static_cast the user manager instance to
ChromeUserManager type.

Previously, a FakeChromeUserManager instance, which didn't inherit
ChromeUserManager, was casted to ChromeUserManager instance. It caused some
bit-rot and crashes in some tests.

BUG=534733
TEST=trybots pass
R=achuith@chromium.org
==========

[commit-bot: I haz the power, 2016-11-15 05:06:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 05:49:59]

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

[commit-bot: I haz the power, 2016-11-15 05:50:00]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2016-11-15 07:49:09]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 07:49:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoshiki, 2016-11-15 07:53:32]

Description was changed from

==========
Make FakeChromeUserManager a subclass of ChromeUserManager

This patch does:
- Make FakeChromeUserManager a subclass of ChromeUserManager
- Use FakeChromeUserManager in the tests intead of FakeUserManager

We have to use a subclass of ChromeUserManager in the chromeos-related tests,
since ChromeUserManager::Get() does static_cast the user manager instance to
ChromeUserManager type.

Previously, a FakeChromeUserManager instance, which didn't inherit
ChromeUserManager, was casted to ChromeUserManager instance. It caused some
bit-rot and crashes in some tests.

BUG=534733
TEST=trybots pass
R=achuith@chromium.org
==========

to

==========
Make FakeChromeUserManager a subclass of ChromeUserManager

This patch does:
- Make FakeChromeUserManager a subclass of ChromeUserManager
- Use FakeChromeUserManager in the tests intead of FakeUserManager

FakeChromeUserManager has to be a subclass of ChromeUserManager in the
chromeos-related tests, since ChromeUserManager::Get() does static_cast the user
manager instance to ChromeUserManager type.

Previously, an instance of FakeChromeUserManager, which didn't inherit
ChromeUserManager, was casted to ChromeUserManager instance wrongly, and it
caused a bit-rot and some crashes in some tests.

BUG=534733
TEST=trybots pass
R=achuith@chromium.org
==========

[commit-bot: I haz the power, 2016-11-15 08:23:22]

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

[commit-bot: I haz the power, 2016-11-15 08:23:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2016-11-15 08:49:18]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 08:49:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoshiki, 2016-11-15 08:49:59]

Patchset #2 (id:20001) has been deleted

[yoshiki, 2016-11-15 08:55:43]

Description was changed from

==========
Make FakeChromeUserManager a subclass of ChromeUserManager

This patch does:
- Make FakeChromeUserManager a subclass of ChromeUserManager
- Use FakeChromeUserManager in the tests intead of FakeUserManager

FakeChromeUserManager has to be a subclass of ChromeUserManager in the
chromeos-related tests, since ChromeUserManager::Get() does static_cast the user
manager instance to ChromeUserManager type.

Previously, an instance of FakeChromeUserManager, which didn't inherit
ChromeUserManager, was casted to ChromeUserManager instance wrongly, and it
caused a bit-rot and some crashes in some tests.

BUG=534733
TEST=trybots pass
R=achuith@chromium.org
==========

to

==========
Make FakeChromeUserManager a subclass of ChromeUserManager

This patch does:
- Make FakeChromeUserManager a subclass of ChromeUserManager
- Use FakeChromeUserManager in the tests intead of FakeUserManager

FakeChromeUserManager has to be a subclass of ChromeUserManager in the
chromeos-related tests, since ChromeUserManager::Get() does static_cast the user
manager instance to ChromeUserManager type.

Previously, an instance of FakeChromeUserManager, which didn't inherit
ChromeUserManager, was casted to ChromeUserManager instance wrongly, and it
caused a bit-rot and some crashes in some tests. This patch re-enables some of
them.

Following tests are still failling due to other reason. We need to fix a cause
and reenable them separately.
- UserCloudPolicyManagerChromeOSTest.BlockingFetchTimeout
- UserCloudPolicyManagerChromeOSTest.BlockingRefreshFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingFirstFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingRefreshFetch


BUG=534733
TEST=trybots pass
R=achuith@chromium.org
==========

[commit-bot: I haz the power, 2016-11-15 09:18:04]

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

[commit-bot: I haz the power, 2016-11-15 09:18:05]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2016-11-15 10:14:07]

Description was changed from

==========
Make FakeChromeUserManager a subclass of ChromeUserManager

This patch does:
- Make FakeChromeUserManager a subclass of ChromeUserManager
- Use FakeChromeUserManager in the tests intead of FakeUserManager

FakeChromeUserManager has to be a subclass of ChromeUserManager in the
chromeos-related tests, since ChromeUserManager::Get() does static_cast the user
manager instance to ChromeUserManager type.

Previously, an instance of FakeChromeUserManager, which didn't inherit
ChromeUserManager, was casted to ChromeUserManager instance wrongly, and it
caused a bit-rot and some crashes in some tests. This patch re-enables some of
them.

Following tests are still failling due to other reason. We need to fix a cause
and reenable them separately.
- UserCloudPolicyManagerChromeOSTest.BlockingFetchTimeout
- UserCloudPolicyManagerChromeOSTest.BlockingRefreshFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingFirstFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingRefreshFetch


BUG=534733
TEST=trybots pass
R=achuith@chromium.org
==========

to

==========
Make FakeChromeUserManager subclass of ChromeUserManager

This patch does:
- Make FakeChromeUserManager a subclass of ChromeUserManager
- Use FakeChromeUserManager in the tests intead of FakeUserManager

FakeChromeUserManager has to be a subclass of ChromeUserManager in the
chromeos-related tests, since ChromeUserManager::Get() does static_cast the user
manager instance to ChromeUserManager type.

Previously, an instance of FakeChromeUserManager, which didn't inherit
ChromeUserManager, was casted to ChromeUserManager instance wrongly, and it
caused a bit-rot and some crashes in some tests. This patch re-enables some of
them.

Following tests are still failling due to other reason. We need to fix a cause
and reenable them separately.
- UserCloudPolicyManagerChromeOSTest.BlockingFetchTimeout
- UserCloudPolicyManagerChromeOSTest.BlockingRefreshFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingFirstFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingRefreshFetch


BUG=534733
TEST=trybots pass
R=achuith@chromium.org
==========

[yoshiki, 2016-11-15 10:14:20]

yoshiki@chromium.org changed reviewers:
+ achuith@chromium.org

[yoshiki, 2016-11-15 10:14:21]

achuith@, could you take a first look, since most of the changes are under
chrome/browser/chromeos/login/users/?

[achuithb, 2016-11-15 20:13:27]

achuith@chromium.org changed reviewers:
+ xiyuan@chromium.org

[achuithb, 2016-11-15 20:13:28]

Xiyuan, can you also take a look?

This looks ok to me overall.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc (right):

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:29: bool
PostDelayedTask(const tracked_objects::Location& from_here,
Add comment:
// TaskRunner overrides.
or something similar

You can also make these private

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:39: };
DISALLOW_COPY_AND_ASSIGN in private section

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:51:
multi_profile_user_controller_(NULL) {
move the initialization of these to header

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:357: for
(user_manager::UserList::const_iterator it = users_.begin();
range loop

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:385: for
(user_manager::UserList::const_iterator it = users.begin();
Does a range loop not work here?

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:425: for
(user_manager::UserList::iterator it = users_.begin(); it != users_.end();
range loop

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:635: if
(!users_.empty()) {
Early return seems preferable here rather than nesting.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:637: for
(user_manager::UserList::const_iterator it = users_.begin();
range loop

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.h (right):

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:126: 
remove newline?

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:190:
user_manager::User* GetActiveUserInternal() const;
Add a comment

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:196:
BootstrapManager* bootstrap_manager_;
= nullptr

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:197:
MultiProfileUserController* multi_profile_user_controller_;
= nullptr

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/scoped_user_manager_enabler.cc (right):

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/scoped_user_manager_enabler.cc:25:
user_manager::UserManager::SetInstance(NULL);
nullptr here

[xiyuan, 2016-11-15 21:19:54]

The approach looks okay as a stop gap. Inheritance is evil. :p We will revisit
it later to clean up the code.

[achuithb, 2016-11-15 22:24:54]

On 2016/11/15 21:19:54, xiyuan wrote:
> The approach looks okay as a stop gap. Inheritance is evil. :p We will revisit
> it later to clean up the code.

Why do you dislike this inheritance? ChromeUserManager is an interface class, so
this should be ok, right?

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/chro...

[stevenjb, 2016-11-15 22:34:19]

On 2016/11/15 22:24:54, achuithb wrote:
> On 2016/11/15 21:19:54, xiyuan wrote:
> > The approach looks okay as a stop gap. Inheritance is evil. :p We will
revisit
> > it later to clean up the code.
> 
> Why do you dislike this inheritance? ChromeUserManager is an interface class,
so
> this should be ok, right?
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/chro...

The ugly part is that user_manager::UserManagerBase is not an interface class,
and ChromeUserManager derives from that.

To clarify Xiyuan's statement (from my perspective anyway), *multiple*
inheritance is evil :)

I will leave this to Xiyuan to review, I was just curious when I saw this go by.
I don't have any suggestions of my own.

[xiyuan, 2016-11-15 22:42:48]

On 2016/11/15 22:24:54, achuithb wrote:
> On 2016/11/15 21:19:54, xiyuan wrote:
> > The approach looks okay as a stop gap. Inheritance is evil. :p We will
revisit
> > it later to clean up the code.
> 
> Why do you dislike this inheritance? ChromeUserManager is an interface class,
so
> this should be ok, right?
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/chro...

I am not against the CL and think it is okay. 

We have complicated class hierarchy here and think needs some clean up.
ChromeUserManager is not exactly an interface because it inherits from
UserManagerBase that is a partial impl of UserManager interface. Meanwhile it
inherits from UserManagerInterface then it also has its own abstract methods.
This makes the code a bit hard to read and track.

[achuithb, 2016-11-15 22:58:38]

On 2016/11/15 22:42:48, xiyuan wrote:
> On 2016/11/15 22:24:54, achuithb wrote:
> > On 2016/11/15 21:19:54, xiyuan wrote:
> > > The approach looks okay as a stop gap. Inheritance is evil. :p We will
> revisit
> > > it later to clean up the code.
> > 
> > Why do you dislike this inheritance? ChromeUserManager is an interface
class,
> so
> > this should be ok, right?
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/chro...
> 
> I am not against the CL and think it is okay. 
> 
> We have complicated class hierarchy here and think needs some clean up.
> ChromeUserManager is not exactly an interface because it inherits from
> UserManagerBase that is a partial impl of UserManager interface. Meanwhile it
> inherits from UserManagerInterface then it also has its own abstract methods.
> This makes the code a bit hard to read and track.

Ah! I didn't realize UserManagerBase was a partially implemented class. I don't
think this CL makes things worse, but this situation is definitely far from
ideal.

[yoshiki, 2016-11-16 12:28:37]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 12:28:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 13:08:01]

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

[commit-bot: I haz the power, 2016-11-16 13:08:02]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2016-11-16 15:50:19]

I also don't think this is the best. But it's good for the first step, since at
least this solves the wrong static-cast. We should clean up the hierarchy even
further.

achuithb@, xiyuan@, PTAL.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc (right):

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:29: bool
PostDelayedTask(const tracked_objects::Location& from_here,
On 2016/11/15 20:13:28, achuithb wrote:
> Add comment:
> // TaskRunner overrides.
> or something similar
> 
> You can also make these private

Done.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:39: };
On 2016/11/15 20:13:28, achuithb wrote:
> DISALLOW_COPY_AND_ASSIGN in private section

Done.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:51:
multi_profile_user_controller_(NULL) {
On 2016/11/15 20:13:28, achuithb wrote:
> move the initialization of these to header

Done.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:357: for
(user_manager::UserList::const_iterator it = users_.begin();
On 2016/11/15 20:13:28, achuithb wrote:
> range loop

Done.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:385: for
(user_manager::UserList::const_iterator it = users.begin();
On 2016/11/15 20:13:28, achuithb wrote:
> Does a range loop not work here?

Done.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:425: for
(user_manager::UserList::iterator it = users_.begin(); it != users_.end();
On 2016/11/15 20:13:28, achuithb wrote:
> range loop

Done.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:635: if
(!users_.empty()) {
On 2016/11/15 20:13:28, achuithb wrote:
> Early return seems preferable here rather than nesting.

Done.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:637: for
(user_manager::UserList::const_iterator it = users_.begin();
On 2016/11/15 20:13:28, achuithb wrote:
> range loop

Done.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.h (right):

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:126: 
On 2016/11/15 20:13:28, achuithb wrote:
> remove newline?

Done.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:190:
user_manager::User* GetActiveUserInternal() const;
On 2016/11/15 20:13:28, achuithb wrote:
> Add a comment

Done.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:196:
BootstrapManager* bootstrap_manager_;
On 2016/11/15 20:13:28, achuithb wrote:
> = nullptr

Done.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:197:
MultiProfileUserController* multi_profile_user_controller_;
On 2016/11/15 20:13:28, achuithb wrote:
> = nullptr

Done.

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/scoped_user_manager_enabler.cc (right):

https://codereview.chromium.org/2504513002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/scoped_user_manager_enabler.cc:25:
user_manager::UserManager::SetInstance(NULL);
On 2016/11/15 20:13:28, achuithb wrote:
> nullptr here

Done.

[achuithb, 2016-11-16 19:51:23]

lgtm. Not sure if Xiyuan would like to take another look.

[xiyuan, 2016-11-16 19:54:14]

On 2016/11/16 19:51:23, achuithb wrote:
> lgtm. Not sure if Xiyuan would like to take another look.

lgtm

I don't have more to add. :)

[yoshiki, 2016-11-16 23:38:05]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2016-11-16 23:39:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 01:18:17]

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

[commit-bot: I haz the power, 2016-11-17 01:18:19]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yoshiki, 2016-11-17 01:24:23]

yoshiki@chromium.org changed reviewers:
+ asargent@chromium.org, msw@chromium.org

[yoshiki, 2016-11-17 01:25:34]

asargent@, chrome/browser/extensions/*
msw@, chrome/browser/ui/*

PTAL, Thanks.

[msw, 2016-11-17 18:31:23]

c/b/ui lgtm with nits (that probably apply elsewhere in this cl too)

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/ash/m...
File
chrome/browser/ui/ash/multi_user/multi_user_context_menu_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_context_menu_chromeos_unittest.cc:53:
chromeos::FakeChromeUserManager* fake_user_manager_;  // Not owned.
nit: this value isn't ever used after being set; instead, inline
user_manager_enabler_(new chromeos::FakeChromeUserManager), since that is what
takes ownership, and otherwise the ownership isn't obvious.

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/ash/m...
File
chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos_unittest.cc:159:
chromeos::FakeChromeUserManager* fake_user_manager_;  // Not owned.
ditto nit

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/multi_user/multi_user_util_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_util_chromeos_unittest.cc:108:
chromeos::FakeChromeUserManager* fake_user_manager_;  // Not owned.
nit: okay, this one is used; it'd be nice to either (1) still inline the manager
instantiation in the enabler ctor and provide an accessor on the enabler to get
the manager, or (2) replace the "Not owned." comment here with something more
useful, like "Owned by |user_manager_enabler_|."

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_finder_chromeos_unittest.cc (right):

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_finder_chromeos_unittest.cc:103:
chromeos::FakeChromeUserManager* fake_user_manager_;  // Not owned.
ditto nit

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/profile_info_handler_unittest.cc (right):

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/profile_info_handler_unittest.cc:90: //
|user_manager_| is destroyed by |user_manager_enabler_|.
ditto nit: this comment is sufficient, but the |user_manager_| isn't accessed,
so it could be removed and the instantiation inlined.

[asargent_no_longer_on_chrome, 2016-11-17 20:40:39]

extensions lgtm

[yoshiki, 2016-11-18 05:35:30]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2016-11-18 05:36:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 05:55:09]

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

[commit-bot: I haz the power, 2016-11-18 05:55:11]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2016-11-18 15:01:13]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 15:01:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 15:39:10]

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

[commit-bot: I haz the power, 2016-11-18 15:39:13]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2016-11-18 15:54:15]

Thanks!

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/ash/m...
File
chrome/browser/ui/ash/multi_user/multi_user_context_menu_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_context_menu_chromeos_unittest.cc:53:
chromeos::FakeChromeUserManager* fake_user_manager_;  // Not owned.
On 2016/11/17 18:31:22, msw wrote:
> nit: this value isn't ever used after being set; instead, inline
> user_manager_enabler_(new chromeos::FakeChromeUserManager), since that is what
> takes ownership, and otherwise the ownership isn't obvious.

Done.

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/ash/m...
File
chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_notification_blocker_chromeos_unittest.cc:159:
chromeos::FakeChromeUserManager* fake_user_manager_;  // Not owned.
On 2016/11/17 18:31:22, msw wrote:
> ditto nit

Done.

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/multi_user/multi_user_util_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_util_chromeos_unittest.cc:108:
chromeos::FakeChromeUserManager* fake_user_manager_;  // Not owned.
On 2016/11/17 18:31:22, msw wrote:
> nit: okay, this one is used; it'd be nice to either (1) still inline the
manager
> instantiation in the enabler ctor and provide an accessor on the enabler to
get
> the manager, or (2) replace the "Not owned." comment here with something more
> useful, like "Owned by |user_manager_enabler_|."

Done.

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_finder_chromeos_unittest.cc (right):

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_finder_chromeos_unittest.cc:103:
chromeos::FakeChromeUserManager* fake_user_manager_;  // Not owned.
On 2016/11/17 18:31:22, msw wrote:
> ditto nit

Done.

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/profile_info_handler_unittest.cc (right):

https://codereview.chromium.org/2504513002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/profile_info_handler_unittest.cc:90: //
|user_manager_| is destroyed by |user_manager_enabler_|.
On 2016/11/17 18:31:22, msw wrote:
> ditto nit: this comment is sufficient, but the |user_manager_| isn't accessed,
> so it could be removed and the instantiation inlined.

Done.

[yoshiki, 2016-11-18 15:54:22]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2016-11-18 15:54:23]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
achuith@chromium.org, asargent@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2504513002/#ps80001
(title: "Addressed comment")

[commit-bot: I haz the power, 2016-11-18 15:54:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 16:00:25]

Description was changed from

==========
Make FakeChromeUserManager subclass of ChromeUserManager

This patch does:
- Make FakeChromeUserManager a subclass of ChromeUserManager
- Use FakeChromeUserManager in the tests intead of FakeUserManager

FakeChromeUserManager has to be a subclass of ChromeUserManager in the
chromeos-related tests, since ChromeUserManager::Get() does static_cast the user
manager instance to ChromeUserManager type.

Previously, an instance of FakeChromeUserManager, which didn't inherit
ChromeUserManager, was casted to ChromeUserManager instance wrongly, and it
caused a bit-rot and some crashes in some tests. This patch re-enables some of
them.

Following tests are still failling due to other reason. We need to fix a cause
and reenable them separately.
- UserCloudPolicyManagerChromeOSTest.BlockingFetchTimeout
- UserCloudPolicyManagerChromeOSTest.BlockingRefreshFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingFirstFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingRefreshFetch


BUG=534733
TEST=trybots pass
R=achuith@chromium.org
==========

to

==========
Make FakeChromeUserManager subclass of ChromeUserManager

This patch does:
- Make FakeChromeUserManager a subclass of ChromeUserManager
- Use FakeChromeUserManager in the tests intead of FakeUserManager

FakeChromeUserManager has to be a subclass of ChromeUserManager in the
chromeos-related tests, since ChromeUserManager::Get() does static_cast the user
manager instance to ChromeUserManager type.

Previously, an instance of FakeChromeUserManager, which didn't inherit
ChromeUserManager, was casted to ChromeUserManager instance wrongly, and it
caused a bit-rot and some crashes in some tests. This patch re-enables some of
them.

Following tests are still failling due to other reason. We need to fix a cause
and reenable them separately.
- UserCloudPolicyManagerChromeOSTest.BlockingFetchTimeout
- UserCloudPolicyManagerChromeOSTest.BlockingRefreshFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingFirstFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingRefreshFetch


BUG=534733
TEST=trybots pass
R=achuith@chromium.org
==========

[commit-bot: I haz the power, 2016-11-18 16:00:26]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-11-18 16:03:25]

Description was changed from

==========
Make FakeChromeUserManager subclass of ChromeUserManager

This patch does:
- Make FakeChromeUserManager a subclass of ChromeUserManager
- Use FakeChromeUserManager in the tests intead of FakeUserManager

FakeChromeUserManager has to be a subclass of ChromeUserManager in the
chromeos-related tests, since ChromeUserManager::Get() does static_cast the user
manager instance to ChromeUserManager type.

Previously, an instance of FakeChromeUserManager, which didn't inherit
ChromeUserManager, was casted to ChromeUserManager instance wrongly, and it
caused a bit-rot and some crashes in some tests. This patch re-enables some of
them.

Following tests are still failling due to other reason. We need to fix a cause
and reenable them separately.
- UserCloudPolicyManagerChromeOSTest.BlockingFetchTimeout
- UserCloudPolicyManagerChromeOSTest.BlockingRefreshFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingFirstFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingRefreshFetch


BUG=534733
TEST=trybots pass
R=achuith@chromium.org
==========

to

==========
Make FakeChromeUserManager subclass of ChromeUserManager

This patch does:
- Make FakeChromeUserManager a subclass of ChromeUserManager
- Use FakeChromeUserManager in the tests intead of FakeUserManager

FakeChromeUserManager has to be a subclass of ChromeUserManager in the
chromeos-related tests, since ChromeUserManager::Get() does static_cast the user
manager instance to ChromeUserManager type.

Previously, an instance of FakeChromeUserManager, which didn't inherit
ChromeUserManager, was casted to ChromeUserManager instance wrongly, and it
caused a bit-rot and some crashes in some tests. This patch re-enables some of
them.

Following tests are still failling due to other reason. We need to fix a cause
and reenable them separately.
- UserCloudPolicyManagerChromeOSTest.BlockingFetchTimeout
- UserCloudPolicyManagerChromeOSTest.BlockingRefreshFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingFirstFetch
- UserCloudPolicyManagerChromeOSTest.NonBlockingRefreshFetch

BUG=534733
TEST=trybots pass
R=achuith@chromium.org

Committed: https://crrev.com/18898fb63b4079de832cc14a1331a0b0853490f1
Cr-Commit-Position: refs/heads/master@{#433211}
==========

[commit-bot: I haz the power, 2016-11-18 16:03:27]

Patchset 4 (id:??) landed as
https://crrev.com/18898fb63b4079de832cc14a1331a0b0853490f1
Cr-Commit-Position: refs/heads/master@{#433211}