ChromeOS: switch UI language before apps are loaded.

ChromeOS: switch UI language before apps are loaded.

Chrome Apps cannot switch UI language, so we need to initialize apps after
app_locale is set correctly.

This CL splits ProfileImpl::OnPrefsLoaded into two steps
(the former and OnLocaleReady), adding intermediate call
to switch to correct locale. Also Profile is added as
parameter to InputMethodChanged() observers.
chromeos::locale_util::SwitchLanguage() now updates default
input methods for user profile, not currently active set.

TBR=dmazzoni@chromium.org

BUG=469268
TEST=manual

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

[Alexander Alekseev, 2015-04-02 16:46:42]

alemate@chromium.org changed reviewers:
+ nkostylev@chromium.org, noms@chromium.org

[Alexander Alekseev, 2015-04-02 16:46:42]

Please review:

noms@ : profile_impl*
nkostylev@ : all

[Alexander Alekseev, 2015-04-02 16:46:48]

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

[commit-bot: I haz the power, 2015-04-02 16:48:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1055863002/1

[commit-bot: I haz the power, 2015-04-02 17:01:39]

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

[commit-bot: I haz the power, 2015-04-02 17:01:42]

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

[Nikita (slow), 2015-04-02 17:37:05]

lgtm

https://codereview.chromium.org/1055863002/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/1055863002/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:907: TRACE_EVENT0("browser",
"ProfileImpl::OnPrefsLoaded");
As discussed, move trace event and histogram to OnLanguageSwitched()

[noms (inactive), 2015-04-02 18:43:11]

noms@chromium.org changed reviewers:
+ gab@chromium.org

[noms (inactive), 2015-04-02 18:43:12]

Going to pass this one on to Gab, since I don't know anything about prefs,
really.

[Alexander Alekseev, 2015-04-02 18:47:28]

https://codereview.chromium.org/1055863002/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/1055863002/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:907: TRACE_EVENT0("browser",
"ProfileImpl::OnPrefsLoaded");
On 2015/04/02 17:37:05, Nikita Kostylev wrote:
> As discussed, move trace event and histogram to OnLanguageSwitched()

Let's move only histogram, as trace event should be recorded even if prefs
failed to read.

[gab, 2015-04-02 18:49:49]

On 2015/04/02 18:43:12, noms (slow. ping if too slow) wrote:
> Going to pass this one on to Gab, since I don't know anything about prefs,
> really.

Why does this have to be synchronously blocking startup? If Apps need to be
deferred, sad but okay I guess, but shouldn't delay loading the entire profile
(i.e. which synchronously blocks all of startup).

Also, it feels kind of weird to have a "language" specific hook inside of
OnPrefsLoaded, better abstraction would be nicer IMO (e.g.,
delegate/observers?).

[Alexander Alekseev, 2015-04-02 21:05:27]

On 2015/04/02 18:49:49, gab wrote:
> On 2015/04/02 18:43:12, noms (slow. ping if too slow) wrote:
> > Going to pass this one on to Gab, since I don't know anything about prefs,
> > really.
> 
> Why does this have to be synchronously blocking startup? If Apps need to be
> deferred, sad but okay I guess, but shouldn't delay loading the entire profile
> (i.e. which synchronously blocks all of startup).



I've added hook to ExtensionSystem::Get and saw numerous app requests from
OnProfileLoaded / OnProfilePrepared hooks.

The first one is this:
[7584:7584:0402/235426:ERROR:extension_system.cc(25)]
ExtensionSystem::Get('u-.....') called at:
#0 0x00007117b428 base::debug::StackTrace::StackTrace()
#1 0x0000751019de extensions::ExtensionSystem::Get()
#2 0x000070b482c2 ProfileIOData::InitializeOnUIThread()
#3 0x000070e485ce ProfileImplIOData::Handle::LazyInitialize()
#4 0x000070e48e05 ProfileImplIOData::Handle::GetResourceContext()
#5 0x000070e41162 ProfileImpl::GetResourceContext()
#6 0x000073f31506 content::StoragePartitionImplMap::Get()
#7 0x000073d86950 content::BrowserContext::GetStoragePartition()
#8 0x000073d86d0b content::BrowserContext::GetDefaultStoragePartition()
#9 0x000070e40d1f ProfileImpl::GetRequestContext()
#10 0x000070e49807 ProfileImplIOData::Handle::Init()
#11 0x000070e455b2 ProfileImpl::DoFinalInit()
#12 0x000070e465a5 ProfileImpl::OnPrefsLoaded()
...

Then component extensions come: we will apply user keyboard preferences, which
would trigger input methods extension initialization (and ALL input method names
would be incorrect). Then wallpaper preference would trigger wallpaper
extension.

And so on.

I don't see any (easy) way to do asynchronous language resources reload. It
would require rewriting a lot of code.


> 
> Also, it feels kind of weird to have a "language" specific hook inside of
> OnPrefsLoaded, better abstraction would be nicer IMO (e.g.,
> delegate/observers?).

I'll try to replace it with something like |before_on_prefs_loader_hook_|.

[Alexander Alekseev, 2015-04-07 19:13:27]

gab@ : please review.

[gab, 2015-04-08 15:13:20]

