Turning on MultiProfile by default for ChromeOS

Turning on MultiProfile by default for ChromeOS

Merged with issues 184563005, 273653004

BUG=364604, 351244, 349844
TEST=existing tests

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

[sky, 2014-05-14 13:52:56]

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/lifetime...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/lifetime...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:256:
command_line->AppendSwitch(
You have this in a lot of tests. Is there a more central place it can be put?

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

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/profiles...
chrome/browser/profiles/profile_list_desktop_browsertest.cc:135: EXPECT_EQ(1U,
browser_list->size());
This is the same as on 132. Do you really need it again 2 lines later?

[Mr4D (OOO till 08-26), 2014-05-14 16:32:40]

Please see my comments!

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

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/path_util.cc:36: if
(!base::SysInfo::IsRunningOnChromeOS()) {
Indenting

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils_browsertest.cc:536: //     
command_line->AppendSwitch(::switches::kMultiProfiles);
Sorry didn't get around to do this before passing on. Please remove the lines
and maybe even this class?

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user.h (right):

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user.h:242: void set_given_name(const
base::string16& given_name) { given_name_ = given_name; }
Can you please make this into 80 chars?

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager.cc (right):

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager.cc:61:
UserManager::UserAccountData::UserAccountData(const base::string16&
display_name,
Sorry to ask, but could you please also break this accordingly?

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.cc:685: // and login several
users if they want to work with an additional profile.
It feels rather odd since why should e.g. android/windows/.. specify this flag
instead of the other way around?

Just thinking - could we maybe add something like "--os_chromeos_only_test" to
the test framework which does explicit chrome os testing and all other tests
ignore this?

That all said - it appears that the definition of the flag states that this is
temporary of nature and let's hope we remove it. :)

https://codereview.chromium.org/271533004/diff/220001/chrome/common/logging_c...
File chrome/common/logging_chrome.cc (right):

https://codereview.chromium.org/271533004/diff/220001/chrome/common/logging_c...
chrome/common/logging_chrome.cc:229: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kMultiProfiles) &&
We should resolve this one - or remove it. The flag will not be set anymore
which means that we will never get into this if case and either we handle this -
or ignore it.

[zel, 2014-05-14 17:48:39]

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

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/path_util.cc:36: if
(!base::SysInfo::IsRunningOnChromeOS()) {
On 2014/05/14 16:32:40, Mr4D wrote:
> Indenting

Done.

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils_browsertest.cc:536: //     
command_line->AppendSwitch(::switches::kMultiProfiles);
On 2014/05/14 16:32:40, Mr4D wrote:
> Sorry didn't get around to do this before passing on. Please remove the lines
> and maybe even this class?

Done.

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user.h (right):

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user.h:242: void set_given_name(const
base::string16& given_name) { given_name_ = given_name; }
On 2014/05/14 16:32:40, Mr4D wrote:
> Can you please make this into 80 chars?

Done.

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager.cc (right):

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager.cc:61:
UserManager::UserAccountData::UserAccountData(const base::string16&
display_name,
On 2014/05/14 16:32:40, Mr4D wrote:
> Sorry to ask, but could you please also break this accordingly?

how exactly should we break this?

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.cc:685: // and login several
users if they want to work with an additional profile.
On 2014/05/14 16:32:40, Mr4D wrote:
> It feels rather odd since why should e.g. android/windows/.. specify this flag
> instead of the other way around?
> 
> Just thinking - could we maybe add something like "--os_chromeos_only_test" to
> the test framework which does explicit chrome os testing and all other tests
> ignore this?
> 
> That all said - it appears that the definition of the flag states that this is
> temporary of nature and let's hope we remove it. :)

Yeah, this flag is not an ideal solution. We should remove it as soon as this CL
lands by cleaning up the real problem behind those tests.

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/lifetime...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/lifetime...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:256:
command_line->AppendSwitch(
On 2014/05/14 13:52:57, sky wrote:
> You have this in a lot of tests. Is there a more central place it can be put?

It wouldn't work for all tests where we needed it. This is temporary thing. We
will remove this switch soon after this lands.

https://codereview.chromium.org/271533004/diff/220001/chrome/common/logging_c...
File chrome/common/logging_chrome.cc (right):

https://codereview.chromium.org/271533004/diff/220001/chrome/common/logging_c...
chrome/common/logging_chrome.cc:229: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kMultiProfiles) &&
On 2014/05/14 16:32:40, Mr4D wrote:
> We should resolve this one - or remove it. The flag will not be set anymore
> which means that we will never get into this if case and either we handle this
-
> or ignore it.

Done.

[Mr4D (OOO till 08-26), 2014-05-14 18:14:32]

Looked over it again and lgtm.

Thanks for taking this over!

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager.cc (right):

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager.cc:61:
UserManager::UserAccountData::UserAccountData(const base::string16&
display_name,
UserManager::UserAccountData::UserAccountData(
    const base::string16& display_name,
    const base::string16& given_name,
    const std::string& locale)
    : display_name_(display_name),
      given_name_(given_name),
      locale_(locale) {
}

(sorry for nit picking)

[sky, 2014-05-14 19:13:28]

LGTM

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

lgtm

thanks for taking this over

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

https://codereview.chromium.org/271533004/diff/260001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:32: 
nit: drop extra empty line.

https://codereview.chromium.org/271533004/diff/260001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/path_util.cc (right):

https://codereview.chromium.org/271533004/diff/260001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/path_util.cc:37: // On the developer run on
Linux desktop build, if multiple profiles are
nit: drop multi-profiles mention

https://codereview.chromium.org/271533004/diff/260001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user.h (right):

https://codereview.chromium.org/271533004/diff/260001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user.h:200: static User* CreateRegularUser(const
std::string& email);
I don't see this being used in this CL so makes sense to move back to private
API.

[zel, 2014-05-14 21:17:57]

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

https://codereview.chromium.org/271533004/diff/260001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:32: 
On 2014/05/14 20:10:16, Nikita Kostylev wrote:
> nit: drop extra empty line.

Done.

https://codereview.chromium.org/271533004/diff/260001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/path_util.cc (right):

https://codereview.chromium.org/271533004/diff/260001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/path_util.cc:37: // On the developer run on
Linux desktop build, if multiple profiles are
On 2014/05/14 20:10:16, Nikita Kostylev wrote:
> nit: drop multi-profiles mention

Done.

https://codereview.chromium.org/271533004/diff/260001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user.h (right):

https://codereview.chromium.org/271533004/diff/260001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user.h:200: static User* CreateRegularUser(const
std::string& email);
On 2014/05/14 20:10:16, Nikita Kostylev wrote:
> I don't see this being used in this CL so makes sense to move back to private
> API.

Done.

[zel, 2014-05-14 21:22:09]

fingers crossed, all tests should be passing now

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager.cc (right):

https://codereview.chromium.org/271533004/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager.cc:61:
UserManager::UserAccountData::UserAccountData(const base::string16&
display_name,
On 2014/05/14 18:14:32, Mr4D wrote:
> UserManager::UserAccountData::UserAccountData(
>     const base::string16& display_name,
>     const base::string16& given_name,
>     const std::string& locale)
>     : display_name_(display_name),
>       given_name_(given_name),
>       locale_(locale) {
> }
> 
> (sorry for nit picking)

Done.

[zel, 2014-05-14 23:05:39]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-05-14 23:07:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zelidrag@chromium.org/271533004/310001

[commit-bot: I haz the power, 2014-05-15 01:03:23]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

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

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

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

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

[zel, 2014-05-15 01:09:30]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-05-15 01:10:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zelidrag@chromium.org/271533004/310001

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

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-15 02:59:18]

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

[commit-bot: I haz the power, 2014-05-15 02:59:19]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[zel, 2014-05-15 03:07:36]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-05-15 03:09:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zelidrag@chromium.org/271533004/310001

[commit-bot: I haz the power, 2014-05-15 04:01:53]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-15 04:37:02]

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

[commit-bot: I haz the power, 2014-05-15 04:37:03]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[zel, 2014-05-15 07:07:30]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-05-15 07:09:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zelidrag@chromium.org/271533004/310001

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

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

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

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

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

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[zel, 2014-05-15 15:36:00]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-05-15 15:38:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zelidrag@chromium.org/271533004/320001

[commit-bot: I haz the power, 2014-05-15 17:27:41]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-05-15 17:35:41]

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

[commit-bot: I haz the power, 2014-05-15 17:35:42]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[zel, 2014-05-15 17:47:00]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-05-15 17:47:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zelidrag@chromium.org/271533004/320001

[commit-bot: I haz the power, 2014-05-15 18:44:10]

Change committed as 270753