Restart Chrome on ChromeOS as early as possible to speed up restart.

Restart Chrome on ChromeOS as early as possible to speed up restart.

Chrome on ChromeOS is sometimes restarted on user session startup.
This CL moves restart as early as possible to prevent duplicating startup
steps before and after restart.

BUG=346913
TEST=manual

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

[Alexander Alekseev, 2014-06-06 16:43:45]

Please review.

Startup trace files after change shows that all necessary shutdown steps are
executed.

[Nikita (slow), 2014-06-10 14:21:00]

https://codereview.chromium.org/322533002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/322533002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:679: bool
LoginUtilsImpl::RestartToApplyPerSessionFlagsIfNeed(Profile* profile) {
nit: Move this to unnamed namespace.

https://codereview.chromium.org/322533002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:932: CommandLine
LoginUtilsImpl::CreatePerSessionCommandLine(Profile* profile) {
nit: Please move to unnamed namespace.

https://codereview.chromium.org/322533002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:940: bool
LoginUtilsImpl::NeedRestartToApplyPerSessionFlags(
nit: Same here.

https://codereview.chromium.org/322533002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:950: if
(about_flags::AreSwitchesIdenticalToCurrentCommandLine(
nit: rewrite this as 

return !about_flags::AreSwitchesIdenticalToCurrentCommandLine(...);

[Alexander Alekseev, 2014-06-10 22:05:14]

https://codereview.chromium.org/322533002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/322533002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:679: bool
LoginUtilsImpl::RestartToApplyPerSessionFlagsIfNeed(Profile* profile) {
On 2014/06/10 14:21:00, Nikita Kostylev wrote:
> nit: Move this to unnamed namespace.

It's called from profile_impl.cc .

I've moved it to a separate function in chromeos namespace.
But it is tricky, see other comment.

https://codereview.chromium.org/322533002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/322533002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.cc:982:
static_cast<LoginUtilsImpl*>(LoginUtilsImpl::Get())->AttemptRestart(profile);
Need a way to call LoginUtilsImpl method here.
Static_cast ?

[Nikita (slow), 2014-06-11 12:42:51]

Please revert to the first patchset and move only these functions:

CreatePerSessionCommandLine()
NeedRestartToApplyPerSessionFlags()

https://codereview.chromium.org/322533002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/322533002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:679: bool
LoginUtilsImpl::RestartToApplyPerSessionFlagsIfNeed(Profile* profile) {
On 2014/06/10 22:05:13, alemate wrote:
> On 2014/06/10 14:21:00, Nikita Kostylev wrote:
> > nit: Move this to unnamed namespace.
> 
> It's called from profile_impl.cc .
> 
> I've moved it to a separate function in chromeos namespace.
> But it is tricky, see other comment.

Sorry, since this one is called from ProfileImpl you should not make it a global
function instead which is not advised per Chromium style guide.

You should move only those function that are called from this class only.

[Alexander Alekseev, 2014-06-11 15:15:50]

https://codereview.chromium.org/322533002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/322533002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:679: bool
LoginUtilsImpl::RestartToApplyPerSessionFlagsIfNeed(Profile* profile) {
On 2014/06/11 12:42:51, Nikita Kostylev wrote:
> On 2014/06/10 22:05:13, alemate wrote:
> > On 2014/06/10 14:21:00, Nikita Kostylev wrote:
> > > nit: Move this to unnamed namespace.
> > 
> > It's called from profile_impl.cc .
> > 
> > I've moved it to a separate function in chromeos namespace.
> > But it is tricky, see other comment.
> 
> Sorry, since this one is called from ProfileImpl you should not make it a
global
> function instead which is not advised per Chromium style guide.
> 
> You should move only those function that are called from this class only.

Done.

[Nikita (slow), 2014-06-16 13:53:37]

As discussed, please add additional condition so that such an early Chrome
restart is only allowed when you're signin in as an existing user and is not
signing through GAIA (i.e. )

Otherwise for these cases you should leave existing behavior which postpones
Chrome restart till OAuth2 token is verified:
* signin as new user
* signin as an existing user with password changed (enter new password, taked
directly to GAIA flow)
* signin as an existing user, force GAIA sign in. For instance just go through
[+Add user] or enter password incorrectly 4 times in user pod, redirected to
GAIA.

using GAIA
ExistingUserController::CompleteLogin()
results in PerformLogin(user_context, LoginPerformer::AUTH_MODE_EXTENSION);

using user pod
ExistingUserController::Login()
results in
PerformLogin(user_context, LoginPerformer::AUTH_MODE_INTERNAL);

[Alexander Alekseev, 2014-06-17 18:31:14]

Please review.


On 2014/06/16 13:53:37, Nikita Kostylev wrote:
> As discussed, please add additional condition so that such an early Chrome
> restart is only allowed when you're signin in as an existing user and is not
> signing through GAIA (i.e. )
> 
> Otherwise for these cases you should leave existing behavior which postpones
> Chrome restart till OAuth2 token is verified:
> * signin as new user
> * signin as an existing user with password changed (enter new password, taked
> directly to GAIA flow)
> * signin as an existing user, force GAIA sign in. For instance just go through
> [+Add user] or enter password incorrectly 4 times in user pod, redirected to
> GAIA.
> 
> using GAIA
> ExistingUserController::CompleteLogin()
> results in PerformLogin(user_context, LoginPerformer::AUTH_MODE_EXTENSION);
> 
> using user pod
> ExistingUserController::Login()
> results in
> PerformLogin(user_context, LoginPerformer::AUTH_MODE_INTERNAL);

Done.

[Nikita (slow), 2014-06-18 12:23:30]

lgtm

https://codereview.chromium.org/322533002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/322533002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.cc:125: if (!controller)
Could be simplified as

return !controller ||
       (controller->auth_mode() == LoginPerformer::AUTH_MODE_INTERNAL &&
        controller->password_changed());

https://codereview.chromium.org/322533002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.cc:128: // Early restart is possible
only if user keys are up to date.
nit: I guess this should say "OAuth token is up to date"

https://codereview.chromium.org/322533002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils.h (right):

https://codereview.chromium.org/322533002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.h:94: // |early_restart| is true if
this restart attempt happens before user keys
nit: Before user profile is fully initialized.

[Alexander Alekseev, 2014-06-18 15:06:55]

https://codereview.chromium.org/322533002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/322533002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.cc:125: if (!controller)
On 2014/06/18 12:23:30, Nikita Kostylev wrote:
> Could be simplified as
> 
> return !controller ||
>        (controller->auth_mode() == LoginPerformer::AUTH_MODE_INTERNAL &&
>         controller->password_changed());

I decided to add comments here and rely on optimizer for speed.
That is why it is expanded.

https://codereview.chromium.org/322533002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.cc:128: // Early restart is possible
only if user keys are up to date.
On 2014/06/18 12:23:30, Nikita Kostylev wrote:
> nit: I guess this should say "OAuth token is up to date"

Done.

https://codereview.chromium.org/322533002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils.h (right):

https://codereview.chromium.org/322533002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.h:94: // |early_restart| is true if
this restart attempt happens before user keys
On 2014/06/18 12:23:30, Nikita Kostylev wrote:
> nit: Before user profile is fully initialized.

Done.

[Nikita (slow), 2014-06-20 08:32:25]

still lgtm

[Alexander Alekseev, 2014-06-24 14:44:22]

-davemoore@ (seems to be busy)
+rlp@

Please review 	chrome/browser/profiles/*

[rpetterson, 2014-06-24 19:54:41]

profiles LGTM

[Alexander Alekseev, 2014-06-24 20:30:54]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-06-24 20:31:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alemate@chromium.org/322533002/80001

[commit-bot: I haz the power, 2014-06-25 00:03:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/25183)

[commit-bot: I haz the power, 2014-06-25 00:13:52]

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

[commit-bot: I haz the power, 2014-06-25 00:13:53]

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

[Alexander Alekseev, 2014-06-26 01:49:12]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-06-26 01:50:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alemate@chromium.org/322533002/150001

[commit-bot: I haz the power, 2014-06-26 05:16:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/25914)

[commit-bot: I haz the power, 2014-06-26 05:28:19]

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

[commit-bot: I haz the power, 2014-06-26 05:28:21]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/25914)

[Alexander Alekseev, 2014-06-26 14:27:17]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-06-26 14:30:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alemate@chromium.org/322533002/170001

[commit-bot: I haz the power, 2014-06-26 17:48:15]

Change committed as 280045

[Nikita (slow), 2014-06-27 17:19:46]

https://codereview.chromium.org/322533002/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/322533002/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils.cc:122: LOG(ERROR) <<
"CanPerformEarlyRestart(): called.";
I don't think that this LOG(ERROR) is really needed.
Should be either removed or downgraded to VLOG(1).