Install Chrome OS apps to shared location

Install Chrome OS apps to shared location

This CL implements install and uninstall to shared location. New functionality
is disabled by default and can be enabled only by command line switch. The switch
will not be enabled until extensions resources verification is not enabled by
default in force mode for all apps/extensions.

Follow up CL should add support for GC in shared location.

BUG=235263
TEST=browser_tests + manual
R=asargent@chromium.org
TBR=mnissler@chromium.org

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

[Dmitry Polukhin, 2014-05-15 19:19:51]

Antony, please take a look. High level comments about approach for app resources
sharing between multiple users are very welcome.

[asargent_no_longer_on_chrome, 2014-05-16 17:56:19]

I like the overall approach. 

I think it might be a little better to have ExtensionAssetsManager be an
abstract interface, and then two separate implementations of
ExtensionAssetsManagerDefault and ExtensionAssetsManagerDefaultChromeos, rather
than to have ExtensionAssetsManagerDefaultChromeos inherit from a concrete
implementation.

https://codereview.chromium.org/273193006/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/crx_installer.h (right):

https://codereview.chromium.org/273193006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.h:234: void FinishCompleteInstall(const
base::FilePath& version_dir);
nit: with a "Finish" post-condition to run, "CompleteInstall" becomes a sort of
confusing name. I don't have any great suggestions, but maybe something along
the lines of what it actually does?

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

https://codereview.chromium.org/273193006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:135: // also
possible. User specific apps should be excluded to don't expose apps
grammar nit: "should be excluded to don't" -> "should be excluded to not"

https://codereview.chromium.org/273193006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:215:
shared_install_dir);
Will concurrent user sessions (eg a chromebox allowing multiple remote desktop
sessions) ever be a thing we support in ChromeOS? If so it seems like we'd need
to think a little about some sort of locking mechanism here so that there isn't
a race condition here to write the files to the directory.

[Dmitry Polukhin, 2014-05-16 22:45:50]

Will make ExtensionAssetsManager abstract class.

Please take a look to my replay about thread safety below because it will change
things significantly if we have to make ExtensionAssetsManagerChromeOS shared
between multiple task runner sequences.

https://codereview.chromium.org/273193006/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/crx_installer.h (right):

https://codereview.chromium.org/273193006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.h:234: void FinishCompleteInstall(const
base::FilePath& version_dir);
On 2014/05/16 17:56:19, Antony Sargent wrote:
> nit: with a "Finish" post-condition to run, "CompleteInstall" becomes a sort
of
> confusing name. I don't have any great suggestions, but maybe something along
> the lines of what it actually does?
> 
> 

Done.

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

https://codereview.chromium.org/273193006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:135: // also
possible. User specific apps should be excluded to don't expose apps
On 2014/05/16 17:56:19, Antony Sargent wrote:
> grammar nit: "should be excluded to don't" -> "should be excluded to not"

Done.

https://codereview.chromium.org/273193006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:215:
shared_install_dir);
On 2014/05/16 17:56:19, Antony Sargent wrote:
> Will concurrent user sessions (eg a chromebox allowing multiple remote desktop
> sessions) ever be a thing we support in ChromeOS? If so it seems like we'd
need
> to think a little about some sort of locking mechanism here so that there
isn't
> a race condition here to write the files to the directory. 

