Always load signin profile on Chrome OS startup.

Always load signin profile on Chrome OS startup.
Starting a Chrome OS session (e.g. on Linux build) using "--login-profile=" and then pressing Ctrl+Shift+L triggers some nasty behavior, as the sign-in profile was not loaded yet (which is not the case during normal boot-up, as this profile is usually the first thing that loads). This patch aims to force its loading for such cases.
This affects not only stub login, but also any session that was restored.
Current patch reiterates CL from https://codereview.chromium.org/240333011/

BUG=346763
Committed: https://crrev.com/9d3e646a9cc0ef50320b5c30847886a3e0171adc
Cr-Commit-Position: refs/heads/master@{#318220}

[Ivan Podogov, 2015-02-17 14:39:10]

ginkage@chromium.org changed reviewers:
+ mattm@chromium.org, mtomasz@chromium.org, nkostylev@chromium.org,
skuhne@chromium.org

[Ivan Podogov, 2015-02-17 14:40:09]

PTAL

nkostylev@ for chrome/browser/chromeos/
mtomasz@ for chrome/browser/*file*
skuhne@ for chrome/browser/profiles/
mattm@ for chrome/browser/safe_browsing/

[Nikita (slow), 2015-02-17 14:44:55]

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:538: if
(!chromeos::ProfileHelper::IsSigninProfile(profile()))
We're discussing with Pavel that another potential optimization here would be
loading "signin profile" in async way since it is not required for session
restore (after crash/just restart) case.

However there's a potential edge case:
* session restore starts
  ** "signin profile" async loading is started
* screen lock is activated which tries to access "signin profile".

[Nikita (slow), 2015-02-17 14:46:24]

On 2015/02/17 14:40:09, Ivan Podogov wrote:
> PTAL
> 
> nkostylev@ for chrome/browser/chromeos/

lgtm

[Mr4D (OOO till 08-26), 2015-02-17 15:46:51]

lgtm for c/b/p

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:815: ->GetSigninProfileDir() !=
profile->GetPath() &&
You might want to add a comment that a supervised user cannot be added to a
multi profile scenario (or can it?).

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:277: if (idx == index)
You could do a "idx++" here to eliminate line 279.

Nit: You might then use != in 274 and remove the continue.

[Nikita (slow), 2015-02-17 16:40:22]

Please change description to smth else.
Instead of "Reiterate dzhioev's patch" link to that CL.

[mattm, 2015-02-17 23:13:06]

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1032: // On
Chrome OS we should disable safe browsing for signin profile.
Comment seemed a little confusing.
Is "On Chrome OS we also need to disable safe browsing for signin profile."
correct?

More importantly though, this is worrisome. Does it mean on real devices
safebrowsing service will always be enabled, even if the user has disabled
safebrowsing option on their profile(s)? (Would lead to wasting bandwidth
updating the safebrowsing lists when nothing is actually going to use them.)

[mtomasz, 2015-02-18 03:13:07]

I know very little about signin profiles, but this change looks complex. Are we
sure it can't be done simpler?

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc
(right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc:66:
drive::DriveIntegrationService* CreateDriveIntegrationService(
Could you provide some context? The CL description is empty, and not much info
in crbug.com. Why this is needed?

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc:74:
CHECK(fake_drive_service_ == NULL);
nit: How about ASSERT_FALSE?

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_browsertest.cc:363: #if defined(OS_CHROMEOS)
Why is this change needed?

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_browsertest.cc:364: for (size_t idx = 0; idx !=
loaded_profiles.size(); ++idx) {
for (const auto& ... : ...)

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:268: base::FilePath
GetNonSigninProfileAt(ProfileInfoCache& cache, size_t index) {
ProfileInfoCache& -> const ProfileInfoCache&

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:270: size_t i, idx = 0,
profile_num = cache.GetNumberOfProfiles();
We have here i, idx, index. It's really difficult to understand. Can we use
better naming?

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:299: unsigned int
initial_profile_count = profile_manager->GetNumberOfProfiles();
Is number of profiles "unsigned int" or "size_t"? See #270 vs. #299.

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:302:
ASSERT_NE(initial_profile_count, 0U);
Opposite argument order. Should be:
ASSERT(expected, actual)
EXPECT(expected, actual)

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:369: unsigned int
initial_profile_count = profile_manager->GetNumberOfProfiles();
ditto

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:420:
ASSERT_EQ(initial_profile_count, cache.GetNumberOfProfiles());
This can be EXPECT. AFAIK ASSERT should be used only if continuing would result
in a crash.

[Ivan Podogov, 2015-02-18 08:57:32]

New patchsets have been uploaded after l-g-t-m from
nkostylev@chromium.org,skuhne@chromium.org

[Ivan Podogov, 2015-02-18 09:03:30]

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_browsertest.cc:363: #if defined(OS_CHROMEOS)
On 2015/02/18 03:13:06, mtomasz wrote:
> Why is this change needed?

Because we force the sign-in profile to load, and the profile at index 0 might
not be an actual user profile, so we just look for the first non-signin one.

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1032: // On
Chrome OS we should disable safe browsing for signin profile.
On 2015/02/17 23:13:06, mattm wrote:
> Comment seemed a little confusing.
> Is "On Chrome OS we also need to disable safe browsing for signin profile."
> correct?
> 
> More importantly though, this is worrisome. Does it mean on real devices
> safebrowsing service will always be enabled, even if the user has disabled
> safebrowsing option on their profile(s)? (Would lead to wasting bandwidth
> updating the safebrowsing lists when nothing is actually going to use them.)

Signin profile loads first thing during boot-up anyway. I'm not sure if the safe
browsing is enabled in it by default, but it doesn't really matter, as this
profile is not used for any browsing as such. We may check if that is the case,
but nevertheless, this CL doesn't actually change the existing behavior on
devices.

[Nikita (slow), 2015-02-19 14:48:09]

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1032: // On
Chrome OS we should disable safe browsing for signin profile.
On 2015/02/18 09:03:30, Ivan Podogov wrote:
> On 2015/02/17 23:13:06, mattm wrote:
> > Comment seemed a little confusing.
> > Is "On Chrome OS we also need to disable safe browsing for signin profile."
> > correct?
> > 
> > More importantly though, this is worrisome. Does it mean on real devices
> > safebrowsing service will always be enabled, even if the user has disabled
> > safebrowsing option on their profile(s)? (Would lead to wasting bandwidth
> > updating the safebrowsing lists when nothing is actually going to use them.)
> 
> Signin profile loads first thing during boot-up anyway. I'm not sure if the
safe
> browsing is enabled in it by default, but it doesn't really matter, as this
> profile is not used for any browsing as such. We may check if that is the
case,
> but nevertheless, this CL doesn't actually change the existing behavior on
> devices.

To clarify what Ivan said, signin profile is in fact used for some limited
"browsing" i.e. when Chrome OS user signs in.

I think safebrowsing is probably enabled for it (not sure though).
But as Ivan mentioned this CL doesn't change how things work for production code
in terms of signin profile and safebrowsing.

On Chrome OS fresh boot signin profile always loads. After user signs in it is
still alive (browsing data is cleared though). Things differ if Chrome crashes
and is restarted while inside user session. Then user profile is loaded
synchronously on Chrome startup + now signin profile will load as well. It will
be used on lock screen.

This particular test works with its own profile: pref_service2.

Signin profile is now always created for "session restore after crash" / browser
restart case which happen to be the same that is used for running tests.

Therefore signin profile needs to be marked here so that it doesn't interfere
with tests purposes.

[mtomasz, 2015-02-20 02:00:42]

(Please mark addressed comments with Done).

https://codereview.chromium.org/933503004/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:274: #if
defined(OS_CHROMEOS)
On OS different than Chrome OS, this loop is unnecessary. How about moving the
define around the loop?

[Ivan Podogov, 2015-02-24 11:43:17]

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc
(right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc:66:
drive::DriveIntegrationService* CreateDriveIntegrationService(
On 2015/02/18 03:13:06, mtomasz wrote:
> Could you provide some context? The CL description is empty, and not much info
> in http://crbug.com. Why this is needed?

Done.

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc:74:
CHECK(fake_drive_service_ == NULL);
On 2015/02/18 03:13:06, mtomasz wrote:
> nit: How about ASSERT_FALSE?

There are some problems with assert macro that prevent such check from being
compiled. Anyway, there's no crash if this check is false, so no need for an
assert IMHO.

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_browsertest.cc:364: for (size_t idx = 0; idx !=
loaded_profiles.size(); ++idx) {
On 2015/02/18 03:13:06, mtomasz wrote:
> for (const auto& ... : ...)

Done.

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:268: base::FilePath
GetNonSigninProfileAt(ProfileInfoCache& cache, size_t index) {
On 2015/02/18 03:13:06, mtomasz wrote:
> ProfileInfoCache& -> const ProfileInfoCache&

Done.

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:270: size_t i, idx = 0,
profile_num = cache.GetNumberOfProfiles();
On 2015/02/18 03:13:06, mtomasz wrote:
> We have here i, idx, index. It's really difficult to understand. Can we use
> better naming?

Done.

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:277: if (idx == index)
On 2015/02/17 15:46:51, Mr4D wrote:
> You could do a "idx++" here to eliminate line 279.
> 
> Nit: You might then use != in 274 and remove the continue.

Done.

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:299: unsigned int
initial_profile_count = profile_manager->GetNumberOfProfiles();
On 2015/02/18 03:13:07, mtomasz wrote:
> Is number of profiles "unsigned int" or "size_t"? See #270 vs. #299.

Done.

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:302:
ASSERT_NE(initial_profile_count, 0U);
On 2015/02/18 03:13:06, mtomasz wrote:
> Opposite argument order. Should be:
> ASSERT(expected, actual)
> EXPECT(expected, actual)

Done.

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:369: unsigned int
initial_profile_count = profile_manager->GetNumberOfProfiles();
On 2015/02/18 03:13:07, mtomasz wrote:
> ditto

Done.

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:420:
ASSERT_EQ(initial_profile_count, cache.GetNumberOfProfiles());
On 2015/02/18 03:13:06, mtomasz wrote:
> This can be EXPECT. AFAIK ASSERT should be used only if continuing would
result
> in a crash.

Done.

https://codereview.chromium.org/933503004/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:274: #if
defined(OS_CHROMEOS)
On 2015/02/20 02:00:42, mtomasz wrote:
> On OS different than Chrome OS, this loop is unnecessary. How about moving the
> define around the loop?

Done.

[mattm, 2015-02-24 20:39:06]

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc
(right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc:66:
drive::DriveIntegrationService* CreateDriveIntegrationService(
On 2015/02/24 11:43:16, Ivan Podogov wrote:
> On 2015/02/18 03:13:06, mtomasz wrote:
> > Could you provide some context? The CL description is empty, and not much
info
> > in http://crbug.com. Why this is needed?
> 
> Done.

nit: update the CL description so the first line is a summary of the CL. (The
codereview subject field is independent of the CL description, so you need to
repeat that in the description.)

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1032: // On
Chrome OS we should disable safe browsing for signin profile.
On 2015/02/19 14:48:09, Nikita wrote:
> On 2015/02/18 09:03:30, Ivan Podogov wrote:
> > On 2015/02/17 23:13:06, mattm wrote:
> > > Comment seemed a little confusing.
> > > Is "On Chrome OS we also need to disable safe browsing for signin
profile."
> > > correct?
> > > 
> > > More importantly though, this is worrisome. Does it mean on real devices
> > > safebrowsing service will always be enabled, even if the user has disabled
> > > safebrowsing option on their profile(s)? (Would lead to wasting bandwidth
> > > updating the safebrowsing lists when nothing is actually going to use
them.)
> > 
> > Signin profile loads first thing during boot-up anyway. I'm not sure if the
> safe
> > browsing is enabled in it by default, but it doesn't really matter, as this
> > profile is not used for any browsing as such. We may check if that is the
> case,
> > but nevertheless, this CL doesn't actually change the existing behavior on
> > devices.
> 
> To clarify what Ivan said, signin profile is in fact used for some limited
> "browsing" i.e. when Chrome OS user signs in.
> 
> I think safebrowsing is probably enabled for it (not sure though).
> But as Ivan mentioned this CL doesn't change how things work for production
code
> in terms of signin profile and safebrowsing.
> 
> On Chrome OS fresh boot signin profile always loads. After user signs in it is
> still alive (browsing data is cleared though). Things differ if Chrome crashes
> and is restarted while inside user session. Then user profile is loaded
> synchronously on Chrome startup + now signin profile will load as well. It
will
> be used on lock screen.
> 
> This particular test works with its own profile: pref_service2.
> 
> Signin profile is now always created for "session restore after crash" /
browser
> restart case which happen to be the same that is used for running tests.
> 
> Therefore signin profile needs to be marked here so that it doesn't interfere
> with tests purposes.

Okay, I verified that is the case and filed
https://code.google.com/p/chromium/issues/detail?id=461493 for that issue.
Could you add a TODO "remove this when crbug.com/461493 is fixed"?

[mtomasz, 2015-02-25 05:49:41]

https://codereview.chromium.org/933503004/diff/160001/chrome/browser/profiles...
File chrome/browser/profiles/profile_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/160001/chrome/browser/profiles...
chrome/browser/profiles/profile_browsertest.cc:18: #include
"chrome/browser/chromeos/profiles/profile_helper.h"
Shall this be included only for chromeos? See #32.

https://codereview.chromium.org/933503004/diff/160001/chrome/browser/profiles...
chrome/browser/profiles/profile_browsertest.cc:366: profile = loaded_profile;
nit: Shall we add break;?

https://codereview.chromium.org/933503004/diff/160001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/160001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:268: base::FilePath
GetNonSigninProfileAt(const ProfileInfoCache& cache,
This can go to anon namespace next to other utility functions.

https://codereview.chromium.org/933503004/diff/160001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:275: if
(path.BaseName().value() == chrome::kInitialProfile)
Can we assume that all non-signin profiles are initial ones? I don't know. Shall
we check against GetSigninProfileDir instead?

https://codereview.chromium.org/933503004/diff/160001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:277: if (index-- == 0)
This method is always called with index = 0. Can we remove the |index| argument
and rename it to "GetFirstNonSigninProfile?"

[Ivan Podogov, 2015-02-25 11:11:04]

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/120001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1032: // On
Chrome OS we should disable safe browsing for signin profile.
On 2015/02/24 20:39:06, mattm wrote:
> On 2015/02/19 14:48:09, Nikita wrote:
> > On 2015/02/18 09:03:30, Ivan Podogov wrote:
> > > On 2015/02/17 23:13:06, mattm wrote:
> > > > Comment seemed a little confusing.
> > > > Is "On Chrome OS we also need to disable safe browsing for signin
> profile."
> > > > correct?
> > > > 
> > > > More importantly though, this is worrisome. Does it mean on real devices
> > > > safebrowsing service will always be enabled, even if the user has
disabled
> > > > safebrowsing option on their profile(s)? (Would lead to wasting
bandwidth
> > > > updating the safebrowsing lists when nothing is actually going to use
> them.)
> > > 
> > > Signin profile loads first thing during boot-up anyway. I'm not sure if
the
> > safe
> > > browsing is enabled in it by default, but it doesn't really matter, as
this
> > > profile is not used for any browsing as such. We may check if that is the
> > case,
> > > but nevertheless, this CL doesn't actually change the existing behavior on
> > > devices.
> > 
> > To clarify what Ivan said, signin profile is in fact used for some limited
> > "browsing" i.e. when Chrome OS user signs in.
> > 
> > I think safebrowsing is probably enabled for it (not sure though).
> > But as Ivan mentioned this CL doesn't change how things work for production
> code
> > in terms of signin profile and safebrowsing.
> > 
> > On Chrome OS fresh boot signin profile always loads. After user signs in it
is
> > still alive (browsing data is cleared though). Things differ if Chrome
crashes
> > and is restarted while inside user session. Then user profile is loaded
> > synchronously on Chrome startup + now signin profile will load as well. It
> will
> > be used on lock screen.
> > 
> > This particular test works with its own profile: pref_service2.
> > 
> > Signin profile is now always created for "session restore after crash" /
> browser
> > restart case which happen to be the same that is used for running tests.
> > 
> > Therefore signin profile needs to be marked here so that it doesn't
interfere
> > with tests purposes.
> 
> Okay, I verified that is the case and filed
> https://code.google.com/p/chromium/issues/detail?id=461493 for that issue.
> Could you add a TODO "remove this when crbug.com/461493 is fixed"?

Done.

https://codereview.chromium.org/933503004/diff/160001/chrome/browser/profiles...
File chrome/browser/profiles/profile_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/160001/chrome/browser/profiles...
chrome/browser/profiles/profile_browsertest.cc:18: #include
"chrome/browser/chromeos/profiles/profile_helper.h"
On 2015/02/25 05:49:41, mtomasz wrote:
> Shall this be included only for chromeos? See #32.

Done.

https://codereview.chromium.org/933503004/diff/160001/chrome/browser/profiles...
chrome/browser/profiles/profile_browsertest.cc:366: profile = loaded_profile;
On 2015/02/25 05:49:41, mtomasz wrote:
> nit: Shall we add break;?

Done.

https://codereview.chromium.org/933503004/diff/160001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/160001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:268: base::FilePath
GetNonSigninProfileAt(const ProfileInfoCache& cache,
On 2015/02/25 05:49:41, mtomasz wrote:
> This can go to anon namespace next to other utility functions.

Done.

https://codereview.chromium.org/933503004/diff/160001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:277: if (index-- == 0)
On 2015/02/25 05:49:41, mtomasz wrote:
> This method is always called with index = 0. Can we remove the |index|
argument
> and rename it to "GetFirstNonSigninProfile?"

Done.

[mattm, 2015-02-25 23:26:04]

safe_browsing lgtm

[mtomasz, 2015-02-26 04:27:50]

In the CL description please add an extra new line between the title and the
rest.

Please add TEST= if your team does it.

Besides, lgtm with nits. Cheers!

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc
(right):

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:538:
CHECK(fake_drive_service_ == NULL);
nit: This must not happen, so please change to a DCHECK.

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:144: return profile &&
nit: Passing null doesn't make sense. Can we add a DCHECK that |profile| is not
null instead and add a comment to IsSigninProfile() that |profile| must not be
null? AFAIK this is what we do usually.

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc
(right):

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc:74:
CHECK(fake_drive_service_ == NULL);
nit: ditto

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:119: base::FilePath
signin = chromeos::ProfileHelper::GetSigninProfileDir();
nit: This can be const
nit: Variable names shouldn't be verbs. How about signinPath?

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:415:
ASSERT_EQ(initial_profile_count + 1, cache.GetNumberOfProfiles());
nit: Can this be EXPECT_EQ? ASSERT should be used only when continuing would
result in a crash.

[Ivan Podogov, 2015-02-26 11:02:08]

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc
(right):

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:538:
CHECK(fake_drive_service_ == NULL);
On 2015/02/26 04:27:50, mtomasz wrote:
> nit: This must not happen, so please change to a DCHECK.

Done.

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:144: return profile &&
On 2015/02/26 04:27:50, mtomasz wrote:
> nit: Passing null doesn't make sense. Can we add a DCHECK that |profile| is
not
> null instead and add a comment to IsSigninProfile() that |profile| must not be
> null? AFAIK this is what we do usually.

I looked at the functions above and below this one, and somehow it doesn't seem
like this is what's usually done. :)
I'll keep it consistent with the rest of this file for now.

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc
(right):

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc:74:
CHECK(fake_drive_service_ == NULL);
On 2015/02/26 04:27:50, mtomasz wrote:
> nit: ditto

Done.

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:119: base::FilePath
signin = chromeos::ProfileHelper::GetSigninProfileDir();
On 2015/02/26 04:27:50, mtomasz wrote:
> nit: This can be const
> nit: Variable names shouldn't be verbs. How about signinPath?

Done.

https://codereview.chromium.org/933503004/diff/220001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:415:
ASSERT_EQ(initial_profile_count + 1, cache.GetNumberOfProfiles());
On 2015/02/26 04:27:50, mtomasz wrote:
> nit: Can this be EXPECT_EQ? ASSERT should be used only when continuing would
> result in a crash.

Done.

[Ivan Podogov, 2015-02-26 11:02:17]

The CQ bit was checked by ginkage@chromium.org

[Ivan Podogov, 2015-02-26 11:02:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
nkostylev@chromium.org, skuhne@chromium.org, mtomasz@chromium.org,
mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/933503004/#ps240001
(title: "Final nits.")

[commit-bot: I haz the power, 2015-02-26 11:03:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/933503004/240001

[commit-bot: I haz the power, 2015-02-26 12:18:17]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-02-26 12:19:03]

Patchset 13 (id:??) landed as
https://crrev.com/9d3e646a9cc0ef50320b5c30847886a3e0171adc
Cr-Commit-Position: refs/heads/master@{#318220}