gab@chromium.org changed reviewers:
+ robliao@chromium.org

[gab, 2015-04-08 15:15:42]

@Rob's, see below.

On 2015/04/02 21:05:27, Alexander Alekseev wrote:
> On 2015/04/02 18:49:49, gab wrote:
> > On 2015/04/02 18:43:12, noms (slow. ping if too slow) wrote:
> > > Going to pass this one on to Gab, since I don't know anything about prefs,
> > > really.
> > 
> > Why does this have to be synchronously blocking startup? If Apps need to be
> > deferred, sad but okay I guess, but shouldn't delay loading the entire
profile
> > (i.e. which synchronously blocks all of startup).
> 
> 
> 
> I've added hook to ExtensionSystem::Get and saw numerous app requests from
> OnProfileLoaded / OnProfilePrepared hooks.> 
> The first one is this:
> [7584:7584:0402/235426:ERROR:extension_system.cc(25)]
> ExtensionSystem::Get('u-.....') called at:
> #0 0x00007117b428 base::debug::StackTrace::StackTrace()
> #1 0x0000751019de extensions::ExtensionSystem::Get()
> #2 0x000070b482c2 ProfileIOData::InitializeOnUIThread()
> #3 0x000070e485ce ProfileImplIOData::Handle::LazyInitialize()
> #4 0x000070e48e05 ProfileImplIOData::Handle::GetResourceContext()
> #5 0x000070e41162 ProfileImpl::GetResourceContext()
> #6 0x000073f31506 content::StoragePartitionImplMap::Get()
> #7 0x000073d86950 content::BrowserContext::GetStoragePartition()
> #8 0x000073d86d0b content::BrowserContext::GetDefaultStoragePartition()
> #9 0x000070e40d1f ProfileImpl::GetRequestContext()
> #10 0x000070e49807 ProfileImplIOData::Handle::Init()
> #11 0x000070e455b2 ProfileImpl::DoFinalInit()
> #12 0x000070e465a5 ProfileImpl::OnPrefsLoaded()
> ...
> 
> Then component extensions come: we will apply user keyboard preferences, which
> would trigger input methods extension initialization (and ALL input method
names
> would be incorrect). Then wallpaper preference would trigger wallpaper
> extension.
> 
> And so on.
> 
> I don't see any (easy) way to do asynchronous language resources reload. It
> would require rewriting a lot of code.


+robliao/rkaplow who are currently looking into making extension initialization
asynchronous.



> 
> 
> > 
> > Also, it feels kind of weird to have a "language" specific hook inside of
> > OnPrefsLoaded, better abstraction would be nicer IMO (e.g.,
> > delegate/observers?).
> 
> I'll try to replace it with something like |before_on_prefs_loader_hook_|.