Yes, in M37 we enabled multi-profile by default on Chrome OS. We do have race
condition here because ExtensionService::GetFileTaskRunner uses different
sequence tokens for different profiles. Moreover WeakPtrs used in implementation
are also not safe :( I didn't notice it when I looked to
ExtensionService::GetFileTaskRunner first time. So we have two options: (1)
change GetFileTaskRunner to use the same sequence token or make
ExtensionAssetsManagerChromeOS read-only. I think we can use first approach
because there is no sense in parallel file IO to the same disk, it will only
reduce performance. What do you think?

[asargent_no_longer_on_chrome, 2014-05-19 18:25:19]

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

https://codereview.chromium.org/273193006/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:215:
shared_install_dir);
On 2014/05/16 22:45:50, Dmitry Polukhin wrote:
> On 2014/05/16 17:56:19, Antony Sargent wrote:
> > Will concurrent user sessions (eg a chromebox allowing multiple remote
desktop
> > sessions) ever be a thing we support in ChromeOS? If so it seems like we'd
> need
> > to think a little about some sort of locking mechanism here so that there
> isn't
> > a race condition here to write the files to the directory. 
> 
> Yes, in M37 we enabled multi-profile by default on Chrome OS. We do have race
> condition here because ExtensionService::GetFileTaskRunner uses different
> sequence tokens for different profiles. Moreover WeakPtrs used in
implementation
> are also not safe :( I didn't notice it when I looked to
> ExtensionService::GetFileTaskRunner first time. So we have two options: (1)
> change GetFileTaskRunner to use the same sequence token or make
> ExtensionAssetsManagerChromeOS read-only. I think we can use first approach
> because there is no sense in parallel file IO to the same disk, it will only
> reduce performance. What do you think?

I spent a little time thinking about this and happened to catch cpu@ in the
hallway and asked him about the implications of this for windows. I'm slightly
worried that we'd lose out on some potential gains of doing concurrent writes,
since it sounds like at least on windows the system is in some cases able to
make faster progress with multiple threads doing "independent" I/O, where
independent roughly means isn't trying to write the same blocks. 

What about having some sort of a queue with a lock (or perhaps N locks scoped by
extension id), where the first incoming job grabs the lock, check for the
existence of the directory, and if it isn't present, proceeds to do the writes.
Any jobs that arrive during this time will just register a callback, and when
the first arriving job finishes it can then dispatch any registered callbacks
with the result that the directory was found or created.

[Dmitry Polukhin, 2014-05-19 21:30:22]

PTAL

I added helper class that works on UI thread and resolve parallel install case
for the same ID+version.

[asargent_no_longer_on_chrome, 2014-05-20 21:20:25]

I like the direction - I found a few nits, made a couple of optional
suggestions, and may have found one synchronization problem.

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/extension_assets_manager.cc (right):

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager.cc:21: // |local_install_dir|
or some common location shared for multiple users.
looks like this comment is out of date?

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/extension_assets_manager.h (right):

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager.h:20: // shared place for
multiple profiles (users). This class manages install and
nit: "in shared place" -> "in a shared place"

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager.h:22: // Chrome OS only. This
class must be used only from extension file task runner
nit: "only from extension file task runner" -> "only from the extension file
task runner"

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager.h:26: // Callback that is
invoked when the extension assets installed.
nit: "when the extension assets installed" -> "when the extension assets are
installed"

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager.h:48:
ExtensionAssetsManager() {}
Do you need this constructor?

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/extension_assets_manager_chromeos.cc (right):

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:245: const
std::string& user_name = profile->GetProfileName();
I assume that GetProfileName() is guaranteed to be unique? (this isn't clear
from the comment on its declaration in profile.h)

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:286:
callback.Run(shared_version_dir);
Since you don't do anything here but dispatch the callback, would it work to
just get rid of this method completely and then in the places you were calling
it, instead of posting a task to
ExtensionAssetsManagerChromeOS::RunInstallCallback, you just posted |callback|
as the task instead?

Eg instead of:

ExtensionAssetsManagerChromeOS::GetFileTaskRunner(profile)->PostTask(
    FROM_HERE,
    base::Bind(&ExtensionAssetsManagerChromeOS::RunInstallCallback,
               base::FilePath(shared_path),
               callback));

you did:

ExtensionAssetsManagerChromeOS::GetFileTaskRunner(profile)->PostTask(
    FROM_HERE,
    base::Bind(callback, base::FilePath(shared_path));

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:335: if
(!extension_info &&
It looks like extension_info gets initialized to NULL above and can't get set to
anything else before here, right? Would it make sense to just move the
declaration of it down here inside the else block? (another though: perhaps you
would want to just skip having the else block, and add a return statement after
the PostTask call just above here)

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:340: if
(!version_info &&
similar comment about version_info

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:430:
base::Bind(&ExtensionAssetsManagerChromeOS::DeleteSharedExtension, id));
Imagine this scenario:

-We have a version of extension A in the cache
-User 1 uninstalls extension A, so we post the tasks here on the file task
runner for User 1's profile to delete the version and extension directories. 
-User 2 is just starting install of extension A, and posts a task to the file
task runner for User 2's profile to begin writing the extension directory. 
-User 1's task to delete the directory and User 2's job to write to it run in
parallel, potentially resulting in files being deleted that should not be. 


Can this sequence of events happen, since the file task runners for User 1 and
User 2 use different sequence tokens? To prevent it, perhaps you also need to
keep bookkeeping for the delete happening on the file task runner, with a
callback for it to let you know when it is finished and its safe for other users
to start writing again.


Also, in the CL description, you mentioned needing to add GC support. I assume
you were referring to cleaning up the cache after profiles are deleted from the
machine, which might not run through this code?

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/extension_assets_manager_chromeos.h (right):

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.h:19: // Chrome OS
specific implementation assets manager that share default apps
nit: "that share" -> "that shares"

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.h:58: // Return
|true| if |extension| can be installed in shared place for all users
nit: "can be installed in shared place" -> "can be installed in a shared place"

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.h:62: // Called in
UI thread to check if given version of the |extension| already
a couple grammar nits; this should be:

"Called on the UI thread to check if a given version of the |extension| already
exists at the shared location."

[Dmitry Polukhin, 2014-05-20 23:42:16]

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/extension_assets_manager.cc (right):

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager.cc:21: // |local_install_dir|
or some common location shared for multiple users.
On 2014/05/20 21:20:25, Antony Sargent wrote:
> looks like this comment is out of date?

Done.

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/extension_assets_manager.h (right):

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager.h:20: // shared place for
multiple profiles (users). This class manages install and
On 2014/05/20 21:20:25, Antony Sargent wrote:
> nit: "in shared place" -> "in a shared place"

Done.

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager.h:22: // Chrome OS only. This
class must be used only from extension file task runner
On 2014/05/20 21:20:25, Antony Sargent wrote:
> nit: "only from extension file task runner" -> "only from the extension file
> task runner"

Done.

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager.h:26: // Callback that is
invoked when the extension assets installed.
On 2014/05/20 21:20:25, Antony Sargent wrote:
> nit: "when the extension assets installed" -> "when the extension assets are
> installed"

Done.

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager.h:48:
ExtensionAssetsManager() {}
On 2014/05/20 21:20:25, Antony Sargent wrote:
> Do you need this constructor? 

Done.

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/extension_assets_manager_chromeos.cc (right):

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:245: const
std::string& user_name = profile->GetProfileName();
On 2014/05/20 21:20:25, Antony Sargent wrote:
> I assume that GetProfileName() is guaranteed to be unique? (this isn't clear
> from the comment on its declaration in profile.h)
> 

On Chrome OS it is user email and Chrome OS distinguishes users by email so same
profile name means same user.

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:286:
callback.Run(shared_version_dir);
On 2014/05/20 21:20:25, Antony Sargent wrote:
> Since you don't do anything here but dispatch the callback, would it work to
> just get rid of this method completely and then in the places you were calling
> it, instead of posting a task to
> ExtensionAssetsManagerChromeOS::RunInstallCallback, you just posted |callback|
> as the task instead?
> 
> Eg instead of:
> 
> ExtensionAssetsManagerChromeOS::GetFileTaskRunner(profile)->PostTask(
>     FROM_HERE,
>     base::Bind(&ExtensionAssetsManagerChromeOS::RunInstallCallback,
>                base::FilePath(shared_path),
>                callback));
> 
> you did:
> 
> ExtensionAssetsManagerChromeOS::GetFileTaskRunner(profile)->PostTask(
>     FROM_HERE,
>     base::Bind(callback, base::FilePath(shared_path));
> 
> 

Yes, I used to have some extra thing but now it can be removed. Thanks for
finding it!

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:335: if
(!extension_info &&
On 2014/05/20 21:20:25, Antony Sargent wrote:
> It looks like extension_info gets initialized to NULL above and can't get set
to
> anything else before here, right? Would it make sense to just move the
> declaration of it down here inside the else block? (another though: perhaps
you
> would want to just skip having the else block, and add a return statement
after
> the PostTask call just above here)
> 
> 

If I move declaration to for-loop it will be re-initialized on every iteration.
So it was optimization if we have more than one pending install for the same
id+version. I rewrote the code to interchange for-loop and if-statement (now I
have for-loop in both then and else parts).

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:340: if
(!version_info &&
On 2014/05/20 21:20:25, Antony Sargent wrote:
> similar comment about version_info 

Done.

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:430:
base::Bind(&ExtensionAssetsManagerChromeOS::DeleteSharedExtension, id));
On 2014/05/20 21:20:25, Antony Sargent wrote:
> Imagine this scenario:
> 
> -We have a version of extension A in the cache
> -User 1 uninstalls extension A, so we post the tasks here on the file task
> runner for User 1's profile to delete the version and extension directories. 
> -User 2 is just starting install of extension A, and posts a task to the file
> task runner for User 2's profile to begin writing the extension directory. 
> -User 1's task to delete the directory and User 2's job to write to it run in
> parallel, potentially resulting in files being deleted that should not be. 
> 
> 
> Can this sequence of events happen, since the file task runners for User 1 and
> User 2 use different sequence tokens? To prevent it, perhaps you also need to
> keep bookkeeping for the delete happening on the file task runner, with a
> callback for it to let you know when it is finished and its safe for other
users
> to start writing again.
> 
> 
> Also, in the CL description, you mentioned needing to add GC support. I assume
> you were referring to cleaning up the cache after profiles are deleted from
the
> machine, which might not run through this code?
> 

Yep, we do have race here even between removing given version and at the same
time installing it for another profile. Will fix it.

As for GC, Chrome has ExtensionGarbageCollector that works for profile specific
Extensions dir. We need the same for shared Extensions dir + we need clean up
for the case when user is removed from the device. I'm going to inherit from
ExtensionGarbageCollector and make Chrome OS specific implementation with
additional logic.

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/extension_assets_manager_chromeos.h (right):

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.h:19: // Chrome OS
specific implementation assets manager that share default apps
On 2014/05/20 21:20:25, Antony Sargent wrote:
> nit: "that share" -> "that shares"

Done.

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.h:58: // Return
|true| if |extension| can be installed in shared place for all users
On 2014/05/20 21:20:25, Antony Sargent wrote:
> nit: "can be installed in shared place" -> "can be installed in a shared
place"

Done.

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.h:62: // Called in
UI thread to check if given version of the |extension| already
On 2014/05/20 21:20:25, Antony Sargent wrote:
> a couple grammar nits; this should be:
> 
> "Called on the UI thread to check if a given version of the |extension|
already
> exists at the shared location."

Done.

[Dmitry Polukhin, 2014-05-21 00:37:45]

PTAL

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/extension_assets_manager_chromeos.cc (right):

https://codereview.chromium.org/273193006/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:430:
base::Bind(&ExtensionAssetsManagerChromeOS::DeleteSharedExtension, id));
On 2014/05/20 23:42:16, Dmitry Polukhin wrote:
> On 2014/05/20 21:20:25, Antony Sargent wrote:
> > Imagine this scenario:
> > 
> > -We have a version of extension A in the cache
> > -User 1 uninstalls extension A, so we post the tasks here on the file task
> > runner for User 1's profile to delete the version and extension directories.

> > -User 2 is just starting install of extension A, and posts a task to the
file
> > task runner for User 2's profile to begin writing the extension directory. 
> > -User 1's task to delete the directory and User 2's job to write to it run
in
> > parallel, potentially resulting in files being deleted that should not be. 
> > 
> > 
> > Can this sequence of events happen, since the file task runners for User 1
and
> > User 2 use different sequence tokens? To prevent it, perhaps you also need
to
> > keep bookkeeping for the delete happening on the file task runner, with a
> > callback for it to let you know when it is finished and its safe for other
> users
> > to start writing again.
> > 
> > 
> > Also, in the CL description, you mentioned needing to add GC support. I
assume
> > you were referring to cleaning up the cache after profiles are deleted from
> the
> > machine, which might not run through this code?
> > 
> 
> Yep, we do have race here even between removing given version and at the same
> time installing it for another profile. Will fix it.
> 
> As for GC, Chrome has ExtensionGarbageCollector that works for profile
specific
> Extensions dir. We need the same for shared Extensions dir + we need clean up
> for the case when user is removed from the device. I'm going to inherit from
> ExtensionGarbageCollector and make Chrome OS specific implementation with
> additional logic.

I thought more about it and it seems that this race condition may result in bad
result only if user 1 was the only user of the extension and extension dir is
being removed. There is no problem with removing version dir under extension dir
because installation code will just use another candidate version dir (see
extensions::file_util::InstallExtension). Code inside InstallExtension checks
that version dir doesn't exist before moving extensions into it. Empty extension
dir should be small issue and it should be cleaned up in GC. Therefore I just
removed this if-statement and wrote comment.

[Dmitry Polukhin, 2014-05-22 17:33:53]

friendly ping

[asargent_no_longer_on_chrome, 2014-05-28 21:36:38]

On 2014/05/22 17:33:53, Dmitry Polukhin wrote:
> friendly ping

Sorry for silence, I was on vacation for several days and am back in the office
today. I'll take a look this afternoon.

[asargent_no_longer_on_chrome, 2014-05-28 21:44:49]

lgtm

https://codereview.chromium.org/273193006/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/extension_assets_manager_chromeos.cc (right):

https://codereview.chromium.org/273193006/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:411: // used by
single user and installing the same extension for other user.
A few grammar nits with this sentence. 

did you mean something like: "It will be removed by GC when it is safe to do so,
and this avoids a race condition between concurrent uninstall by one user and
install by another". ?

[Dmitry Polukhin, 2014-05-28 21:59:18]

+ gab@ for OWNER review chrome/browser/prefs/browser_prefs.cc

Antony, thank you for review!

https://codereview.chromium.org/273193006/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/extension_assets_manager_chromeos.cc (right):

https://codereview.chromium.org/273193006/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/extension_assets_manager_chromeos.cc:411: // used by
single user and installing the same extension for other user.
On 2014/05/28 21:44:49, Antony Sargent wrote:
> A few grammar nits with this sentence. 
> 
> did you mean something like: "It will be removed by GC when it is safe to do
so,
> and this avoids a race condition between concurrent uninstall by one user and
> install by another". ?
> 
> 
> 

Done.

[Dmitry Polukhin, 2014-05-28 23:49:29]

+ Mattias for OWNER review chrome/browser/prefs/browser_prefs.cc (gab@ seems to
be on leave)

[Dmitry Polukhin, 2014-05-29 21:31:55]

Changes in chrome/browser/prefs/browser_prefs.cc are trivial, committing as TBR.

[Dmitry Polukhin, 2014-05-29 22:19:14]

Committed patchset #10 manually as r273620 (presubmit successful).

[Mattias Nissler (ping if slow), 2014-05-30 07:39:22]

browser_prefs.cc LGTM.