Componentize component_updater: Move some paths/constants to component.

Break some of the chrome/common dependencies in implementation by moving some paths and constants to the component.

Design doc: https://docs.google.com/document/d/1F76yNZCnPnGzgNXhI-sCFKlXulwx_s_OKTGbJS5NhbA/edit?usp=sharing

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

[tommycli, 2014-06-16 23:37:26]

waffles: This patch breaks a lot of the "simple" dependencies on chrome/common -
mostly constants. PTAL. Thanks!

[blundell, 2014-06-17 17:26:43]

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:803:
Tokenize(cmdline->GetSwitchValueASCII(switches::kComponentUpdater),
is the reason that you moved this code because switches::KComponentUpdater is
defined in //chrome? You can move the switch to be defined in the
component_updater component instead; see e.g. google_switches in
//components/google.

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:420: if
(PathService::Get(chrome::DIR_COMPONENT_CLD2, &cld2_base_dir)) {
Maybe the CLD-related constants should be moved into the translate component?

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_ping_manager.h (right):

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_ping_manager.h:23:
PingManager(const std::string& chrome_version,
s/chrome_version/application_version (since the component isn't
//chrome-specific anymore)

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/componen...
File chrome/browser/component_updater/recovery_component_installer.cc (left):

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/componen...
chrome/browser/component_updater/recovery_component_installer.cc:74: Version
version(prefs->GetString(prefs::kRecoveryComponentVersion));
This pref could move into the component_updater component instead.

https://codereview.chromium.org/334783002/diff/140001/chrome/common/pref_names.h
File chrome/common/pref_names.h (left):

https://codereview.chromium.org/334783002/diff/140001/chrome/common/pref_name...
chrome/common/pref_names.h:748: extern const char kRecoveryComponentVersion[];
Should these be getting moved into the component?

[tommycli, 2014-06-17 19:01:57]

blundell: addressed your comments below. thanks!

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:803:
Tokenize(cmdline->GetSwitchValueASCII(switches::kComponentUpdater),
On 2014/06/17 17:26:42, blundell wrote:
> is the reason that you moved this code because switches::KComponentUpdater is
> defined in //chrome? You can move the switch to be defined in the
> component_updater component instead; see e.g. google_switches in
> //components/google.

Done.

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:420: if
(PathService::Get(chrome::DIR_COMPONENT_CLD2, &cld2_base_dir)) {
On 2014/06/17 17:26:42, blundell wrote:
> Maybe the CLD-related constants should be moved into the translate component?

This particular one would be tricky. Current definition of
chrome::DIR_COMPONENT_CLD2 is chrome::DIR_USER_DATA + "/CLD".

One alternative we could do is to eliminate the DIR_COMPONENT_CLD2 entirely, and
have CldComponentInstallerTraits take the user data dir as an argument, and
simply append /CLD in its implementation.

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_ping_manager.h (right):

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_ping_manager.h:23:
PingManager(const std::string& chrome_version,
On 2014/06/17 17:26:43, blundell wrote:
> s/chrome_version/application_version (since the component isn't
> //chrome-specific anymore)

Done.

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/componen...
File chrome/browser/component_updater/recovery_component_installer.cc (left):

https://codereview.chromium.org/334783002/diff/140001/chrome/browser/componen...
chrome/browser/component_updater/recovery_component_installer.cc:74: Version
version(prefs->GetString(prefs::kRecoveryComponentVersion));
On 2014/06/17 17:26:43, blundell wrote:
> This pref could move into the component_updater component instead.

Done.

https://codereview.chromium.org/334783002/diff/140001/chrome/common/pref_names.h
File chrome/common/pref_names.h (left):

https://codereview.chromium.org/334783002/diff/140001/chrome/common/pref_name...
chrome/common/pref_names.h:748: extern const char kRecoveryComponentVersion[];
On 2014/06/17 17:26:43, blundell wrote:
> Should these be getting moved into the component?

kComponentUpdaterState looks unused. I moved the other to the component.

[blundell, 2014-06-17 19:16:24]

LGTM

https://codereview.chromium.org/334783002/diff/200001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/334783002/diff/200001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:805: configurator,
chrome::VersionInfo().Version(),
nit: I think these should all be on separate lines. Maybe run "git cl format"?

https://codereview.chromium.org/334783002/diff/200001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_ping_manager.cc (right):

https://codereview.chromium.org/334783002/diff/200001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_ping_manager.cc:41: void
SendPing(const std::string& chrome_version,
s/chrome_version/application_version everywhere in this CL

[tommycli, 2014-06-17 19:31:07]

https://codereview.chromium.org/334783002/diff/200001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/334783002/diff/200001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:805: configurator,
chrome::VersionInfo().Version(),
On 2014/06/17 19:16:24, blundell wrote:
> nit: I think these should all be on separate lines. Maybe run "git cl format"?

Done.

https://codereview.chromium.org/334783002/diff/200001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_ping_manager.cc (right):

https://codereview.chromium.org/334783002/diff/200001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_ping_manager.cc:41: void
SendPing(const std::string& chrome_version,
On 2014/06/17 19:16:24, blundell wrote:
> s/chrome_version/application_version everywhere in this CL

Done.

[Sorin Jianu, 2014-06-18 01:10:01]

I will try to get to this review tomorrow. I am behind things as I was tree
sheriff over the weekend.

[tommycli, 2014-06-18 18:15:25]

On 2014/06/18 01:10:01, Sorin Jianu wrote:
> I will try to get to this review tomorrow. I am behind things as I was tree
> sheriff over the weekend.

sorin: thanks

[waffles, 2014-06-18 22:30:37]

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:390: cus,
g_browser_process->local_state(), recovery_base_dir);
Can we avoid passing these directories as arguments? (Is there a reason we might
use a different directory on iOS?)

[tommycli, 2014-06-19 00:28:40]

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:390: cus,
g_browser_process->local_state(), recovery_base_dir);
On 2014/06/18 22:30:37, waffles wrote:
> Can we avoid passing these directories as arguments? (Is there a reason we
might
> use a different directory on iOS?)

DIR_RECOVERY_BASE is always DIR_USER_DATA/recovery.

We could pass in DIR_USER_DATA, eliminate DIR_RECOVERY_BASE, and have the
recovery component hardcode it to "recovery".

Is that what you were thinking?

[waffles, 2014-06-19 00:32:45]

On 2014/06/19 00:28:40, tommycli wrote:
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
> File chrome/browser/chrome_browser_main.cc (right):
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
> chrome/browser/chrome_browser_main.cc:390: cus,
> g_browser_process->local_state(), recovery_base_dir);
> On 2014/06/18 22:30:37, waffles wrote:
> > Can we avoid passing these directories as arguments? (Is there a reason we
> might
> > use a different directory on iOS?)
> 
> DIR_RECOVERY_BASE is always DIR_USER_DATA/recovery.
> 
> We could pass in DIR_USER_DATA, eliminate DIR_RECOVERY_BASE, and have the
> recovery component hardcode it to "recovery".
> 
> Is that what you were thinking?

Well, rather than hardcode, could we move the constant into a /components
constants file?
Is there no equivalent to DIR_USER_DATA outside of /chrome? Where do other
embedders write these files (and is there an embedder-agnostic way to query for
something like DIR_USER_DATA)?

[Sorin Jianu, 2014-06-19 00:48:22]

Tommy, thank you for working on this. I have some ideas about how to organize
this code. It is possible that these ideas are junk and won't work. I will not
take it personally. Sometimes, one needs to try writing the code to understand
if a suggestion is sound or not.

There are two issues we want to improve in this code.

1. How to deal with the paths for the component installers.
2. How to deal with the additional ambient parameters that the component updater
and its dependencies need.

We could break off some of the constants used in the PathService, for #1.

We could use the Configurator interface to support additional configuration
parameters for #2.

I can help you by landing a code to make it easier for you to do #2. Let me know
if you'd like me to do that.

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:793: chrome::VersionInfo().Version(),
This could be members of the configurator.

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:391: }
I second the comment above. It seems we need to have the component paths
available to the components but right now, the directory names are defined as
path service constants somewhere in the "chrome" proper, which we don't want to
depend on. I suggest moving the constants from the chrome_paths.h to some
component_paths.h under src/components. This way, the components would be able
to access their directory paths, and there would be no need to hook up this data
down to Register* functions, then to each component constructor. 

I am not certain this change is possible. But we see that was relatively neat
code before, it is now cluttered with calls to PathService (1 line of code
exploded and it became 5).

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.h (right):

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.h:56: base::FilePath
GetInstalledPath(const base::FilePath& base) const;
It would be nice to not have to touch the definition of component installers as
a result of this refactoring. Especially, change the interface to their class.

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_ping_manager.cc (right):

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_ping_manager.cc:111:
application_version, platform_name, app_element, "");
It looks to me that the additional member variables are just used to call the
free standing public functions in the
"chrome/browser/component_updater/component_updater_utils.h".

In this case, I suggest making the builder functions in the
component_updater_utils.h a builder class, which can take a dependency of the
Configurator, and have the Configurator return these versions and platform name.
I can do this change and you can then rebase that on me.

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_service.cc:147: const
std::string& application_version,
same Configurator deal.

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_service.cc:282:
ping_manager_(new PingManager(application_version,
Seems ok to introduce a dependency on Configurator in the PingManager,
especially since we have some other configuration parameters here.

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_service.h:254: const
std::string& application_version,
we can get rid of the members if we can make them members of the Configurator
interface.

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
File chrome/browser/component_updater/update_checker.cc (right):

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
chrome/browser/component_updater/update_checker.cc:64: application_version,
platform_name, app_elements, additional_attributes);
Same thing as for the ping manager.

[tommycli, 2014-06-20 23:44:28]

On 2014/06/19 00:48:22, Sorin Jianu wrote:
> Tommy, thank you for working on this. I have some ideas about how to organize
> this code. It is possible that these ideas are junk and won't work. I will not
> take it personally. Sometimes, one needs to try writing the code to understand
> if a suggestion is sound or not.
> 
> There are two issues we want to improve in this code.
> 
> 1. How to deal with the paths for the component installers.
> 2. How to deal with the additional ambient parameters that the component
updater
> and its dependencies need.
> 
> We could break off some of the constants used in the PathService, for #1.
> 
> We could use the Configurator interface to support additional configuration
> parameters for #2.
> 
> I can help you by landing a code to make it easier for you to do #2. Let me
know
> if you'd like me to do that.
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/browser_...
> File chrome/browser/browser_process_impl.cc (right):
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/browser_...
> chrome/browser/browser_process_impl.cc:793: chrome::VersionInfo().Version(),
> This could be members of the configurator.
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
> File chrome/browser/chrome_browser_main.cc (right):
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
> chrome/browser/chrome_browser_main.cc:391: }
> I second the comment above. It seems we need to have the component paths
> available to the components but right now, the directory names are defined as
> path service constants somewhere in the "chrome" proper, which we don't want
to
> depend on. I suggest moving the constants from the chrome_paths.h to some
> component_paths.h under src/components. This way, the components would be able
> to access their directory paths, and there would be no need to hook up this
data
> down to Register* functions, then to each component constructor. 
> 
> I am not certain this change is possible. But we see that was relatively neat
> code before, it is now cluttered with calls to PathService (1 line of code
> exploded and it became 5).
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> File chrome/browser/component_updater/cld_component_installer.h (right):
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> chrome/browser/component_updater/cld_component_installer.h:56: base::FilePath
> GetInstalledPath(const base::FilePath& base) const;
> It would be nice to not have to touch the definition of component installers
as
> a result of this refactoring. Especially, change the interface to their class.
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> File chrome/browser/component_updater/component_updater_ping_manager.cc
(right):
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> chrome/browser/component_updater/component_updater_ping_manager.cc:111:
> application_version, platform_name, app_element, "");
> It looks to me that the additional member variables are just used to call the
> free standing public functions in the
> "chrome/browser/component_updater/component_updater_utils.h".
> 
> In this case, I suggest making the builder functions in the
> component_updater_utils.h a builder class, which can take a dependency of the
> Configurator, and have the Configurator return these versions and platform
name.
> I can do this change and you can then rebase that on me.
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> File chrome/browser/component_updater/component_updater_service.cc (right):
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> chrome/browser/component_updater/component_updater_service.cc:147: const
> std::string& application_version,
> same Configurator deal.
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> chrome/browser/component_updater/component_updater_service.cc:282:
> ping_manager_(new PingManager(application_version,
> Seems ok to introduce a dependency on Configurator in the PingManager,
> especially since we have some other configuration parameters here.
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> File chrome/browser/component_updater/component_updater_service.h (right):
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> chrome/browser/component_updater/component_updater_service.h:254: const
> std::string& application_version,
> we can get rid of the members if we can make them members of the Configurator
> interface.
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> File chrome/browser/component_updater/update_checker.cc (right):
> 
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> chrome/browser/component_updater/update_checker.cc:64: application_version,
> platform_name, app_elements, additional_attributes);
> Same thing as for the ping manager.

For #1, we can define the paths under a components_paths.h path provider. It
looks like this is done in components/nacl/common/nacl_paths.h. There is a
nacl::RegisterPathProvider() method called once to register the path provider.
In our case, since all the component paths are dependent on DIR_USER_DATA, we
could make our version have a RegisterPathProvider(base::FilePath
components_root) signature.

If that sounds reasonable, I'll put together a separate patch doing that and
send it to you for review.

For #2. What you said sounds good. We can keep the embedder-specific
ChromeConfigurator in component_updater_configurator.cc under chrome/ and move
the .h file to the component. Then we can eliminate the extra application_name,
version, etc. arguments.

If you want to help and land a CL to start us on that path, that would be great!
If you don't have time, I'll make an attempt after I do #1.

Thanks,
Tommy

[blundell, 2014-06-23 14:27:35]

On 2014/06/20 23:44:28, tommycli wrote:
> On 2014/06/19 00:48:22, Sorin Jianu wrote:
> > Tommy, thank you for working on this. I have some ideas about how to
organize
> > this code. It is possible that these ideas are junk and won't work. I will
not
> > take it personally. Sometimes, one needs to try writing the code to
understand
> > if a suggestion is sound or not.
> > 
> > There are two issues we want to improve in this code.
> > 
> > 1. How to deal with the paths for the component installers.
> > 2. How to deal with the additional ambient parameters that the component
> updater
> > and its dependencies need.
> > 
> > We could break off some of the constants used in the PathService, for #1.
> > 
> > We could use the Configurator interface to support additional configuration
> > parameters for #2.
> > 
> > I can help you by landing a code to make it easier for you to do #2. Let me
> know
> > if you'd like me to do that.
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/browser_...
> > File chrome/browser/browser_process_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/browser_...
> > chrome/browser/browser_process_impl.cc:793: chrome::VersionInfo().Version(),
> > This could be members of the configurator.
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
> > File chrome/browser/chrome_browser_main.cc (right):
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
> > chrome/browser/chrome_browser_main.cc:391: }
> > I second the comment above. It seems we need to have the component paths
> > available to the components but right now, the directory names are defined
as
> > path service constants somewhere in the "chrome" proper, which we don't want
> to
> > depend on. I suggest moving the constants from the chrome_paths.h to some
> > component_paths.h under src/components. This way, the components would be
able
> > to access their directory paths, and there would be no need to hook up this
> data
> > down to Register* functions, then to each component constructor. 
> > 
> > I am not certain this change is possible. But we see that was relatively
neat
> > code before, it is now cluttered with calls to PathService (1 line of code
> > exploded and it became 5).
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > File chrome/browser/component_updater/cld_component_installer.h (right):
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > chrome/browser/component_updater/cld_component_installer.h:56:
base::FilePath
> > GetInstalledPath(const base::FilePath& base) const;
> > It would be nice to not have to touch the definition of component installers
> as
> > a result of this refactoring. Especially, change the interface to their
class.
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > File chrome/browser/component_updater/component_updater_ping_manager.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > chrome/browser/component_updater/component_updater_ping_manager.cc:111:
> > application_version, platform_name, app_element, "");
> > It looks to me that the additional member variables are just used to call
the
> > free standing public functions in the
> > "chrome/browser/component_updater/component_updater_utils.h".
> > 
> > In this case, I suggest making the builder functions in the
> > component_updater_utils.h a builder class, which can take a dependency of
the
> > Configurator, and have the Configurator return these versions and platform
> name.
> > I can do this change and you can then rebase that on me.
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > File chrome/browser/component_updater/component_updater_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > chrome/browser/component_updater/component_updater_service.cc:147: const
> > std::string& application_version,
> > same Configurator deal.
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > chrome/browser/component_updater/component_updater_service.cc:282:
> > ping_manager_(new PingManager(application_version,
> > Seems ok to introduce a dependency on Configurator in the PingManager,
> > especially since we have some other configuration parameters here.
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > File chrome/browser/component_updater/component_updater_service.h (right):
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > chrome/browser/component_updater/component_updater_service.h:254: const
> > std::string& application_version,
> > we can get rid of the members if we can make them members of the
Configurator
> > interface.
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > File chrome/browser/component_updater/update_checker.cc (right):
> > 
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > chrome/browser/component_updater/update_checker.cc:64: application_version,
> > platform_name, app_elements, additional_attributes);
> > Same thing as for the ping manager.
> 
> For #1, we can define the paths under a components_paths.h path provider. It
> looks like this is done in components/nacl/common/nacl_paths.h. There is a
> nacl::RegisterPathProvider() method called once to register the path provider.
> In our case, since all the component paths are dependent on DIR_USER_DATA, we
> could make our version have a RegisterPathProvider(base::FilePath
> components_root) signature.
> 
> If that sounds reasonable, I'll put together a separate patch doing that and
> send it to you for review.
> 
> For #2. What you said sounds good. We can keep the embedder-specific
> ChromeConfigurator in component_updater_configurator.cc under chrome/ and move
> the .h file to the component. Then we can eliminate the extra
application_name,
> version, etc. arguments.
> 

I'd like to avoid having a free-standing function declared in the component and
defined in the embedder. Assuming that component_updater is going to need a
ComponentUpdaterClient interface for information that it gets from its embedder,
maybe MakeComponentUpdaterConfigurator() should be a method on that interface?

> If you want to help and land a CL to start us on that path, that would be
great!
> If you don't have time, I'll make an attempt after I do #1.
> 
> Thanks,
> Tommy

[tommycli, 2014-06-23 20:59:52]

On 2014/06/23 14:27:35, blundell wrote:
> On 2014/06/20 23:44:28, tommycli wrote:
> > On 2014/06/19 00:48:22, Sorin Jianu wrote:
> > > Tommy, thank you for working on this. I have some ideas about how to
> organize
> > > this code. It is possible that these ideas are junk and won't work. I will
> not
> > > take it personally. Sometimes, one needs to try writing the code to
> understand
> > > if a suggestion is sound or not.
> > > 
> > > There are two issues we want to improve in this code.
> > > 
> > > 1. How to deal with the paths for the component installers.
> > > 2. How to deal with the additional ambient parameters that the component
> > updater
> > > and its dependencies need.
> > > 
> > > We could break off some of the constants used in the PathService, for #1.
> > > 
> > > We could use the Configurator interface to support additional
configuration
> > > parameters for #2.
> > > 
> > > I can help you by landing a code to make it easier for you to do #2. Let
me
> > know
> > > if you'd like me to do that.
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/browser_...
> > > File chrome/browser/browser_process_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/browser_...
> > > chrome/browser/browser_process_impl.cc:793:
chrome::VersionInfo().Version(),
> > > This could be members of the configurator.
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
> > > File chrome/browser/chrome_browser_main.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
> > > chrome/browser/chrome_browser_main.cc:391: }
> > > I second the comment above. It seems we need to have the component paths
> > > available to the components but right now, the directory names are defined
> as
> > > path service constants somewhere in the "chrome" proper, which we don't
want
> > to
> > > depend on. I suggest moving the constants from the chrome_paths.h to some
> > > component_paths.h under src/components. This way, the components would be
> able
> > > to access their directory paths, and there would be no need to hook up
this
> > data
> > > down to Register* functions, then to each component constructor. 
> > > 
> > > I am not certain this change is possible. But we see that was relatively
> neat
> > > code before, it is now cluttered with calls to PathService (1 line of code
> > > exploded and it became 5).
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > File chrome/browser/component_updater/cld_component_installer.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > chrome/browser/component_updater/cld_component_installer.h:56:
> base::FilePath
> > > GetInstalledPath(const base::FilePath& base) const;
> > > It would be nice to not have to touch the definition of component
installers
> > as
> > > a result of this refactoring. Especially, change the interface to their
> class.
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > File chrome/browser/component_updater/component_updater_ping_manager.cc
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > chrome/browser/component_updater/component_updater_ping_manager.cc:111:
> > > application_version, platform_name, app_element, "");
> > > It looks to me that the additional member variables are just used to call
> the
> > > free standing public functions in the
> > > "chrome/browser/component_updater/component_updater_utils.h".
> > > 
> > > In this case, I suggest making the builder functions in the
> > > component_updater_utils.h a builder class, which can take a dependency of
> the
> > > Configurator, and have the Configurator return these versions and platform
> > name.
> > > I can do this change and you can then rebase that on me.
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > File chrome/browser/component_updater/component_updater_service.cc
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > chrome/browser/component_updater/component_updater_service.cc:147: const
> > > std::string& application_version,
> > > same Configurator deal.
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > chrome/browser/component_updater/component_updater_service.cc:282:
> > > ping_manager_(new PingManager(application_version,
> > > Seems ok to introduce a dependency on Configurator in the PingManager,
> > > especially since we have some other configuration parameters here.
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > File chrome/browser/component_updater/component_updater_service.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > chrome/browser/component_updater/component_updater_service.h:254: const
> > > std::string& application_version,
> > > we can get rid of the members if we can make them members of the
> Configurator
> > > interface.
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > File chrome/browser/component_updater/update_checker.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > chrome/browser/component_updater/update_checker.cc:64:
application_version,
> > > platform_name, app_elements, additional_attributes);
> > > Same thing as for the ping manager.
> > 
> > For #1, we can define the paths under a components_paths.h path provider. It
> > looks like this is done in components/nacl/common/nacl_paths.h. There is a
> > nacl::RegisterPathProvider() method called once to register the path
provider.
> > In our case, since all the component paths are dependent on DIR_USER_DATA,
we
> > could make our version have a RegisterPathProvider(base::FilePath
> > components_root) signature.
> > 
> > If that sounds reasonable, I'll put together a separate patch doing that and
> > send it to you for review.
> > 
> > For #2. What you said sounds good. We can keep the embedder-specific
> > ChromeConfigurator in component_updater_configurator.cc under chrome/ and
move
> > the .h file to the component. Then we can eliminate the extra
> application_name,
> > version, etc. arguments.
> > 
> 
> I'd like to avoid having a free-standing function declared in the component
and
> defined in the embedder. Assuming that component_updater is going to need a
> ComponentUpdaterClient interface for information that it gets from its
embedder,
> maybe MakeComponentUpdaterConfigurator() should be a method on that interface?
> 
> > If you want to help and land a CL to start us on that path, that would be
> great!
> > If you don't have time, I'll make an attempt after I do #1.
> > 
> > Thanks,
> > Tommy

blundell: I believe ComponentUpdateService::Configurator IS the pure virtual
'client interface' that provides the embedder-specific information.

[Sorin Jianu, 2014-06-23 21:38:52]

I second Tommy's idea. The ComponentUpdateService::Configurator is an abstract
interface. We use it with success to customize the behavior of the
ComponentUpdater in unit tests for dependency injection using
component_updater::ComponentUpdateServiceFactory.

One of the challenges with ComponentUpdateService::Configurator is that is
several member functions which need to be implemented. I don't know of a better
way to solve this problem though.

[tommycli, 2014-06-23 21:55:28]

Hi sorin,

I consolidated all the paths and constants under components/, and removed all
the paths passed as arguments.

If that looks good to you, I can spin off that portion of the patch into a
separate CL we can commit while we continue to work on the Configurator stuff.

Tommy

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:391: }
On 2014/06/19 00:48:21, Sorin Jianu wrote:
> I second the comment above. It seems we need to have the component paths
> available to the components but right now, the directory names are defined as
> path service constants somewhere in the "chrome" proper, which we don't want
to
> depend on. I suggest moving the constants from the chrome_paths.h to some
> component_paths.h under src/components. This way, the components would be able
> to access their directory paths, and there would be no need to hook up this
data
> down to Register* functions, then to each component constructor. 
> 
> I am not certain this change is possible. But we see that was relatively neat
> code before, it is now cluttered with calls to PathService (1 line of code
> exploded and it became 5).

Done.

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.h (right):

https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.h:56: base::FilePath
GetInstalledPath(const base::FilePath& base) const;
On 2014/06/19 00:48:22, Sorin Jianu wrote:
> It would be nice to not have to touch the definition of component installers
as
> a result of this refactoring. Especially, change the interface to their class.

Done.

[Sorin Jianu, 2014-06-23 21:58:10]

Tommy, I am just in the process of moving config stuff around. First change is
mechanical, the second change would make the protocol builders take a dependency
on the Configurator.

ETA end of day today for both. Are you ok with this?

[tommycli, 2014-06-23 22:04:27]

On 2014/06/23 21:58:10, Sorin Jianu wrote:
> Tommy, I am just in the process of moving config stuff around. First change is
> mechanical, the second change would make the protocol builders take a
dependency
> on the Configurator.
> 
> ETA end of day today for both. Are you ok with this?

No problem. Thanks for your help!

[blundell, 2014-06-24 14:11:41]

On 2014/06/23 20:59:52, tommycli wrote:
> On 2014/06/23 14:27:35, blundell wrote:
> > On 2014/06/20 23:44:28, tommycli wrote:
> > > On 2014/06/19 00:48:22, Sorin Jianu wrote:
> > > > Tommy, thank you for working on this. I have some ideas about how to
> > organize
> > > > this code. It is possible that these ideas are junk and won't work. I
will
> > not
> > > > take it personally. Sometimes, one needs to try writing the code to
> > understand
> > > > if a suggestion is sound or not.
> > > > 
> > > > There are two issues we want to improve in this code.
> > > > 
> > > > 1. How to deal with the paths for the component installers.
> > > > 2. How to deal with the additional ambient parameters that the component
> > > updater
> > > > and its dependencies need.
> > > > 
> > > > We could break off some of the constants used in the PathService, for
#1.
> > > > 
> > > > We could use the Configurator interface to support additional
> configuration
> > > > parameters for #2.
> > > > 
> > > > I can help you by landing a code to make it easier for you to do #2. Let
> me
> > > know
> > > > if you'd like me to do that.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/browser_...
> > > > File chrome/browser/browser_process_impl.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/browser_...
> > > > chrome/browser/browser_process_impl.cc:793:
> chrome::VersionInfo().Version(),
> > > > This could be members of the configurator.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
> > > > File chrome/browser/chrome_browser_main.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/chrome_b...
> > > > chrome/browser/chrome_browser_main.cc:391: }
> > > > I second the comment above. It seems we need to have the component paths
> > > > available to the components but right now, the directory names are
defined
> > as
> > > > path service constants somewhere in the "chrome" proper, which we don't
> want
> > > to
> > > > depend on. I suggest moving the constants from the chrome_paths.h to
some
> > > > component_paths.h under src/components. This way, the components would
be
> > able
> > > > to access their directory paths, and there would be no need to hook up
> this
> > > data
> > > > down to Register* functions, then to each component constructor. 
> > > > 
> > > > I am not certain this change is possible. But we see that was relatively
> > neat
> > > > code before, it is now cluttered with calls to PathService (1 line of
code
> > > > exploded and it became 5).
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > > File chrome/browser/component_updater/cld_component_installer.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > > chrome/browser/component_updater/cld_component_installer.h:56:
> > base::FilePath
> > > > GetInstalledPath(const base::FilePath& base) const;
> > > > It would be nice to not have to touch the definition of component
> installers
> > > as
> > > > a result of this refactoring. Especially, change the interface to their
> > class.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > > File chrome/browser/component_updater/component_updater_ping_manager.cc
> > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > > chrome/browser/component_updater/component_updater_ping_manager.cc:111:
> > > > application_version, platform_name, app_element, "");
> > > > It looks to me that the additional member variables are just used to
call
> > the
> > > > free standing public functions in the
> > > > "chrome/browser/component_updater/component_updater_utils.h".
> > > > 
> > > > In this case, I suggest making the builder functions in the
> > > > component_updater_utils.h a builder class, which can take a dependency
of
> > the
> > > > Configurator, and have the Configurator return these versions and
platform
> > > name.
> > > > I can do this change and you can then rebase that on me.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > > File chrome/browser/component_updater/component_updater_service.cc
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > > chrome/browser/component_updater/component_updater_service.cc:147: const
> > > > std::string& application_version,
> > > > same Configurator deal.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > > chrome/browser/component_updater/component_updater_service.cc:282:
> > > > ping_manager_(new PingManager(application_version,
> > > > Seems ok to introduce a dependency on Configurator in the PingManager,
> > > > especially since we have some other configuration parameters here.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > > File chrome/browser/component_updater/component_updater_service.h
(right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > > chrome/browser/component_updater/component_updater_service.h:254: const
> > > > std::string& application_version,
> > > > we can get rid of the members if we can make them members of the
> > Configurator
> > > > interface.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > > File chrome/browser/component_updater/update_checker.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/334783002/diff/240001/chrome/browser/componen...
> > > > chrome/browser/component_updater/update_checker.cc:64:
> application_version,
> > > > platform_name, app_elements, additional_attributes);
> > > > Same thing as for the ping manager.
> > > 
> > > For #1, we can define the paths under a components_paths.h path provider.
It
> > > looks like this is done in components/nacl/common/nacl_paths.h. There is a
> > > nacl::RegisterPathProvider() method called once to register the path
> provider.
> > > In our case, since all the component paths are dependent on DIR_USER_DATA,
> we
> > > could make our version have a RegisterPathProvider(base::FilePath
> > > components_root) signature.
> > > 
> > > If that sounds reasonable, I'll put together a separate patch doing that
and
> > > send it to you for review.
> > > 
> > > For #2. What you said sounds good. We can keep the embedder-specific
> > > ChromeConfigurator in component_updater_configurator.cc under chrome/ and
> move
> > > the .h file to the component. Then we can eliminate the extra
> > application_name,
> > > version, etc. arguments.
> > > 
> > 
> > I'd like to avoid having a free-standing function declared in the component
> and
> > defined in the embedder. Assuming that component_updater is going to need a
> > ComponentUpdaterClient interface for information that it gets from its
> embedder,
> > maybe MakeComponentUpdaterConfigurator() should be a method on that
interface?
> > 
> > > If you want to help and land a CL to start us on that path, that would be
> > great!
> > > If you don't have time, I'll make an attempt after I do #1.
> > > 
> > > Thanks,
> > > Tommy
> 
> blundell: I believe ComponentUpdateService::Configurator IS the pure virtual
> 'client interface' that provides the embedder-specific information.

I think maybe I misunderstood? I thought that you were suggesting moving
component_updater_configurator.h into the component. That's what I was objecting
to.

[tommycli, 2014-06-26 21:58:20]

sorin: This patch now just moves some paths and constants into
components/component_updater. I'm going to put the Configurator changes in a
separate patch.

[Sorin Jianu, 2014-06-26 22:02:51]

Tommy, I will take a look after my change lands and this code is rebased. Is
that ok?

[tommycli, 2014-06-26 22:07:42]

On 2014/06/26 22:02:51, Sorin Jianu wrote:
> Tommy, I will take a look after my change lands and this code is rebased. Is
> that ok?

No problem. Thanks!

[Sorin Jianu, 2014-06-26 22:30:34]

The CQ bit was checked by sorin@chromium.org

[Sorin Jianu, 2014-06-26 22:30:37]

The CQ bit was unchecked by sorin@chromium.org

[tommycli, 2014-06-27 17:48:12]

sorin: I rebased onto your patch. I have some questions, listed below:

Thanks!

https://codereview.chromium.org/334783002/diff/380001/chrome/common/chrome_pa...
File chrome/common/chrome_paths.cc (left):

https://codereview.chromium.org/334783002/diff/380001/chrome/common/chrome_pa...
chrome/common/chrome_paths.cc:389: case chrome::FILE_WIDEVINE_CDM_ADAPTER:
This widevine path also belongs in components/, but this path is hard to move,
as it depends on the GetInternalPlugins directory.

One way we could do this is add a second argument to the PathProvider
registration, and pass in a the path, retrieved via
chrome::DIR_INTERNAL_COMPONENTS, in the same way we are going to pass in
chrome::DIR_USER_DATA.

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
File components/component_updater/common/component_updater_paths.cc (right):

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
components/component_updater/common/component_updater_paths.cc:47: void
RegisterPathProvider(const base::FilePath& components_root) {
Are we okay with this path provider needing a root directory before it can be
registered? This is necessary because the component directories depend on the
user data directory, defined in chrome/

[Sorin Jianu, 2014-06-30 22:27:53]

Thank you Tommy.

First, it appears that we have not added new members to the Configurator. Will
this be part of a future CL?

Second, I don't have an opinion about the path providers.

Third, perhaps we can consolidate some of the constants instead of a file for
each constant definition.

https://codereview.chromium.org/334783002/diff/380001/chrome/common/chrome_pa...
File chrome/common/chrome_paths.cc (left):

https://codereview.chromium.org/334783002/diff/380001/chrome/common/chrome_pa...
chrome/common/chrome_paths.cc:389: case chrome::FILE_WIDEVINE_CDM_ADAPTER:
On 2014/06/27 17:48:11, tommycli wrote:
> This widevine path also belongs in components/, but this path is hard to move,
> as it depends on the GetInternalPlugins directory.
> 
> One way we could do this is add a second argument to the PathProvider
> registration, and pass in a the path, retrieved via
> chrome::DIR_INTERNAL_COMPONENTS, in the same way we are going to pass in
> chrome::DIR_USER_DATA.

Tommy, I don't understand what this means. Two ideas though: for changes to CDM
we need to consult with xhwang@. Second, maybe we do this change in a separate
CL where I could see the code.

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
File components/component_updater.gypi (right):

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
components/component_updater.gypi:17:
'component_updater/browser/component_updater_switches.cc',
That is another the part I am not certain about. Do we need "browser" and
"common" directories?
Do we need all these files or can we move all or most of these constants into a
component_updater_constants instead of few constants in many files. Just an
idea.

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
File components/component_updater/DEPS (right):

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
components/component_updater/DEPS:1: include_rules = [
Is this unchanged?

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
File components/component_updater/browser/component_updater_switches.h (right):

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
components/component_updater/browser/component_updater_switches.h:5: #ifndef
COMPONENTS_COMPONENT_UPDATER_BROWSER_COMPONENT_UPDATER_SWITCHES_H_
I wonder why this does not look like a new file.

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
File components/component_updater/common/component_updater_paths.cc (right):

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
components/component_updater/common/component_updater_paths.cc:47: void
RegisterPathProvider(const base::FilePath& components_root) {
On 2014/06/27 17:48:12, tommycli wrote:
> Are we okay with this path provider needing a root directory before it can be
> registered? This is necessary because the component directories depend on the
> user data directory, defined in chrome/

No idea, I don't know how these providers work.

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
File components/component_updater/common/component_updater_paths.h (right):

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
components/component_updater/common/component_updater_paths.h:13: PATH_START =
10000,
How did we come up with this hard-coded value here?

[tommycli, 2014-07-01 00:03:04]

Hi sorin,

Yes, the configurator changes will be a different CL. I tried to keep this one
smaller for ease of reviewing.

I'll ask thestig about the PathProvider issue.

https://codereview.chromium.org/334783002/diff/380001/chrome/common/chrome_pa...
File chrome/common/chrome_paths.cc (left):

https://codereview.chromium.org/334783002/diff/380001/chrome/common/chrome_pa...
chrome/common/chrome_paths.cc:389: case chrome::FILE_WIDEVINE_CDM_ADAPTER:
On 2014/06/30 22:27:53, Sorin Jianu wrote:
> On 2014/06/27 17:48:11, tommycli wrote:
> > This widevine path also belongs in components/, but this path is hard to
move,
> > as it depends on the GetInternalPlugins directory.
> > 
> > One way we could do this is add a second argument to the PathProvider
> > registration, and pass in a the path, retrieved via
> > chrome::DIR_INTERNAL_COMPONENTS, in the same way we are going to pass in
> > chrome::DIR_USER_DATA.
> 
> Tommy, I don't understand what this means. Two ideas though: for changes to
CDM
> we need to consult with xhwang@. Second, maybe we do this change in a separate
> CL where I could see the code.

Sounds good. Let's do it in a separate CL.

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
File components/component_updater.gypi (right):

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
components/component_updater.gypi:17:
'component_updater/browser/component_updater_switches.cc',
On 2014/06/30 22:27:53, Sorin Jianu wrote:
> That is another the part I am not certain about. Do we need "browser" and
> "common" directories?
> Do we need all these files or can we move all or most of these constants into
a
> component_updater_constants instead of few constants in many files. Just an
> idea.
> 

Done.

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
File components/component_updater/DEPS (right):

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
components/component_updater/DEPS:1: include_rules = [
On 2014/06/30 22:27:53, Sorin Jianu wrote:
> Is this unchanged?

It's a new file, but 'git' is detecting it as a 'copy' from /sync/base/DEPS,
which has the same contents.

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
File components/component_updater/browser/component_updater_switches.h (right):

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
components/component_updater/browser/component_updater_switches.h:5: #ifndef
COMPONENTS_COMPONENT_UPDATER_BROWSER_COMPONENT_UPDATER_SWITCHES_H_
On 2014/06/30 22:27:53, Sorin Jianu wrote:
> I wonder why this does not look like a new file.

I think 'git' detects this as a copy, since the contents are pretty similar.

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
File components/component_updater/common/component_updater_paths.cc (right):

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
components/component_updater/common/component_updater_paths.cc:47: void
RegisterPathProvider(const base::FilePath& components_root) {
On 2014/06/30 22:27:53, Sorin Jianu wrote:
> On 2014/06/27 17:48:12, tommycli wrote:
> > Are we okay with this path provider needing a root directory before it can
be
> > registered? This is necessary because the component directories depend on
the
> > user data directory, defined in chrome/
> 
> No idea, I don't know how these providers work.

No problem, I'll ask thestig to take a second look at it.

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
File components/component_updater/common/component_updater_paths.h (right):

https://codereview.chromium.org/334783002/diff/380001/components/component_up...
components/component_updater/common/component_updater_paths.h:13: PATH_START =
10000,
On 2014/06/30 22:27:53, Sorin Jianu wrote:
> How did we come up with this hard-coded value here?

PATH_START must be different for each PathProvider.

The previous highest one was 9000, and they increment by 1000 each time.

See:
https://code.google.com/p/chromium/codesearch#search/&q=%22PATH_START%20=%22&...

[tommycli, 2014-07-01 00:04:06]

sorin:

Oh, and one more thing:

I got rid of component_updater_constants. It was unnecessary.

I couldn't consolidate component_updater_switches and component_updater_paths,
and the pattern in other components was to keep them separate.

[tommycli, 2014-07-01 00:05:36]

thestig: I have a question on PathProvider, below.

https://codereview.chromium.org/334783002/diff/440001/components/component_up...
File components/component_updater/component_updater_paths.cc (right):

https://codereview.chromium.org/334783002/diff/440001/components/component_up...
components/component_updater/component_updater_paths.cc:46: void
RegisterPathProvider(const base::FilePath& components_root) {
Does this look reasonable? Since the component paths depend on DIR_USER_DATA,
I'm passing that value in at register-time.

[Lei Zhang, 2014-07-01 00:23:29]

https://codereview.chromium.org/334783002/diff/440001/components/component_up...
File components/component_updater/component_updater_paths.cc (right):

https://codereview.chromium.org/334783002/diff/440001/components/component_up...
components/component_updater/component_updater_paths.cc:21: NOTREACHED();
Just DCHECK instead of handling?

https://codereview.chromium.org/334783002/diff/440001/components/component_up...
components/component_updater/component_updater_paths.cc:46: void
RegisterPathProvider(const base::FilePath& components_root) {
On 2014/07/01 00:05:36, tommycli wrote:
> Does this look reasonable? Since the component paths depend on DIR_USER_DATA,
> I'm passing that value in at register-time.

Seems reasonable.

[tommycli, 2014-07-01 00:28:56]

https://codereview.chromium.org/334783002/diff/440001/components/component_up...
File components/component_updater/component_updater_paths.cc (right):

https://codereview.chromium.org/334783002/diff/440001/components/component_up...
components/component_updater/component_updater_paths.cc:21: NOTREACHED();
On 2014/07/01 00:23:29, Lei Zhang wrote:
> Just DCHECK instead of handling?

Done.

https://codereview.chromium.org/334783002/diff/440001/components/component_up...
components/component_updater/component_updater_paths.cc:46: void
RegisterPathProvider(const base::FilePath& components_root) {
On 2014/07/01 00:23:29, Lei Zhang wrote:
> On 2014/07/01 00:05:36, tommycli wrote:
> > Does this look reasonable? Since the component paths depend on
DIR_USER_DATA,
> > I'm passing that value in at register-time.
> 
> Seems reasonable.

Done.

[tommycli, 2014-07-01 00:29:59]

thestig: may i have a review for:

chrome/app
chrome/common
chrome/test/base
?

[Lei Zhang, 2014-07-01 05:44:32]

chrome/ lgtm

[tommycli, 2014-07-01 17:26:55]

sorin: addressed your comments, thanks!

[Sorin Jianu, 2014-07-01 18:00:27]

lgtm

Thank you Tommy!

In case, you did not do this already, please run lint and format over the
component updater files our team owns.

Also, I'd like to ask you nicely to sit on this CL for a few days. I might land
something that needs to be merged in M37 and I prefer to have a simple merge if
the CUS files don't move.

[tommycli, 2014-07-01 18:39:07]

On 2014/07/01 18:00:27, Sorin Jianu wrote:
> lgtm
> 
> Thank you Tommy!
> 
> In case, you did not do this already, please run lint and format over the
> component updater files our team owns.
> 
> Also, I'd like to ask you nicely to sit on this CL for a few days. I might
land
> something that needs to be merged in M37 and I prefer to have a simple merge
if
> the CUS files don't move.

Thanks, I'll sit on this patch until you let me know when it's okay to send to
CQ then.

I already did git cl format on this, let me know if you have another linter in
mind.

[Sorin Jianu, 2014-07-01 18:46:52]

We run:
git cl lint
git cl format

[tommycli, 2014-07-07 16:20:07]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-07-07 16:20:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/334783002/500001

[commit-bot: I haz the power, 2014-07-07 18:06:28]

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

[commit-bot: I haz the power, 2014-07-07 18:14:38]

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

[commit-bot: I haz the power, 2014-07-07 18:14:39]

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

[tommycli, 2014-07-07 19:25:57]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-07-07 19:27:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/334783002/500001

[tommycli, 2014-07-07 19:28:18]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2014-07-07 19:42:44]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-07-07 19:43:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/334783002/520001

[tommycli, 2014-07-08 00:32:59]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-07-08 00:33:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/334783002/560001

[commit-bot: I haz the power, 2014-07-08 04:00:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-08 04:28:13]

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

[commit-bot: I haz the power, 2014-07-08 04:28:14]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)

[tommycli, 2014-07-08 13:34:56]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-07-08 13:35:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/334783002/560001

[commit-bot: I haz the power, 2014-07-08 14:17:05]

Change committed as 281744