[robliao, 2015-04-13 17:32:30]

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.cc:497: const
base::Callback<void(Profile*, const base::Closure&)>& /* callback*/) {
Nit: Space after the k in callback.

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:282: // This hook interrupts initialization
flow.
It seems what we really want to do is wait for all values to be ready before
actually going down the OnPrefsLoaded path.

If locale should be treated as a pref, can we delay OnPrefsLoaded? All other
components waiting on this pref would benefit.

If locale should not be treated as a pref, can we separate out the relevant
calls in OnPrefsLoaded to something like OnLocaleLoaded? This way it's clear
that we're specifically waiting on locale to get loaded.

[gab, 2015-04-13 17:44:57]

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:282: // This hook interrupts initialization
flow.
On 2015/04/13 17:32:30, robliao wrote:
> It seems what we really want to do is wait for all values to be ready before
> actually going down the OnPrefsLoaded path.
> 
> If locale should be treated as a pref, can we delay OnPrefsLoaded? All other
> components waiting on this pref would benefit.

We have the concept of a PrefFilter which can be attached to a JsonPrefStore and
gets first bid before prefs are handed to Chrome (putting this out there, though
I'm not convinced yet that we want this to synchronously block all other prefs).

@Rob: do you think this could be made asynchronous (i.e. it's synchronous today
because extensions are loaded synchronously, could this become one of the async
requirements to begin async load of extensions?)

> 
> If locale should not be treated as a pref, can we separate out the relevant
> calls in OnPrefsLoaded to something like OnLocaleLoaded? This way it's clear
> that we're specifically waiting on locale to get loaded.

Agreed that this would be better nomenclature if we take the synchronous route.

[robliao, 2015-04-13 17:49:38]

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:282: // This hook interrupts initialization
flow.
On 2015/04/13 17:44:57, gab wrote:
> On 2015/04/13 17:32:30, robliao wrote:
> > It seems what we really want to do is wait for all values to be ready before
> > actually going down the OnPrefsLoaded path.
> > 
> > If locale should be treated as a pref, can we delay OnPrefsLoaded? All other
> > components waiting on this pref would benefit.
> 
> We have the concept of a PrefFilter which can be attached to a JsonPrefStore
and
> gets first bid before prefs are handed to Chrome (putting this out there,
though
> I'm not convinced yet that we want this to synchronously block all other
prefs).
> 
> @Rob: do you think this could be made asynchronous (i.e. it's synchronous
today
> because extensions are loaded synchronously, could this become one of the
async
> requirements to begin async load of extensions?)
> 
> > 
> > If locale should not be treated as a pref, can we separate out the relevant
> > calls in OnPrefsLoaded to something like OnLocaleLoaded? This way it's clear
> > that we're specifically waiting on locale to get loaded.
> 
> Agreed that this would be better nomenclature if we take the synchronous
route.

This could be make asynchronous, but I would rather have a full asynchronous
scheme set before forcing code to do a one-off to handle this.

One more data requirement isn't going hurt things too much on the async front.

[Alexander Alekseev, 2015-04-14 14:20:44]

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_impl.cc:497: const
base::Callback<void(Profile*, const base::Closure&)>& /* callback*/) {
On 2015/04/13 17:32:30, robliao wrote:
> Nit: Space after the k in callback.

Done.

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:282: // This hook interrupts initialization
flow.
On 2015/04/13 17:32:30, robliao wrote:
> It seems what we really want to do is wait for all values to be ready before
> actually going down the OnPrefsLoaded path.

Hmm. What do you mean?

UI language switch means closing current resource bundle and opening another
language-specific. Component extensions have their manifest stored in resource
bundle, so we need to switch to correct resource bundle before accessing
manifests.


> 
> If locale should be treated as a pref, can we delay OnPrefsLoaded? All other
> components waiting on this pref would benefit.
> 
> If locale should not be treated as a pref, can we separate out the relevant
> calls in OnPrefsLoaded to something like OnLocaleLoaded? This way it's clear
> that we're specifically waiting on locale to get loaded.

[robliao, 2015-04-14 17:33:20]

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:282: // This hook interrupts initialization
flow.
On 2015/04/14 14:20:44, Alexander Alekseev wrote:
> On 2015/04/13 17:32:30, robliao wrote:
> > It seems what we really want to do is wait for all values to be ready before
> > actually going down the OnPrefsLoaded path.
> 
> Hmm. What do you mean?
> 
> UI language switch means closing current resource bundle and opening another
> language-specific. Component extensions have their manifest stored in resource
> bundle, so we need to switch to correct resource bundle before accessing
> manifests.
> 
> 
> > 
> > If locale should be treated as a pref, can we delay OnPrefsLoaded? All other
> > components waiting on this pref would benefit.
> > 
> > If locale should not be treated as a pref, can we separate out the relevant
> > calls in OnPrefsLoaded to something like OnLocaleLoaded? This way it's clear
> > that we're specifically waiting on locale to get loaded.
> 

I guess the main question here is why does the current resource bundle load
before we load the real language-specific resource bundle?

[Alexander Alekseev, 2015-04-14 18:01:15]

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:282: // This hook interrupts initialization
flow.
On 2015/04/14 17:33:20, robliao wrote:
> On 2015/04/14 14:20:44, Alexander Alekseev wrote:
> > On 2015/04/13 17:32:30, robliao wrote:
> > > It seems what we really want to do is wait for all values to be ready
before
> > > actually going down the OnPrefsLoaded path.
> > 
> > Hmm. What do you mean?
> > 
> > UI language switch means closing current resource bundle and opening another
> > language-specific. Component extensions have their manifest stored in
resource
> > bundle, so we need to switch to correct resource bundle before accessing
> > manifests.
> > 
> > 
> > > 
> > > If locale should be treated as a pref, can we delay OnPrefsLoaded? All
other
> > > components waiting on this pref would benefit.
> > > 
> > > If locale should not be treated as a pref, can we separate out the
relevant
> > > calls in OnPrefsLoaded to something like OnLocaleLoaded? This way it's
clear
> > > that we're specifically waiting on locale to get loaded.
> > 
> 
> I guess the main question here is why does the current resource bundle load
> before we load the real language-specific resource bundle?

ChromeOS signin UI flow is like this:
1) We usually have several users on a single ChromeOS device. One of
them is considered "device owner".
2) Chrome is started in "login manager" mode. We need to show login UI with user
PODs, network/keyboard menus, "Add new user" UIm etc...
To display all this we are using device owners preferences (actually a copy of
them, because owner profile is encrypted and inaccessible).

3) When user logs in, we check whether current user preferences are the same as
current settings, or not. If not, we apply new settings, including UI language
preference.


So that we need to load some resource bundle to display login UI. But after user
logs in, we probably need to close current resource bundle and load another one.

