Delay default apps installation on Chrome OS for first time sign-in

Delay default apps installation on Chrome OS for first time sign-in

Default apps installation significantly affects session start so sometimes
even priority settings are not able to sync in time.

BUG=424075
TEST=manual

Committed: https://crrev.com/a772ed62ff0641df984feffe977b1545e94d53f1
Cr-Commit-Position: refs/heads/master@{#301599}

[Dmitry Polukhin, 2014-10-23 14:11:15]

dpolukhin@chromium.org changed reviewers:
+ asargent@chromium.org, nkostylev@chromium.org

[Dmitry Polukhin, 2014-10-23 14:11:16]

PTAL

[Nikita (slow), 2014-10-23 14:22:47]

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_pref_loader.cc (right):

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_loader.cc:28: // Keep this delay greater
or close to priority settings sync timeout (10 sec)
For the reference: kSyncTimeoutSeconds.

[Nikita (slow), 2014-10-23 14:24:16]

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/external_provider_impl.cc:512: int
oem_extension_creation_flags =
What about OEM default apps?

[asargent_no_longer_on_chrome, 2014-10-23 22:34:35]

lgtm w/ a few suggestions

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_pref_loader.cc (right):

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_loader.cc:28: // Keep this delay greater
or close to priority settings sync timeout (10 sec)
On 2014/10/23 14:22:47, Nikita Kostylev wrote:
> For the reference: kSyncTimeoutSeconds.

optional: Is it worth putting this constant name into the actual comment? Or
maybe even making the kMsDelayPrefLoad value be computed as (kSyncTimeoutSeconds
+ 5000) or (kSyncTimeoutSeconds * 1.5) or something?

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_pref_loader.h (right):

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_loader.h:33: // to don't overload system
on first time user login.
grammar nit: should be "... to not overload the system ..."

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/external_provider_impl.cc:500:
ExternalPrefLoader::DELAY_LOAD : ExternalPrefLoader::NONE;
A thought just occurred to me - is there a way to know that priority pref
settings coming down via sync are pending, or complete? E.g. it would be nice if
there weren't pending settings to just start this immediately, or if the
settings arrived say after 8 seconds, to not have to wait the extra 7 seconds to
begin installing default apps. 

If this bug is urgent or that's a more complicated change than you want to take
on for now, that could certainly just be logged as a follow-up bug or something
(if you agree it's even a good idea).

[Dmitry Polukhin, 2014-10-24 11:40:42]

PTAL

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_pref_loader.cc (right):

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_loader.cc:28: // Keep this delay greater
or close to priority settings sync timeout (10 sec)
On 2014/10/23 22:34:35, asargent wrote:
> On 2014/10/23 14:22:47, Nikita Kostylev wrote:
> > For the reference: kSyncTimeoutSeconds.
> 
> optional: Is it worth putting this constant name into the actual comment? Or
> maybe even making the kMsDelayPrefLoad value be computed as
(kSyncTimeoutSeconds
> + 5000) or (kSyncTimeoutSeconds * 1.5) or something?
> 

Removed this constant.

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_pref_loader.h (right):

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_loader.h:33: // to don't overload system
on first time user login.
On 2014/10/23 22:34:35, asargent wrote:
> grammar nit: should be "... to not overload the system ..."

Done.

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/external_provider_impl.cc:500:
ExternalPrefLoader::DELAY_LOAD : ExternalPrefLoader::NONE;
On 2014/10/23 22:34:35, asargent wrote:
> A thought just occurred to me - is there a way to know that priority pref
> settings coming down via sync are pending, or complete? E.g. it would be nice
if
> there weren't pending settings to just start this immediately, or if the
> settings arrived say after 8 seconds, to not have to wait the extra 7 seconds
to
> begin installing default apps. 
> 
> If this bug is urgent or that's a more complicated change than you want to
take
> on for now, that could certainly just be logged as a follow-up bug or
something
> (if you agree it's even a good idea). 

Good idea, done.

https://codereview.chromium.org/650763003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/external_provider_impl.cc:512: int
oem_extension_creation_flags =
On 2014/10/23 14:24:16, Nikita Kostylev wrote:
> What about OEM default apps?

I don't postponed apps from sync installation so there is no sense to block OEM
default apps. Especially because these ones for LOEM and should require manifest
fetch and most probably light weight. Will reconsider later if it pops up.

[Nikita (slow), 2014-10-27 09:44:49]

lgtm

https://codereview.chromium.org/650763003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/external_pref_loader.cc (right):

https://codereview.chromium.org/650763003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_pref_loader.cc:14: #include
"base/metrics/histogram.h"
#include "chrome/browser/profiles/profile.h"

https://codereview.chromium.org/650763003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_pref_loader.cc:97: :
base_path_id_(base_path_id), options_(options),
nit: one initialization per line now that they all don't fit on a single line.

or just run git cl format which is more convenient.

https://codereview.chromium.org/650763003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_pref_loader.cc:121: PrefServiceSyncable*
prefs = PrefServiceSyncable::FromProfile(profile_);
nit: DCHECK(profile_);

https://codereview.chromium.org/650763003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_pref_loader.cc:128: BrowserThread::FILE,
FROM_HERE,
nit: As a follow up change you can get rid of FILE thread usage here.

[Dmitry Polukhin, 2014-10-27 14:48:48]

https://codereview.chromium.org/650763003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/external_pref_loader.cc (right):

https://codereview.chromium.org/650763003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_pref_loader.cc:14: #include
"base/metrics/histogram.h"
On 2014/10/27 09:44:49, Nikita Kostylev wrote:
> #include "chrome/browser/profiles/profile.h"

Discussed offline, I don't use Profile, only pass pointer.

https://codereview.chromium.org/650763003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_pref_loader.cc:97: :
base_path_id_(base_path_id), options_(options),
On 2014/10/27 09:44:49, Nikita Kostylev wrote:
> nit: one initialization per line now that they all don't fit on a single line.
> 
> or just run git cl format which is more convenient.

Done.

https://codereview.chromium.org/650763003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_pref_loader.cc:121: PrefServiceSyncable*
prefs = PrefServiceSyncable::FromProfile(profile_);
On 2014/10/27 09:44:49, Nikita Kostylev wrote:
> nit: DCHECK(profile_);

Done.

[asargent_no_longer_on_chrome, 2014-10-27 16:06:36]

new version lgtm w/ one suggestion

https://codereview.chromium.org/650763003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/external_pref_loader.h (right):

https://codereview.chromium.org/650763003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/external_pref_loader.h:98: bool pref_observer_added_;
you might consider using base/scoped_observer.h instead of this boolean and the
manual code in the destructor

[Dmitry Polukhin, 2014-10-28 09:50:24]

Thank you for review!

https://codereview.chromium.org/650763003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/external_pref_loader.h (right):

https://codereview.chromium.org/650763003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/external_pref_loader.h:98: bool pref_observer_added_;
On 2014/10/27 16:06:36, asargent wrote:
> you might consider using base/scoped_observer.h instead of this boolean and
the
> manual code in the destructor

Done.

[Dmitry Polukhin, 2014-10-28 09:50:27]

The CQ bit was checked by dpolukhin@chromium.org

[commit-bot: I haz the power, 2014-10-28 09:51:49]

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

[commit-bot: I haz the power, 2014-10-28 11:55:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-28 11:55:57]

Patchset 5 (id:??) landed as
https://crrev.com/a772ed62ff0641df984feffe977b1545e94d53f1
Cr-Commit-Position: refs/heads/master@{#301599}

[binjin, 2015-02-19 14:42:54]

binjin@chromium.org changed reviewers:
+ binjin@chromium.org

[binjin, 2015-02-19 14:42:54]

Nikita, Antony: Could you please have a look at this as reviewers of the
original CL. See https://codereview.chromium.org/650763003

[binjin, 2015-02-19 14:45:08]

On 2015/02/19 14:42:54, binjin wrote:
> Nikita, Antony: Could you please have a look at this as reviewers of the
> original CL. See https://codereview.chromium.org/650763003

Ooops, sorry, wrong thread.

[binjin, 2015-02-19 14:47:39]

On 2015/02/19 14:45:08, binjin wrote:
> On 2015/02/19 14:42:54, binjin wrote:
> > Nikita, Antony: Could you please have a look at this as reviewers of the
> > original CL. See https://codereview.chromium.org/650763003
> 
> Ooops, sorry, wrong thread.

See https://codereview.chromium.org/931663007/