Implement support in DefaultComponentInstaller for picking up bundled

DefaultComponentInstaller support for bundled components.

DefaultComponentInstaller will now pick up component implementations from the plugins directory. It expects the component to have a folder in the plugins directory with the same name as we would store in USER_DATA_DIR. Unlike USER_DATA_DIR, this folder should not contain a set of versioned folders, but the unpacked contents of the CRX itself.

As a consequence of this, the installation directory of a component cannot be determined purely by PathService, so I have gone ahead and removed the relevant PathService entries.

BUG=608128
Committed: https://crrev.com/cb4f42ca4ef394517e4f94a231ae531f331d4ca8
Cr-Commit-Position: refs/heads/master@{#393961}

[waffles, 2016-04-30 01:05:06]

Description was changed from

==========
Implement support in DefaultComponentInstaller for picking up bundled
components from the plugins home directory.

BUG=
==========

to

==========
DefaultComponentInstaller support for bundled components.

DefaultComponentInstaller will now pick up component implementations from the
plugins directory. It expects the component to have a folder in the plugins
directory with the same name as we would store in USER_DATA_DIR. Unlike
USER_DATA_DIR, this folder should not contain a set of versioned folders, but
the unpacked contents of the CRX itself.

As a consequence of this, the installation directory of a component cannot be
determined purely by PathService, so I have gone ahead and removed the relevant
PathService entries.

BUG=608128
==========

[waffles, 2016-04-30 01:12:23]

Patchset #1 (id:1) has been deleted

[waffles, 2016-04-30 01:12:32]

Patchset #1 (id:20001) has been deleted

[waffles, 2016-04-30 01:12:41]

Patchset #1 (id:40001) has been deleted

[waffles, 2016-04-30 01:14:27]

waffles@chromium.org changed reviewers:
+ sorin@chromium.org, xhwang@chromium.org

[waffles, 2016-04-30 01:14:28]

sorin, xhwang, please take a look. If it lgty, I will include a wider set of
component owners.

I'm concerned about introducing run-time regressions, particularly in the
supervised user whitelist code - not sure how to test that.

[xhwang, 2016-05-02 17:47:00]

looking good to me. I just have a few comments/nits.

https://codereview.chromium.org/1937683002/diff/60001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1937683002/diff/60001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:727:
component_updater::RegisterPathProvider(chrome::DIR_INTERNAL_PLUGINS,
Components are part of chrome and not necessarily plugins. Can we have a path
that is not plugin specific?

https://codereview.chromium.org/1937683002/diff/60001/components/component_up...
File components/component_updater/default_component_installer.cc (right):

https://codereview.chromium.org/1937683002/diff/60001/components/component_up...
components/component_updater/default_component_installer.cc:170: std::string
version_s;
nit: "Names should be descriptive; eschew abbreviation"... how about
version_system?

https://codereview.chromium.org/1937683002/diff/60001/components/component_up...
components/component_updater/default_component_installer.cc:182: }
nit: It might be clearer to wrap this block into a static helper function that
returns a boolean "success" so that you can return early upon any failure case.
This way you can break the 5-if-block into smaller pieces.

https://codereview.chromium.org/1937683002/diff/60001/components/component_up...
File components/component_updater/default_component_installer.h (right):

https://codereview.chromium.org/1937683002/diff/60001/components/component_up...
components/component_updater/default_component_installer.h:81: virtual
std::string GetASCIIDirectoryName() const = 0;
What's the reason that we have to use ASCII dir name (string) instead of the
base::FilePath?

[Sorin Jianu, 2016-05-03 00:04:43]

I will take another look after this code is changed per xhwang@ suggestions.

https://codereview.chromium.org/1937683002/diff/60001/components/component_up...
File components/component_updater/default_component_installer.h (right):

https://codereview.chromium.org/1937683002/diff/60001/components/component_up...
components/component_updater/default_component_installer.h:81: virtual
std::string GetASCIIDirectoryName() const = 0;
A more descriptive name would be desired, not sure though what to suggest.

GetASCIIDirectoryName means: a getter, that returns an ASCII value, which is a
directory. It could be more helpful, such as GetComponentDir, since it appears
that they are component directories in the user data dir.

[waffles, 2016-05-03 19:03:44]

Patchset #2 (id:80001) has been deleted

[waffles, 2016-05-03 21:04:23]

https://codereview.chromium.org/1937683002/diff/60001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1937683002/diff/60001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:727:
component_updater::RegisterPathProvider(chrome::DIR_INTERNAL_PLUGINS,
On 2016/05/02 17:46:59, xhwang wrote:
> Components are part of chrome and not necessarily plugins. Can we have a path
> that is not plugin specific?

This is an interesting discussion, due to how Flash is bundled on Mac (and
whether other components will have to do a similar thing, living in the
framework directory). DIR_INTERNAL_PLUGINS is special-cased in PathService, and
I'd prefer to inherit that behavior. I think wfh@ will have some input here.

https://codereview.chromium.org/1937683002/diff/60001/components/component_up...
File components/component_updater/default_component_installer.cc (right):

https://codereview.chromium.org/1937683002/diff/60001/components/component_up...
components/component_updater/default_component_installer.cc:170: std::string
version_s;
On 2016/05/02 17:47:00, xhwang wrote:
> nit: "Names should be descriptive; eschew abbreviation"... how about
> version_system?

Done.

https://codereview.chromium.org/1937683002/diff/60001/components/component_up...
components/component_updater/default_component_installer.cc:182: }
On 2016/05/02 17:47:00, xhwang wrote:
> nit: It might be clearer to wrap this block into a static helper function that
> returns a boolean "success" so that you can return early upon any failure
case.
> This way you can break the 5-if-block into smaller pieces.

Done.

https://codereview.chromium.org/1937683002/diff/60001/components/component_up...
File components/component_updater/default_component_installer.h (right):

https://codereview.chromium.org/1937683002/diff/60001/components/component_up...
components/component_updater/default_component_installer.h:81: virtual
std::string GetASCIIDirectoryName() const = 0;
On 2016/05/02 17:47:00, xhwang wrote:
> What's the reason that we have to use ASCII dir name (string) instead of the
> base::FilePath?

Clarified that this should be a relative path, changed it back to a
base::FilePath.

https://codereview.chromium.org/1937683002/diff/60001/components/component_up...
components/component_updater/default_component_installer.h:81: virtual
std::string GetASCIIDirectoryName() const = 0;
On 2016/05/03 00:04:43, Sorin Jianu wrote:
> A more descriptive name would be desired, not sure though what to suggest.
> 
> GetASCIIDirectoryName means: a getter, that returns an ASCII value, which is a
> directory. It could be more helpful, such as GetComponentDir, since it appears
> that they are component directories in the user data dir.

Changed the name.

[Sorin Jianu, 2016-05-03 21:51:24]

lgtm

thank you!

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
File components/component_updater/default_component_installer.cc (left):

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
components/component_updater/default_component_installer.cc:199: if
(latest_manifest) {
was this a bug?

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
File components/component_updater/default_component_installer.cc (right):

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
components/component_updater/default_component_installer.cc:155: bool
DefaultComponentInstaller::LoadSystemInstallation() {
This is mostly finding the system install. Maybe rename to
FindSystemInstallation?

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
File components/component_updater/default_component_installer.h (right):

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
components/component_updater/default_component_installer.h:79: // Returns a
filesystem-safe name that will be used as the directory name for
filesystem-safe is redundant now, since the return value is a file path now.

[waffles, 2016-05-03 22:14:05]

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
File components/component_updater/default_component_installer.cc (left):

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
components/component_updater/default_component_installer.cc:199: if
(latest_manifest) {
On 2016/05/03 21:51:24, Sorin Jianu wrote:
> was this a bug?

I believe it was safe, as the latest_manifest and latest_path were (and still
are) always set together, but it seemed a weird way to express what was
happening here.

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
File components/component_updater/default_component_installer.cc (right):

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
components/component_updater/default_component_installer.cc:155: bool
DefaultComponentInstaller::LoadSystemInstallation() {
On 2016/05/03 21:51:24, Sorin Jianu wrote:
> This is mostly finding the system install. Maybe rename to
> FindSystemInstallation?

Done.

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
File components/component_updater/default_component_installer.h (right):

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
components/component_updater/default_component_installer.h:79: // Returns a
filesystem-safe name that will be used as the directory name for
On 2016/05/03 21:51:24, Sorin Jianu wrote:
> filesystem-safe is redundant now, since the return value is a file path now.

Done.

[xhwang, 2016-05-03 22:49:42]

Thanks! I still have concerns about putting components in DIR_INTERNAL_PLUGINS
:) Otherwise it looks good.

https://codereview.chromium.org/1937683002/diff/120001/chrome/app/chrome_main...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1937683002/diff/120001/chrome/app/chrome_main...
chrome/app/chrome_main_delegate.cc:727:
component_updater::RegisterPathProvider(chrome::DIR_INTERNAL_PLUGINS,
From the offline discussion it seems we should put the components in a folder
that is parallel to DIR_INTERNAL_PLUGINS? If Flash doesn't work in this model
today, we should investigate to make it work. Putting components in
DIR_INTERNAL_PLUGINS will likely confuse users and developers..

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
File components/component_updater/default_component_installer.h (right):

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
components/component_updater/default_component_installer.h:121: bool
LoadSystemInstallation();
nit: document what it does and what the return value is

[waffles, 2016-05-04 00:58:08]

waffles@chromium.org changed reviewers:
+ andrewhayden@chromium.org, bauerb@chromium.org, wfh@chromium.org

[waffles, 2016-05-04 00:58:08]

wfh, andrewhayden, bauerb; please take a look and let me know what you think.

Will: Your input on using DIR_INTERNAL_PLUGINS, knowledge about the bundled
deployment mechanism on Mac, and the implications of using this for Flash would
be really valuable.

Andrew: I'm curious if this approach can eliminate some of the stuff you're
doing with component_cld_data_harness.cc.

Bernhard: There are some rough edges around the supervised user whitelist
component that I'd appreciate your LGTM or feedback on.

[Bernhard Bauer, 2016-05-04 08:57:37]

supervised_users/ code looks fine to me. Some possible cleanup though:

https://codereview.chromium.org/1937683002/diff/140001/components/component_u...
File components/component_updater/component_updater_paths.cc (right):

https://codereview.chromium.org/1937683002/diff/140001/components/component_u...
components/component_updater/component_updater_paths.cc:50: case
DIR_SUPERVISED_USER_WHITELISTS:
We should probably remove this value now.

[Will Harris, 2016-05-09 23:08:14]

my gut tells me we shouldn't be using DIR_INTERNAL_PLUGINS for this, can't we
create a new type for this type of bundled component update?

Also, I think separating it from DIR_INTERNAL_PLUGINS might mean you can get rid
of the OS X odd handling, since it will simply just be a directory under the
Chrome binary, always.

[waffles, 2016-05-09 23:14:13]

On 2016/05/09 23:08:14, Will Harris wrote:
> my gut tells me we shouldn't be using DIR_INTERNAL_PLUGINS for this, can't we
> create a new type for this type of bundled component update?
> 
> Also, I think separating it from DIR_INTERNAL_PLUGINS might mean you can get
rid
> of the OS X odd handling, since it will simply just be a directory under the
> Chrome binary, always.

We can definitely create a different enum value to use.

I'm worried that if we don't inherit the strange OS X handling, it will be hard
to unify this solution with Flash's.

[Will Harris, 2016-05-09 23:18:01]

did you get to the bottom of why the strange handling is needed on OS X? Is this
just for testing?

[Andrew Hayden (chromium.org), 2016-05-10 08:42:16]

LGTM for CLD stuff. FWIW, CLD2 component update code is dead. I will clean it
up... I'm still working on burning CLD1 out of the codebase because of some bot
problems, but when that's done I'll turn my gaze to deleting the CLD stuff here
as well.

[waffles, 2016-05-10 18:15:53]

On 2016/05/09 23:18:01, Will Harris wrote:
> did you get to the bottom of why the strange handling is needed on OS X? Is
this
> just for testing?

Discussed in e-mail. General consensus at this time seems to be that the special
case for OS X is not longer necessary.

https://codereview.chromium.org/1937683002/diff/120001/chrome/app/chrome_main...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1937683002/diff/120001/chrome/app/chrome_main...
chrome/app/chrome_main_delegate.cc:727:
component_updater::RegisterPathProvider(chrome::DIR_INTERNAL_PLUGINS,
On 2016/05/03 22:49:42, xhwang wrote:
> From the offline discussion it seems we should put the components in a folder
> that is parallel to DIR_INTERNAL_PLUGINS? If Flash doesn't work in this model
> today, we should investigate to make it work. Putting components in
> DIR_INTERNAL_PLUGINS will likely confuse users and developers..

Switched to DIR_MODULE. On most platforms, this is equivalent (but not on OSX).

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
File components/component_updater/default_component_installer.h (right):

https://codereview.chromium.org/1937683002/diff/120001/components/component_u...
components/component_updater/default_component_installer.h:121: bool
LoadSystemInstallation();
On 2016/05/03 22:49:42, xhwang wrote:
> nit: document what it does and what the return value is

Done.

https://codereview.chromium.org/1937683002/diff/140001/components/component_u...
File components/component_updater/component_updater_paths.cc (right):

https://codereview.chromium.org/1937683002/diff/140001/components/component_u...
components/component_updater/component_updater_paths.cc:50: case
DIR_SUPERVISED_USER_WHITELISTS:
On 2016/05/04 08:57:37, Bernhard Bauer wrote:
> We should probably remove this value now.

Yeah, I wasn't sure about this, because it requires significant refactoring of
supervised_user_whitelist_installer.cc - there's a deeply nested function that
would need a reference to the installer passed to it; I'm inclined to punt that
change into its own CL.

[xhwang, 2016-05-10 18:58:20]

https://codereview.chromium.org/1937683002/diff/120001/chrome/app/chrome_main...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1937683002/diff/120001/chrome/app/chrome_main...
chrome/app/chrome_main_delegate.cc:727:
component_updater::RegisterPathProvider(chrome::DIR_INTERNAL_PLUGINS,
On 2016/05/10 18:15:53, waffles wrote:
> On 2016/05/03 22:49:42, xhwang wrote:
> > From the offline discussion it seems we should put the components in a
folder
> > that is parallel to DIR_INTERNAL_PLUGINS? If Flash doesn't work in this
model
> > today, we should investigate to make it work. Putting components in
> > DIR_INTERNAL_PLUGINS will likely confuse users and developers..
> 
> Switched to DIR_MODULE. On most platforms, this is equivalent (but not on
OSX).

On Mac we don't need to be in "Internet Plug-ins", but do we still need to be in
the "framework"? If so, maybe instead of DIR_MODULE, we should have a
DIR_COMPONENT or DIR_SYSTEM_COMPONENT which will be DIR_MODULE on non-mac
platforms, but will be the framework folder on mac.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro...

[waffles, 2016-05-11 21:42:57]

https://codereview.chromium.org/1937683002/diff/120001/chrome/app/chrome_main...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1937683002/diff/120001/chrome/app/chrome_main...
chrome/app/chrome_main_delegate.cc:727:
component_updater::RegisterPathProvider(chrome::DIR_INTERNAL_PLUGINS,
On 2016/05/10 18:58:20, xhwang wrote:
> On 2016/05/10 18:15:53, waffles wrote:
> > On 2016/05/03 22:49:42, xhwang wrote:
> > > From the offline discussion it seems we should put the components in a
> folder
> > > that is parallel to DIR_INTERNAL_PLUGINS? If Flash doesn't work in this
> model
> > > today, we should investigate to make it work. Putting components in
> > > DIR_INTERNAL_PLUGINS will likely confuse users and developers..
> > 
> > Switched to DIR_MODULE. On most platforms, this is equivalent (but not on
> OSX).
> 
> On Mac we don't need to be in "Internet Plug-ins", but do we still need to be
in
> the "framework"? If so, maybe instead of DIR_MODULE, we should have a
> DIR_COMPONENT or DIR_SYSTEM_COMPONENT which will be DIR_MODULE on non-mac
> platforms, but will be the framework folder on mac.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro...

Added a DIR_COMPONENTS, which is DIR_MODULE usually, but framework/Libraries on
Mac. Note that on ToT Libraries is no longer part of the build. I guess it's
also worth mentioning that some components are just data, not really libraries,
so there's that as well, but I'm OK with it.

[xhwang, 2016-05-12 00:48:15]

lgtm

[waffles, 2016-05-12 01:05:11]

waffles@chromium.org changed reviewers:
+ thakis@chromium.org

[waffles, 2016-05-12 01:05:12]

thakis: Need your review for chrome/common/chrome_paths.*; please also cover
chrome/app/chrome_main_delegate.cc.

[Bernhard Bauer, 2016-05-12 08:37:22]

https://codereview.chromium.org/1937683002/diff/140001/components/component_u...
File components/component_updater/component_updater_paths.cc (right):

https://codereview.chromium.org/1937683002/diff/140001/components/component_u...
components/component_updater/component_updater_paths.cc:50: case
DIR_SUPERVISED_USER_WHITELISTS:
On 2016/05/10 18:15:53, waffles wrote:
> On 2016/05/04 08:57:37, Bernhard Bauer wrote:
> > We should probably remove this value now.
> 
> Yeah, I wasn't sure about this, because it requires significant refactoring of
> supervised_user_whitelist_installer.cc - there's a deeply nested function that
> would need a reference to the installer passed to it; I'm inclined to punt
that
> change into its own CL.

D'oh! I overlooked that occurrence. Hm, as it is right now, things would break
if the value of DIR_SUPERVISED_USER_WHITELISTS and the string literal in the
installer would diverge... Should we extract it to a constant (in
chrome_constants.cc)?

[waffles, 2016-05-12 17:06:07]

https://codereview.chromium.org/1937683002/diff/140001/components/component_u...
File components/component_updater/component_updater_paths.cc (right):

https://codereview.chromium.org/1937683002/diff/140001/components/component_u...
components/component_updater/component_updater_paths.cc:50: case
DIR_SUPERVISED_USER_WHITELISTS:
On 2016/05/12 08:37:22, Bernhard Bauer wrote:
> On 2016/05/10 18:15:53, waffles wrote:
> > On 2016/05/04 08:57:37, Bernhard Bauer wrote:
> > > We should probably remove this value now.
> > 
> > Yeah, I wasn't sure about this, because it requires significant refactoring
of
> > supervised_user_whitelist_installer.cc - there's a deeply nested function
that
> > would need a reference to the installer passed to it; I'm inclined to punt
> that
> > change into its own CL.
> 
> D'oh! I overlooked that occurrence. Hm, as it is right now, things would break
> if the value of DIR_SUPERVISED_USER_WHITELISTS and the string literal in the
> installer would diverge... Should we extract it to a constant (in
> chrome_constants.cc)?

We can't depend directly on chrome_constants from here. How large of a risk do
you think this is?

[Sorin Jianu, 2016-05-12 17:22:07]

lgtm

Thank you! Maybe constnits below.

https://codereview.chromium.org/1937683002/diff/200001/components/component_u...
File components/component_updater/default_component_installer.cc (right):

https://codereview.chromium.org/1937683002/diff/200001/components/component_u...
components/component_updater/default_component_installer.cc:162:
std::unique_ptr<base::DictionaryValue> manifest =
can this be const?

https://codereview.chromium.org/1937683002/diff/200001/components/component_u...
components/component_updater/default_component_installer.cc:169: base::Version
version(version_lexical);
const?

https://codereview.chromium.org/1937683002/diff/200001/components/component_u...
components/component_updater/default_component_installer.cc:268: base::FilePath
base_dir = current_install_dir_.DirName();
maybe const?

[Bernhard Bauer, 2016-05-12 17:28:59]

https://codereview.chromium.org/1937683002/diff/140001/components/component_u...
File components/component_updater/component_updater_paths.cc (right):

https://codereview.chromium.org/1937683002/diff/140001/components/component_u...
components/component_updater/component_updater_paths.cc:50: case
DIR_SUPERVISED_USER_WHITELISTS:
On 2016/05/12 17:06:07, waffles wrote:
> On 2016/05/12 08:37:22, Bernhard Bauer wrote:
> > On 2016/05/10 18:15:53, waffles wrote:
> > > On 2016/05/04 08:57:37, Bernhard Bauer wrote:
> > > > We should probably remove this value now.
> > > 
> > > Yeah, I wasn't sure about this, because it requires significant
refactoring
> of
> > > supervised_user_whitelist_installer.cc - there's a deeply nested function
> that
> > > would need a reference to the installer passed to it; I'm inclined to punt
> > that
> > > change into its own CL.
> > 
> > D'oh! I overlooked that occurrence. Hm, as it is right now, things would
break
> > if the value of DIR_SUPERVISED_USER_WHITELISTS and the string literal in the
> > installer would diverge... Should we extract it to a constant (in
> > chrome_constants.cc)?
> 
> We can't depend directly on chrome_constants from here. How large of a risk do
> you think this is?

In practice, probably not a big risk (this code isn't changing much), but it
would be nice to have a constant to share. Really, we could put it anywhere
where both //components/component_updater and //chrome/component_updater can
access it. But I don't really feel that strongly about it. If in doubt, just add
a TODO? :)

[waffles, 2016-05-13 18:19:13]

Thanks, all!

thakis: PTAL. Would be nice to catch a dev build before the branch cut.

https://codereview.chromium.org/1937683002/diff/140001/components/component_u...
File components/component_updater/component_updater_paths.cc (right):

https://codereview.chromium.org/1937683002/diff/140001/components/component_u...
components/component_updater/component_updater_paths.cc:50: case
DIR_SUPERVISED_USER_WHITELISTS:
On 2016/05/12 17:28:58, Bernhard Bauer wrote:
> On 2016/05/12 17:06:07, waffles wrote:
> > On 2016/05/12 08:37:22, Bernhard Bauer wrote:
> > > On 2016/05/10 18:15:53, waffles wrote:
> > > > On 2016/05/04 08:57:37, Bernhard Bauer wrote:
> > > > > We should probably remove this value now.
> > > > 
> > > > Yeah, I wasn't sure about this, because it requires significant
> refactoring
> > of
> > > > supervised_user_whitelist_installer.cc - there's a deeply nested
function
> > that
> > > > would need a reference to the installer passed to it; I'm inclined to
punt
> > > that
> > > > change into its own CL.
> > > 
> > > D'oh! I overlooked that occurrence. Hm, as it is right now, things would
> break
> > > if the value of DIR_SUPERVISED_USER_WHITELISTS and the string literal in
the
> > > installer would diverge... Should we extract it to a constant (in
> > > chrome_constants.cc)?
> > 
> > We can't depend directly on chrome_constants from here. How large of a risk
do
> > you think this is?
> 
> In practice, probably not a big risk (this code isn't changing much), but it
> would be nice to have a constant to share. Really, we could put it anywhere
> where both //components/component_updater and //chrome/component_updater can
> access it. But I don't really feel that strongly about it. If in doubt, just
add
> a TODO? :)

Added it in component_updater_paths; not sure this is a great home for it, but
it is related to these paths...

https://codereview.chromium.org/1937683002/diff/200001/components/component_u...
File components/component_updater/default_component_installer.cc (right):

https://codereview.chromium.org/1937683002/diff/200001/components/component_u...
components/component_updater/default_component_installer.cc:162:
std::unique_ptr<base::DictionaryValue> manifest =
On 2016/05/12 17:22:07, Sorin Jianu wrote:
> can this be const?

I don't think so, because current_manifest_ is not const, and we want to move it
there on :173.

https://codereview.chromium.org/1937683002/diff/200001/components/component_u...
components/component_updater/default_component_installer.cc:169: base::Version
version(version_lexical);
On 2016/05/12 17:22:07, Sorin Jianu wrote:
> const?

Done.

https://codereview.chromium.org/1937683002/diff/200001/components/component_u...
components/component_updater/default_component_installer.cc:268: base::FilePath
base_dir = current_install_dir_.DirName();
On 2016/05/12 17:22:07, Sorin Jianu wrote:
> maybe const?

Done.

[Nico, 2016-05-16 21:07:01]

Hm, won't overwriting files in the bundle at runtime confuse the bundle code
signing bits? Or is the signature only checked at first run after a download and
this is hence safe? +mark, kerrnel, who are familiar with code signing on mac.

[waffles, 2016-05-16 21:09:07]

On 2016/05/16 21:07:01, Nico (vacation May 19 to 22) wrote:
> Hm, won't overwriting files in the bundle at runtime confuse the bundle code
> signing bits? Or is the signature only checked at first run after a download
and
> this is hence safe? +mark, kerrnel, who are familiar with code signing on mac.

Sorry for the confusion. We don't actually overwrite the files in the bundle for
that reason (plus elevation, etc). Updates are still written into DIR_USER_DATA,
so there will be two implementations on disk (one in USER_DATA, one in
COMPONENTS); we prefer the higher-versioned one, or COMPONENTS in case of a tie.

[Nico, 2016-05-16 21:09:52]

lgtm

https://codereview.chromium.org/1937683002/diff/220001/chrome/common/chrome_p...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/1937683002/diff/220001/chrome/common/chrome_p...
chrome/common/chrome_paths.cc:88: // Gets the path for internal plugins.
this comment could point that out then

[Greg K, 2016-05-16 21:10:09]

On 2016/05/16 21:09:07, waffles wrote:
> On 2016/05/16 21:07:01, Nico (vacation May 19 to 22) wrote:
> > Hm, won't overwriting files in the bundle at runtime confuse the bundle code
> > signing bits? Or is the signature only checked at first run after a download
> and
> > this is hence safe? +mark, kerrnel, who are familiar with code signing on
mac.
> 
> Sorry for the confusion. We don't actually overwrite the files in the bundle
for
> that reason (plus elevation, etc). Updates are still written into
DIR_USER_DATA,
> so there will be two implementations on disk (one in USER_DATA, one in
> COMPONENTS); we prefer the higher-versioned one, or COMPONENTS in case of a
tie.

Thanks for clarifying. Yes, overwriting files in the bundle is something we
cannot do, but it appears these files will still be updated in the
USER_DATA_DIR.

Regards,

kerrnel

[waffles, 2016-05-16 21:32:33]

waffles@chromium.org changed reviewers:
- bauerb@chromium.org, wfh@chromium.org

[waffles, 2016-05-16 21:32:34]

Thanks all! (Will and Bernhard moved to cc.)

https://codereview.chromium.org/1937683002/diff/220001/chrome/common/chrome_p...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/1937683002/diff/220001/chrome/common/chrome_p...
chrome/common/chrome_paths.cc:88: // Gets the path for internal plugins.
On 2016/05/16 21:09:52, Nico (vacation May 19 to 22) wrote:
> this comment could point that out then

Done.

[waffles, 2016-05-16 21:42:28]

The CQ bit was checked by waffles@chromium.org

[waffles, 2016-05-16 21:42:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
andrewhayden@chromium.org, xhwang@chromium.org, sorin@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1937683002/#ps240001
(title: "comments through #35")

[commit-bot: I haz the power, 2016-05-16 21:43:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1937683002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1937683002/240001

[commit-bot: I haz the power, 2016-05-16 22:50:46]

Description was changed from

==========
DefaultComponentInstaller support for bundled components.

DefaultComponentInstaller will now pick up component implementations from the
plugins directory. It expects the component to have a folder in the plugins
directory with the same name as we would store in USER_DATA_DIR. Unlike
USER_DATA_DIR, this folder should not contain a set of versioned folders, but
the unpacked contents of the CRX itself.

As a consequence of this, the installation directory of a component cannot be
determined purely by PathService, so I have gone ahead and removed the relevant
PathService entries.

BUG=608128
==========

to

==========
DefaultComponentInstaller support for bundled components.

DefaultComponentInstaller will now pick up component implementations from the
plugins directory. It expects the component to have a folder in the plugins
directory with the same name as we would store in USER_DATA_DIR. Unlike
USER_DATA_DIR, this folder should not contain a set of versioned folders, but
the unpacked contents of the CRX itself.

As a consequence of this, the installation directory of a component cannot be
determined purely by PathService, so I have gone ahead and removed the relevant
PathService entries.

BUG=608128
==========

[commit-bot: I haz the power, 2016-05-16 22:50:47]

Committed patchset #9 (id:240001)

[commit-bot: I haz the power, 2016-05-16 22:52:07]

Description was changed from

==========
DefaultComponentInstaller support for bundled components.

DefaultComponentInstaller will now pick up component implementations from the
plugins directory. It expects the component to have a folder in the plugins
directory with the same name as we would store in USER_DATA_DIR. Unlike
USER_DATA_DIR, this folder should not contain a set of versioned folders, but
the unpacked contents of the CRX itself.

As a consequence of this, the installation directory of a component cannot be
determined purely by PathService, so I have gone ahead and removed the relevant
PathService entries.

BUG=608128
==========

to

==========
DefaultComponentInstaller support for bundled components.

DefaultComponentInstaller will now pick up component implementations from the
plugins directory. It expects the component to have a folder in the plugins
directory with the same name as we would store in USER_DATA_DIR. Unlike
USER_DATA_DIR, this folder should not contain a set of versioned folders, but
the unpacked contents of the CRX itself.

As a consequence of this, the installation directory of a component cannot be
determined purely by PathService, so I have gone ahead and removed the relevant
PathService entries.

BUG=608128

Committed: https://crrev.com/cb4f42ca4ef394517e4f94a231ae531f331d4ca8
Cr-Commit-Position: refs/heads/master@{#393961}
==========

[commit-bot: I haz the power, 2016-05-16 22:52:09]

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