[robliao, 2015-04-15 22:07:29]

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:282: // This hook interrupts initialization
flow.
On 2015/04/14 18:01:15, Alexander Alekseev wrote:
> On 2015/04/14 17:33:20, robliao wrote:
> > On 2015/04/14 14:20:44, Alexander Alekseev wrote:
> > > On 2015/04/13 17:32:30, robliao wrote:
> > > > It seems what we really want to do is wait for all values to be ready
> before
> > > > actually going down the OnPrefsLoaded path.
> > > 
> > > Hmm. What do you mean?
> > > 
> > > UI language switch means closing current resource bundle and opening
another
> > > language-specific. Component extensions have their manifest stored in
> resource
> > > bundle, so we need to switch to correct resource bundle before accessing
> > > manifests.
> > > 
> > > 
> > > > 
> > > > If locale should be treated as a pref, can we delay OnPrefsLoaded? All
> other
> > > > components waiting on this pref would benefit.
> > > > 
> > > > If locale should not be treated as a pref, can we separate out the
> relevant
> > > > calls in OnPrefsLoaded to something like OnLocaleLoaded? This way it's
> clear
> > > > that we're specifically waiting on locale to get loaded.
> > > 
> > 
> > I guess the main question here is why does the current resource bundle load
> > before we load the real language-specific resource bundle?
> 
> ChromeOS signin UI flow is like this:
> 1) We usually have several users on a single ChromeOS device. One of
> them is considered "device owner".
> 2) Chrome is started in "login manager" mode. We need to show login UI with
user
> PODs, network/keyboard menus, "Add new user" UIm etc...
> To display all this we are using device owners preferences (actually a copy of
> them, because owner profile is encrypted and inaccessible).
> 
> 3) When user logs in, we check whether current user preferences are the same
as
> current settings, or not. If not, we apply new settings, including UI language
> preference.
> 
> 
> So that we need to load some resource bundle to display login UI. But after
user
> logs in, we probably need to close current resource bundle and load another
one.

May I suggest this structure then?
The UserSessionManager registers for an OnPrefsLoaded from the prefs.
Once OnPrefsLoaded fires, it does the locale work and fires an OnLocaleReady.

The profile registers for an OnLocaleReady from the UserSessionManager.

The profile only calls the equivalent of OnPrefsLoadedImpl if both OnPrefsLoaded
and OnLocaleReady call back.

[Alexander Alekseev, 2015-04-29 16:32:07]

Done. Please review again.

On 2015/04/15 22:07:29, robliao wrote:
>
https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
> File chrome/browser/profiles/profile.h (right):
> 
>
https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
> chrome/browser/profiles/profile.h:282: // This hook interrupts initialization
> flow.
> On 2015/04/14 18:01:15, Alexander Alekseev wrote:
> > On 2015/04/14 17:33:20, robliao wrote:
> > > On 2015/04/14 14:20:44, Alexander Alekseev wrote:
> > > > On 2015/04/13 17:32:30, robliao wrote:
> > > > > It seems what we really want to do is wait for all values to be ready
> > before
> > > > > actually going down the OnPrefsLoaded path.
> > > > 
> > > > Hmm. What do you mean?
> > > > 
> > > > UI language switch means closing current resource bundle and opening
> another
> > > > language-specific. Component extensions have their manifest stored in
> > resource
> > > > bundle, so we need to switch to correct resource bundle before accessing
> > > > manifests.
> > > > 
> > > > 
> > > > > 
> > > > > If locale should be treated as a pref, can we delay OnPrefsLoaded? All
> > other
> > > > > components waiting on this pref would benefit.
> > > > > 
> > > > > If locale should not be treated as a pref, can we separate out the
> > relevant
> > > > > calls in OnPrefsLoaded to something like OnLocaleLoaded? This way it's
> > clear
> > > > > that we're specifically waiting on locale to get loaded.
> > > > 
> > > 
> > > I guess the main question here is why does the current resource bundle
load
> > > before we load the real language-specific resource bundle?
> > 
> > ChromeOS signin UI flow is like this:
> > 1) We usually have several users on a single ChromeOS device. One of
> > them is considered "device owner".
> > 2) Chrome is started in "login manager" mode. We need to show login UI with
> user
> > PODs, network/keyboard menus, "Add new user" UIm etc...
> > To display all this we are using device owners preferences (actually a copy
of
> > them, because owner profile is encrypted and inaccessible).
> > 
> > 3) When user logs in, we check whether current user preferences are the same
> as
> > current settings, or not. If not, we apply new settings, including UI
language
> > preference.
> > 
> > 
> > So that we need to load some resource bundle to display login UI. But after
> user
> > logs in, we probably need to close current resource bundle and load another
> one.
> 
> May I suggest this structure then?
> The UserSessionManager registers for an OnPrefsLoaded from the prefs.
> Once OnPrefsLoaded fires, it does the locale work and fires an OnLocaleReady.
> 
> The profile registers for an OnLocaleReady from the UserSessionManager.
> 
> The profile only calls the equivalent of OnPrefsLoadedImpl if both
OnPrefsLoaded
> and OnLocaleReady call back.

[robliao, 2015-04-30 06:50:05]

On 2015/04/29 16:32:07, Alexander Alekseev wrote:
> Done. Please review again.
> 
> On 2015/04/15 22:07:29, robliao wrote:
> >
>
https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
> > File chrome/browser/profiles/profile.h (right):
> > 
> >
>
https://codereview.chromium.org/1055863002/diff/40001/chrome/browser/profiles...
> > chrome/browser/profiles/profile.h:282: // This hook interrupts
initialization
> > flow.
> > On 2015/04/14 18:01:15, Alexander Alekseev wrote:
> > > On 2015/04/14 17:33:20, robliao wrote:
> > > > On 2015/04/14 14:20:44, Alexander Alekseev wrote:
> > > > > On 2015/04/13 17:32:30, robliao wrote:
> > > > > > It seems what we really want to do is wait for all values to be
ready
> > > before
> > > > > > actually going down the OnPrefsLoaded path.
> > > > > 
> > > > > Hmm. What do you mean?
> > > > > 
> > > > > UI language switch means closing current resource bundle and opening
> > another
> > > > > language-specific. Component extensions have their manifest stored in
> > > resource
> > > > > bundle, so we need to switch to correct resource bundle before
accessing
> > > > > manifests.
> > > > > 
> > > > > 
> > > > > > 
> > > > > > If locale should be treated as a pref, can we delay OnPrefsLoaded?
All
> > > other
> > > > > > components waiting on this pref would benefit.
> > > > > > 
> > > > > > If locale should not be treated as a pref, can we separate out the
> > > relevant
> > > > > > calls in OnPrefsLoaded to something like OnLocaleLoaded? This way
it's
> > > clear
> > > > > > that we're specifically waiting on locale to get loaded.
> > > > > 
> > > > 
> > > > I guess the main question here is why does the current resource bundle
> load
> > > > before we load the real language-specific resource bundle?
> > > 
> > > ChromeOS signin UI flow is like this:
> > > 1) We usually have several users on a single ChromeOS device. One of
> > > them is considered "device owner".
> > > 2) Chrome is started in "login manager" mode. We need to show login UI
with
> > user
> > > PODs, network/keyboard menus, "Add new user" UIm etc...
> > > To display all this we are using device owners preferences (actually a
copy
> of
> > > them, because owner profile is encrypted and inaccessible).
> > > 
> > > 3) When user logs in, we check whether current user preferences are the
same
> > as
> > > current settings, or not. If not, we apply new settings, including UI
> language
> > > preference.
> > > 
> > > 
> > > So that we need to load some resource bundle to display login UI. But
after
> > user
> > > logs in, we probably need to close current resource bundle and load
another
> > one.
> > 
> > May I suggest this structure then?
> > The UserSessionManager registers for an OnPrefsLoaded from the prefs.
> > Once OnPrefsLoaded fires, it does the locale work and fires an
OnLocaleReady.
> > 
> > The profile registers for an OnLocaleReady from the UserSessionManager.
> > 
> > The profile only calls the equivalent of OnPrefsLoadedImpl if both
> OnPrefsLoaded
> > and OnLocaleReady call back.

Much simpler! Thanks for the change. A few comments...

[robliao, 2015-04-30 06:50:13]

https://codereview.chromium.org/1055863002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/1055863002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:806:
SCOPED_UMA_HISTOGRAM_TIMER("Profile.OnPrefsLoadedTime");
Should this timer label be updated?

https://codereview.chromium.org/1055863002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:862: return;
This return needs an extra linebreak above it.

[Alexander Alekseev, 2015-04-30 19:35:25]

alemate@chromium.org changed reviewers:
+ mpearson@chromium.org

[Alexander Alekseev, 2015-04-30 19:35:26]

+mpearson@ for histograms.xml

Please review.

https://codereview.chromium.org/1055863002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/1055863002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:806:
SCOPED_UMA_HISTOGRAM_TIMER("Profile.OnPrefsLoadedTime");
On 2015/04/30 06:50:13, robliao wrote:
> Should this timer label be updated?
Done.

https://codereview.chromium.org/1055863002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:862: return;
On 2015/04/30 06:50:13, robliao wrote:
> This return needs an extra linebreak above it.

Done.

[Mark P, 2015-04-30 19:38:24]

One minor comment, otherwise histograms.xml lgtm

https://codereview.chromium.org/1055863002/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1055863002/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:30755: +    The amount of time that
elapsed during ProfileImpl::OnLocaleReady.
Please explicitly comment roughly how often this is called.  Once per profile
load?  Once per accept language for a profile? etc.

[Alexander Alekseev, 2015-04-30 19:45:01]

https://codereview.chromium.org/1055863002/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1055863002/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:30755: +    The amount of time that
elapsed during ProfileImpl::OnLocaleReady.
On 2015/04/30 19:38:24, Mark P wrote:
> Please explicitly comment roughly how often this is called.  Once per profile
> load?  Once per accept language for a profile? etc.

Done.

[robliao, 2015-05-01 06:39:55]

lgtm

[Alexander Alekseev, 2015-05-05 11:40:09]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2015-05-05 11:40:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
nkostylev@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1055863002/#ps180001
(title: "Update after review.")

[commit-bot: I haz the power, 2015-05-05 11:40:32]

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

[commit-bot: I haz the power, 2015-05-05 11:46:53]

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

[commit-bot: I haz the power, 2015-05-05 11:46:54]

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

[Alexander Alekseev, 2015-05-05 20:18:24]

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

[Alexander Alekseev, 2015-05-05 20:18:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
robliao@chromium.org, nkostylev@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1055863002/#ps200001
(title: "Fixed CREATE_MODE_SYNCHRONOUS mode.")

[commit-bot: I haz the power, 2015-05-05 20:20:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1055863002/200001

[Alexander Alekseev, 2015-05-05 20:22:55]

alemate@chromium.org changed reviewers:
+ oshima@chromium.org, shuchen@chromium.org

[Alexander Alekseev, 2015-05-05 20:22:56]

I've implemented synchronous profile create mode.
I had to remove one more "GetActiveUserProfile()" call,
which was a source of crashes (because it was called while profile was not
created yet).

robliao@, nkostylev@ :  PTAL

oshima@, please review chrome/browser/ui/ash/system_tray_delegate_chromeos.*

shuchen@: please review  chrome/browser/chromeos/input_method/*

[Alexander Alekseev, 2015-05-05 20:26:15]

alemate@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[Alexander Alekseev, 2015-05-05 20:26:16]

On 2015/05/05 20:22:56, Alexander Alekseev wrote:
> I've implemented synchronous profile create mode.
> I had to remove one more "GetActiveUserProfile()" call,
> which was a source of crashes (because it was called while profile was not
> created yet).
> 
> robliao@, nkostylev@ :  PTAL
> 
> oshima@, please review chrome/browser/ui/ash/system_tray_delegate_chromeos.*
> 
> shuchen@: please review  chrome/browser/chromeos/input_method/*

shuchen@: please also review
chrome/browser/chromeos/extensions/input_method_event_router.*

dmazzoni@: please review
chrome/browser/chromeos/accessibility/accessibility_manager.*

[noms (inactive), 2015-05-05 20:53:15]

https://codereview.chromium.org/1055863002/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/1055863002/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:858: TRACE_EVENT0("browser",
"ProfileImpl::OnPrefsLoaded");
nit:  Did you delete `SCOPED_UMA_HISTOGRAM_TIMER("Profile.OnPrefsLoadedTime")`
on purpose?

https://codereview.chromium.org/1055863002/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:862: 
nit: remove empty line

[Alexander Alekseev, 2015-05-05 21:00:23]

https://codereview.chromium.org/1055863002/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/1055863002/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:858: TRACE_EVENT0("browser",
"ProfileImpl::OnPrefsLoaded");
On 2015/05/05 20:53:15, noms (slow. ping if too slow) wrote:
> nit:  Did you delete `SCOPED_UMA_HISTOGRAM_TIMER("Profile.OnPrefsLoadedTime")`
> on purpose?

It is now replaced by SCOPED_UMA_HISTOGRAM_TIMER("Profile.OnLocaleReadyTime"); .

If you think it's reasonable, I can add it as time "From OnPrefsLoaded() started
till OnLocaleReady() finished"  .

[rkaplow, 2015-05-05 21:07:06]

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

[rkaplow, 2015-05-05 21:07:07]

https://codereview.chromium.org/1055863002/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/1055863002/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:858: TRACE_EVENT0("browser",
"ProfileImpl::OnPrefsLoaded");
On 2015/05/05 21:00:23, Alexander Alekseev wrote:
> On 2015/05/05 20:53:15, noms (slow. ping if too slow) wrote:
> > nit:  Did you delete
`SCOPED_UMA_HISTOGRAM_TIMER("Profile.OnPrefsLoadedTime")`
> > on purpose?
> 
> It is now replaced by SCOPED_UMA_HISTOGRAM_TIMER("Profile.OnLocaleReadyTime");
.
> 
> If you think it's reasonable, I can add it as time "From OnPrefsLoaded()
started
> till OnLocaleReady() finished"  .

if most of the time is in that (i.e. not on the OnProfileCreated) then that
seems ok.

You should add an associated TRACE_EVENT0 for onLocaleReady.

[commit-bot: I haz the power, 2015-05-05 22:05:01]

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

[commit-bot: I haz the power, 2015-05-05 22:05:02]

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

[Shu Chen, 2015-05-06 02:25:38]

https://codereview.chromium.org/1055863002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/1055863002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:425:
manager_->ChangeInputMethodInternal(*descriptor, profile, show_message,
Where is |profile| defined?

[Shu Chen, 2015-05-06 06:14:14]

lgtm for chrome/browser/chromeos/input_method/... and ui/base/ime/...

https://codereview.chromium.org/1055863002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/1055863002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:425:
manager_->ChangeInputMethodInternal(*descriptor, profile, show_message,
On 2015/05/06 02:25:38, Shu Chen wrote:
> Where is |profile| defined?

Please ignore this. I found it is this->profile.

[Alexander Alekseev, 2015-05-06 18:10:58]

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

[Alexander Alekseev, 2015-05-06 18:10:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
nkostylev@chromium.org, mpearson@chromium.org, robliao@chromium.org,
shuchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1055863002/#ps220001
(title: "Fix initial locale load, when no user is active yet.")

[commit-bot: I haz the power, 2015-05-06 18:13:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1055863002/220001

[robliao, 2015-05-06 18:20:32]

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/base/locale_util.cc (right):

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/base/locale_util.cc:67: DCHECK(ime_state);
This DCHECK isn't necessary. We will crash in the next few lines if it's not
true, providing the same info.

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/input_method_event_router.cc (right):

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/input_method_event_router.cc:33:
CHECK(static_cast<content::BrowserContext*>(profile) == context_);
This will trigger a chrome crash if it fails. How critical is it that the
context matches?

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/user_session_manager.cc:1651: bool
synchronous) {
Can this simply be the create mode instead? That way future folks don't have to
guess what true and false means when looking from the caller side.

[Alexander Alekseev, 2015-05-06 18:48:51]

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

[Alexander Alekseev, 2015-05-06 18:48:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
nkostylev@chromium.org, mpearson@chromium.org, robliao@chromium.org,
shuchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1055863002/#ps240001
(title: "Fix build.")

[commit-bot: I haz the power, 2015-05-06 18:51:32]

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

[Alexander Alekseev, 2015-05-06 19:11:31]

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

[Alexander Alekseev, 2015-05-06 19:11:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
nkostylev@chromium.org, mpearson@chromium.org, robliao@chromium.org,
shuchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1055863002/#ps260001
(title: "Fix build.")

[commit-bot: I haz the power, 2015-05-06 19:11:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1055863002/260001

[commit-bot: I haz the power, 2015-05-06 20:49:45]

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

[commit-bot: I haz the power, 2015-05-06 20:49:46]

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

[Alexander Alekseev, 2015-05-07 13:50:39]

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

[Alexander Alekseev, 2015-05-07 13:50:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
nkostylev@chromium.org, mpearson@chromium.org, robliao@chromium.org,
shuchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1055863002/#ps280001
(title: "Disable RespectLocalePreference() for synchronous profile creation.")

[commit-bot: I haz the power, 2015-05-07 13:50:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1055863002/280001

[Alexander Alekseev, 2015-05-07 15:21:57]

https://codereview.chromium.org/1055863002/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/1055863002/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:858: TRACE_EVENT0("browser",
"ProfileImpl::OnPrefsLoaded");
On 2015/05/05 21:07:07, rkaplow wrote:
> On 2015/05/05 21:00:23, Alexander Alekseev wrote:
> > On 2015/05/05 20:53:15, noms (slow. ping if too slow) wrote:
> > > nit:  Did you delete
> `SCOPED_UMA_HISTOGRAM_TIMER("Profile.OnPrefsLoadedTime")`
> > > on purpose?
> > 
> > It is now replaced by
SCOPED_UMA_HISTOGRAM_TIMER("Profile.OnLocaleReadyTime");
> .
> > 
> > If you think it's reasonable, I can add it as time "From OnPrefsLoaded()
> started
> > till OnLocaleReady() finished"  .
> 
> if most of the time is in that (i.e. not on the OnProfileCreated) then that
> seems ok.
> 
> You should add an associated TRACE_EVENT0 for onLocaleReady.

Done.

https://codereview.chromium.org/1055863002/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:862: 
On 2015/05/05 20:53:15, noms (slow. ping if too slow) wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/base/locale_util.cc (right):

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/base/locale_util.cc:67: DCHECK(ime_state);
On 2015/05/06 18:20:32, robliao wrote:
> This DCHECK isn't necessary. We will crash in the next few lines if it's not
> true, providing the same info.

Done.

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/input_method_event_router.cc (right):

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/input_method_event_router.cc:33:
CHECK(static_cast<content::BrowserContext*>(profile) == context_);
On 2015/05/06 18:20:32, robliao wrote:
> This will trigger a chrome crash if it fails. How critical is it that the
> context matches?

Here is crbug.com/483258 - what happens (crash) when message is delivered to a
wrong profile.

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/user_session_manager.cc:1651: bool
synchronous) {
On 2015/05/06 18:20:32, robliao wrote:
> Can this simply be the create mode instead? That way future folks don't have
to
> guess what true and false means when looking from the caller side.

I've reverted this change.

[Nikita (slow), 2015-05-08 10:54:26]

still lgtm however CL description needs to be updated to reflect changes that
you've did like passing Profile.

[Alexander Alekseev, 2015-05-08 12:55:16]

On 2015/05/08 10:54:26, Nikita wrote:
> still lgtm however CL description needs to be updated to reflect changes that
> you've did like passing Profile.

Done.

[robliao, 2015-05-08 17:30:21]

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/input_method_event_router.cc (right):

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/input_method_event_router.cc:33:
CHECK(static_cast<content::BrowserContext*>(profile) == context_);
On 2015/05/07 15:21:57, Alexander Alekseev wrote:
> On 2015/05/06 18:20:32, robliao wrote:
> > This will trigger a chrome crash if it fails. How critical is it that the
> > context matches?
> 
> Here is crbug.com/483258 - what happens (crash) when message is delivered to a
> wrong profile.

Add a comment about the use of the CHECK here then as this is serious business.

[Alexander Alekseev, 2015-05-12 17:30:45]

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/input_method_event_router.cc (right):

https://codereview.chromium.org/1055863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/input_method_event_router.cc:33:
CHECK(static_cast<content::BrowserContext*>(profile) == context_);
On 2015/05/08 17:30:20, robliao wrote:
> On 2015/05/07 15:21:57, Alexander Alekseev wrote:
> > On 2015/05/06 18:20:32, robliao wrote:
> > > This will trigger a chrome crash if it fails. How critical is it that the
> > > context matches?
> > 
> > Here is crbug.com/483258 - what happens (crash) when message is delivered to
a
> > wrong profile.
> 
> Add a comment about the use of the CHECK here then as this is serious
business.

I've replaced CHECK with DCHECK and comment.
I'll submit another CL with CHECK, so that revert would be easier if needed.

[oshima, 2015-05-12 20:11:15]

chrome/browser/ui/ash lgtm (you may tbr for this kind of trivial change)

[robliao, 2015-05-12 20:16:28]

lgtm

https://codereview.chromium.org/1055863002/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/base/locale_util.cc (right):

https://codereview.chromium.org/1055863002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/base/locale_util.cc:67: 
Nit: Remove extra line.

[Alexander Alekseev, 2015-05-12 20:23:57]

alemate@chromium.org changed reviewers:
- dmazzoni@chromium.org

[Alexander Alekseev, 2015-05-12 20:24:21]

https://codereview.chromium.org/1055863002/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/base/locale_util.cc (right):

https://codereview.chromium.org/1055863002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/base/locale_util.cc:67: 
On 2015/05/12 20:16:28, robliao wrote:
> Nit: Remove extra line.

Done.

[Alexander Alekseev, 2015-05-12 20:24:41]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2015-05-12 20:24:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org, shuchen@chromium.org, nkostylev@chromium.org,
oshima@chromium.org, robliao@chromium.org
Link to the patchset: https://codereview.chromium.org/1055863002/#ps340001
(title: "Remove empty line.")

[commit-bot: I haz the power, 2015-05-12 20:25:56]

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

[robliao, 2015-05-12 20:28:35]

On 2015/05/12 20:25:56, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1055863002/340001

FYI, I think you still need approval on c/b/profiles/*

Check out https://www.chromium.org/developers/useful-extensions and install the
Chromite Butler to make rietveld a bit easier to use.

[Alexander Alekseev, 2015-05-12 20:36:17]

The CQ bit was unchecked by alemate@chromium.org

[Alexander Alekseev, 2015-05-12 20:38:01]

On 2015/05/12 20:28:35, robliao wrote:
> On 2015/05/12 20:25:56, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1055863002/340001
> 
> FYI, I think you still need approval on c/b/profiles/*


Yes, you're right.

[noms (inactive), 2015-05-12 21:20:01]

On 2015/05/12 20:38:01, Alexander Alekseev wrote:
> On 2015/05/12 20:28:35, robliao wrote:
> > On 2015/05/12 20:25:56, I haz the power (commit-bot) wrote:
> > > CQ is trying da patch. Follow status at
> > >  https://chromium-cq-status.appspot.com/patch-status/1055863002/340001
> > 
> > FYI, I think you still need approval on c/b/profiles/*
> 
> 
> Yes, you're right.

Just waiting on Gab's blessing, since he knows the most about prefs (even though
it's in profiles)

[noms (inactive), 2015-05-12 21:20:58]

On 2015/05/12 21:20:01, noms (slow. ping if too slow) wrote:
> On 2015/05/12 20:38:01, Alexander Alekseev wrote:
> > On 2015/05/12 20:28:35, robliao wrote:
> > > On 2015/05/12 20:25:56, I haz the power (commit-bot) wrote:
> > > > CQ is trying da patch. Follow status at
> > > >  https://chromium-cq-status.appspot.com/patch-status/1055863002/340001
> > > 
> > > FYI, I think you still need approval on c/b/profiles/*
> > 
> > 
> > Yes, you're right.
> 
> Just waiting on Gab's blessing, since he knows the most about prefs (even
though
> it's in profiles)

Ah, I see that gab -> robliao. LGTM!

[noms (inactive), 2015-05-12 21:21:01]

On 2015/05/12 21:20:01, noms (slow. ping if too slow) wrote:
> On 2015/05/12 20:38:01, Alexander Alekseev wrote:
> > On 2015/05/12 20:28:35, robliao wrote:
> > > On 2015/05/12 20:25:56, I haz the power (commit-bot) wrote:
> > > > CQ is trying da patch. Follow status at
> > > >  https://chromium-cq-status.appspot.com/patch-status/1055863002/340001
> > > 
> > > FYI, I think you still need approval on c/b/profiles/*
> > 
> > 
> > Yes, you're right.
> 
> Just waiting on Gab's blessing, since he knows the most about prefs (even
though
> it's in profiles)

Ah, I see that gab -> robliao. LGTM!

[Alexander Alekseev, 2015-05-12 21:23:14]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2015-05-12 21:23:43]

On 2015/05/12 21:21:01, noms (slow. ping if too slow) wrote:
> On 2015/05/12 21:20:01, noms (slow. ping if too slow) wrote:
> > On 2015/05/12 20:38:01, Alexander Alekseev wrote:
> > > On 2015/05/12 20:28:35, robliao wrote:
> > > > On 2015/05/12 20:25:56, I haz the power (commit-bot) wrote:
> > > > > CQ is trying da patch. Follow status at
> > > > >  https://chromium-cq-status.appspot.com/patch-status/1055863002/340001
> > > > 
> > > > FYI, I think you still need approval on c/b/profiles/*
> > > 
> > > 
> > > Yes, you're right.
> > 
> > Just waiting on Gab's blessing, since he knows the most about prefs (even
> though
> > it's in profiles)
> 
> Ah, I see that gab -> robliao. LGTM!

Thanks!

[commit-bot: I haz the power, 2015-05-12 21:23:52]

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

[commit-bot: I haz the power, 2015-05-12 21:36:35]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-05-12 21:37:25]

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