Componentize ComponentUpdaterConfigurator

Componentize ComponentUpdaterConfigurator

This CL splits a cross platform subset of
ChromeComponentUpdaterConfigurator into
//components/component_updater/configurator_impl
That way the code can be shared on iOS.

This CL also provides a iOS implementation of the configurator in
//ios/chrome.

BUG=517890
Committed: https://crrev.com/f8a881411ab905efb1405d0f915252e5df7d2a23
Cr-Commit-Position: refs/heads/master@{#344173}

[droger, 2015-08-07 14:21:56]

Patchset #1 (id:1) has been deleted

[droger, 2015-08-07 14:40:54]

Patchset #1 (id:20001) has been deleted

[droger, 2015-08-07 14:48:03]

droger@chromium.org changed reviewers:
+ sorin@chromium.org

[droger, 2015-08-07 14:49:44]

https://codereview.chromium.org/1281913002/diff/40001/ios/chrome/browser/comp...
File ios/chrome/browser/component_updater/component_updater_configurator.cc
(right):

https://codereview.chromium.org/1281913002/diff/40001/ios/chrome/browser/comp...
ios/chrome/browser/component_updater/component_updater_configurator.cc:57:
return nullptr;
Is it fine to return null here? On iOS we can't do anything out of process.

[Sorin Jianu, 2015-08-07 19:02:01]

Thank you, I will take a look at this today.

[Sorin Jianu, 2015-08-08 01:00:30]

I still need to chew on the change. Sorry for the wait.

[Sorin Jianu, 2015-08-11 02:27:36]

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

[Sorin Jianu, 2015-08-11 02:27:37]

Thank you for sending the CL for review. We talked about it over. This
refactoring needs to happen, unfortunately it touches a piece of code that is
not great to begin with.

This is what we would like you to consider. Create ConfiguratorImpl class that
implements the reusable implementation that we want to share between the
instances of update::Configurator on desktop and iOS platforms. Then, use
composition instead of inheritance to reuse the common implementation. The most
derived class will override what they must, while mechanically delegate most of
the overrides to the common implementation, contained within as a data member.

We don't like the design based on DefaultConfigurator due to what's called
"fragile base class problem" or implementation inheritance.

[droger, 2015-08-11 11:11:07]

Done. Please take a look.

[Sorin Jianu, 2015-08-12 02:27:51]

Thank you! We need to reconsider deleting the files in the CL, please see the
rationale articulated in the bug.

Other than that, the code is mostly fine, we are touching a part of the code
that has many rough edges, but it will be the owners' responsibility to fix it
later on.

https://codereview.chromium.org/1281913002/diff/40001/ios/chrome/browser/comp...
File ios/chrome/browser/component_updater/component_updater_configurator.cc
(right):

https://codereview.chromium.org/1281913002/diff/40001/ios/chrome/browser/comp...
ios/chrome/browser/component_updater/component_updater_configurator.cc:57:
return nullptr;
On 2015/08/07 14:49:44, droger wrote:
> Is it fine to return null here? On iOS we can't do anything out of process.

It is correct to return nullptr in this case, according to
https://code.google.com/p/chromium/codesearch#chromium/src/components/update_...

and the code at: 
https://code.google.com/p/chromium/codesearch#chromium/src/components/update_...

https://codereview.chromium.org/1281913002/diff/80001/chrome/browser/componen...
File chrome/browser/component_updater/chrome_component_updater_configurator.cc
(right):

https://codereview.chromium.org/1281913002/diff/80001/chrome/browser/componen...
chrome/browser/component_updater/chrome_component_updater_configurator.cc:24: //
update_client::Configurator:
I think this comment should be // update_client::Configurator overrides.

https://codereview.chromium.org/1281913002/diff/80001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_url_constants.h (left):

https://codereview.chromium.org/1281913002/diff/80001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_url_constants.h:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
There is some context for why these files were created:
https://code.google.com/p/chromium/issues/detail?id=476211

https://codereview.chromium.org/1281913002/diff/80001/components/component_up...
File components/component_updater/configurator_impl.h (right):

https://codereview.chromium.org/1281913002/diff/80001/components/component_up...
components/component_updater/configurator_impl.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/1281913002/diff/80001/components/component_up...
components/component_updater/configurator_impl.h:12: #include "base/version.h"
technically, a forward decl for base::Version is sufficient but not a big deal.

https://codereview.chromium.org/1281913002/diff/80001/components/update_clien...
File components/update_client/update_query_params_delegate.h (right):

https://codereview.chromium.org/1281913002/diff/80001/components/update_clien...
components/update_client/update_query_params_delegate.h:27: // Returns the value
we use for the "updaterchannel=" and "prodchannel="
This addition is suboptimal. UpdateQueryParamsDelegate used to be a pure
interface, now it has a static member. I agree that the design of the class is
debatable (has a ctor) and refactoring this is not in scope.

https://codereview.chromium.org/1281913002/diff/80001/ios/chrome/browser/comp...
File ios/chrome/browser/component_updater/component_updater_configurator.cc
(right):

https://codereview.chromium.org/1281913002/diff/80001/ios/chrome/browser/comp...
ios/chrome/browser/component_updater/component_updater_configurator.cc:26: //
update_client::Configurator:
update_client::Configurator overrides.

https://codereview.chromium.org/1281913002/diff/80001/ios/chrome/browser/comp...
ios/chrome/browser/component_updater/component_updater_configurator.cc:100:
return GetApplicationContext()->GetApplicationLocale();
In the future, would it be possible to get a componentized implementation for
GetChannel and GetLang, which work for both Chrome and iOS? I am thinking that
if we can get these implemented as a component, then, we could mechanically
delegate their implementation to configurator_impl_. In this case, the only
aspect that needs customization is the threading aspect (which makes sense,
since I understand that threading is different).

https://codereview.chromium.org/1281913002/diff/80001/ios/chrome/browser/comp...
File ios/chrome/browser/component_updater/component_updater_configurator.h
(right):

https://codereview.chromium.org/1281913002/diff/80001/ios/chrome/browser/comp...
ios/chrome/browser/component_updater/component_updater_configurator.h:21:
scoped_refptr<update_client::Configurator> MakeComponentUpdaterConfigurator(
I don't know how the code is structured on the iOS side.

I see MakeComponentUpdaterConfigurator is introduced in the component_updater
namespace. Would we have a ODR violation if later on, on the Chrome side we
would define a similar name? On Chrome end, we are using
MakeChromeComponentUpdaterConfigurator. Note the Chrome qualification.

[droger, 2015-08-12 08:43:42]

https://codereview.chromium.org/1281913002/diff/80001/chrome/browser/componen...
File chrome/browser/component_updater/chrome_component_updater_configurator.cc
(right):

https://codereview.chromium.org/1281913002/diff/80001/chrome/browser/componen...
chrome/browser/component_updater/chrome_component_updater_configurator.cc:24: //
update_client::Configurator:
On 2015/08/12 02:27:51, Sorin Jianu wrote:
> I think this comment should be // update_client::Configurator overrides.

Done.

https://codereview.chromium.org/1281913002/diff/80001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_url_constants.h (left):

https://codereview.chromium.org/1281913002/diff/80001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_url_constants.h:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/08/12 02:27:51, Sorin Jianu wrote:
> There is some context for why these files were created:
> https://code.google.com/p/chromium/issues/detail?id=476211

Done.

https://codereview.chromium.org/1281913002/diff/80001/components/component_up...
File components/component_updater/configurator_impl.h (right):

https://codereview.chromium.org/1281913002/diff/80001/components/component_up...
components/component_updater/configurator_impl.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/08/12 02:27:51, Sorin Jianu wrote:
> 2015

Done.

https://codereview.chromium.org/1281913002/diff/80001/components/component_up...
components/component_updater/configurator_impl.h:12: #include "base/version.h"
On 2015/08/12 02:27:51, Sorin Jianu wrote:
> technically, a forward decl for base::Version is sufficient but not a big
deal.

base::Version is used as a return type on line 60, so I added this include to
conform to the IWYU rule.
Is this reasoning wrong?

https://codereview.chromium.org/1281913002/diff/80001/components/update_clien...
File components/update_client/update_query_params_delegate.h (right):

https://codereview.chromium.org/1281913002/diff/80001/components/update_clien...
components/update_client/update_query_params_delegate.h:27: // Returns the value
we use for the "updaterchannel=" and "prodchannel="
On 2015/08/12 02:27:51, Sorin Jianu wrote:
> This addition is suboptimal. UpdateQueryParamsDelegate used to be a pure
> interface, now it has a static member. I agree that the design of the class is
> debatable (has a ctor) and refactoring this is not in scope.

I can move this function elsewhere if you prefer.
It can also be a free function (not in a class).

Just let me know.

https://codereview.chromium.org/1281913002/diff/80001/ios/chrome/browser/comp...
File ios/chrome/browser/component_updater/component_updater_configurator.cc
(right):

https://codereview.chromium.org/1281913002/diff/80001/ios/chrome/browser/comp...
ios/chrome/browser/component_updater/component_updater_configurator.cc:26: //
update_client::Configurator:
On 2015/08/12 02:27:51, Sorin Jianu wrote:
> update_client::Configurator overrides.

Done.

https://codereview.chromium.org/1281913002/diff/80001/ios/chrome/browser/comp...
ios/chrome/browser/component_updater/component_updater_configurator.cc:100:
return GetApplicationContext()->GetApplicationLocale();
On 2015/08/12 02:27:51, Sorin Jianu wrote:
> In the future, would it be possible to get a componentized implementation for
> GetChannel and GetLang, which work for both Chrome and iOS? I am thinking that
> if we can get these implemented as a component, then, we could mechanically
> delegate their implementation to configurator_impl_. In this case, the only
> aspect that needs customization is the threading aspect (which makes sense,
> since I understand that threading is different).

You are right.
It would be possible to componentize that, but it is unlikely to happen in the
short term for priority reasons (cost/benefit ratio is lower than a lot of other
componentization work).

https://codereview.chromium.org/1281913002/diff/80001/ios/chrome/browser/comp...
File ios/chrome/browser/component_updater/component_updater_configurator.h
(right):

https://codereview.chromium.org/1281913002/diff/80001/ios/chrome/browser/comp...
ios/chrome/browser/component_updater/component_updater_configurator.h:21:
scoped_refptr<update_client::Configurator> MakeComponentUpdaterConfigurator(
On 2015/08/12 02:27:51, Sorin Jianu wrote:
> I don't know how the code is structured on the iOS side.
> 
> I see MakeComponentUpdaterConfigurator is introduced in the component_updater
> namespace. Would we have a ODR violation if later on, on the Chrome side we
> would define a similar name? On Chrome end, we are using
> MakeChromeComponentUpdaterConfigurator. Note the Chrome qualification.

iOS does not use //chrome at all, so technically it should be fine to have the
same names.
However, I would like to avoid using the same names if we can, because this can
be confusing and makes searching code harder.
I renamed to MakeIOSComponentUpdaterConfigurator, that should reduce the risk of
name collision.

[Sorin Jianu, 2015-08-12 17:16:55]

lgtm

Thank you! I forgot to ask you to run git cl format before you land. The code
looks ok style-wise but we always rely on cl format.

https://codereview.chromium.org/1281913002/diff/80001/components/component_up...
File components/component_updater/configurator_impl.h (right):

https://codereview.chromium.org/1281913002/diff/80001/components/component_up...
components/component_updater/configurator_impl.h:12: #include "base/version.h"
On 2015/08/12 08:43:41, droger wrote:
> On 2015/08/12 02:27:51, Sorin Jianu wrote:
> > technically, a forward decl for base::Version is sufficient but not a big
> deal.
> 
> base::Version is used as a return type on line 60, so I added this include to
> conform to the IWYU rule.
> Is this reasoning wrong?

The idea is that types that are used for parameters and return types in function
declarations can be forward declared:
http://stackoverflow.com/questions/553682/when-can-i-use-a-forward-declaration

https://codereview.chromium.org/1281913002/diff/80001/components/update_clien...
File components/update_client/update_query_params_delegate.h (right):

https://codereview.chromium.org/1281913002/diff/80001/components/update_clien...
components/update_client/update_query_params_delegate.h:27: // Returns the value
we use for the "updaterchannel=" and "prodchannel="
On 2015/08/12 08:43:41, droger wrote:
> On 2015/08/12 02:27:51, Sorin Jianu wrote:
> > This addition is suboptimal. UpdateQueryParamsDelegate used to be a pure
> > interface, now it has a static member. I agree that the design of the class
is
> > debatable (has a ctor) and refactoring this is not in scope.
> 
> I can move this function elsewhere if you prefer.
> It can also be a free function (not in a class).
> 
> Just let me know.

Free function would be better imho.

[droger, 2015-08-13 11:27:35]

droger@chromium.org changed reviewers:
+ asargent@chromium.org, sdefresne@chromium.org

[droger, 2015-08-13 11:27:35]

+sdefresne for the new dependency of the components on version_info.
+asargent as OWNER of chrome_update_query_params_delegate.cc

https://codereview.chromium.org/1281913002/diff/80001/components/component_up...
File components/component_updater/configurator_impl.h (right):

https://codereview.chromium.org/1281913002/diff/80001/components/component_up...
components/component_updater/configurator_impl.h:12: #include "base/version.h"
On 2015/08/12 17:16:55, Sorin Jianu wrote:
> On 2015/08/12 08:43:41, droger wrote:
> > On 2015/08/12 02:27:51, Sorin Jianu wrote:
> > > technically, a forward decl for base::Version is sufficient but not a big
> > deal.
> > 
> > base::Version is used as a return type on line 60, so I added this include
to
> > conform to the IWYU rule.
> > Is this reasoning wrong?
> 
> The idea is that types that are used for parameters and return types in
function
> declarations can be forward declared:
> http://stackoverflow.com/questions/553682/when-can-i-use-a-forward-declaration

Ok, done.

https://codereview.chromium.org/1281913002/diff/80001/components/update_clien...
File components/update_client/update_query_params_delegate.h (right):

https://codereview.chromium.org/1281913002/diff/80001/components/update_clien...
components/update_client/update_query_params_delegate.h:27: // Returns the value
we use for the "updaterchannel=" and "prodchannel="
On 2015/08/12 17:16:55, Sorin Jianu wrote:
> On 2015/08/12 08:43:41, droger wrote:
> > On 2015/08/12 02:27:51, Sorin Jianu wrote:
> > > This addition is suboptimal. UpdateQueryParamsDelegate used to be a pure
> > > interface, now it has a static member. I agree that the design of the
class
> is
> > > debatable (has a ctor) and refactoring this is not in scope.
> > 
> > I can move this function elsewhere if you prefer.
> > It can also be a free function (not in a class).
> > 
> > Just let me know.
> 
> Free function would be better imho.

I moved the function to UpdateQueryParams, it seems like a good fit.

[sdefresne, 2015-08-13 12:20:01]

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
File components/update_client/configurator.h (right):

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
components/update_client/configurator.h:13: #include "base/version.h"
Why adding this #include, the class is already forward-declared below? If the
#include is really required, at least remove the forward-declaration.

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
File components/update_client/update_query_params.cc (right):

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
components/update_client/update_query_params.cc:136: const char*
UpdateQueryParams::GetChannelString(version_info::Channel channel) {
This is duplicating version_info::GetChannelString(). Please use the function
from the version_info component instead. Or document why it is required not to
use version_info::GetChannelString().

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
File components/update_client/update_query_params.h (right):

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
components/update_client/update_query_params.h:58: static const char*
GetChannelString(version_info::Channel channel);
version_info::Channel is a "enum class" and thus can be forward-declared, please
do so and only include the header in the implementation file.

namespace version_info {
enum class Channel;
}

https://codereview.chromium.org/1281913002/diff/120001/ios/chrome/browser/com...
File ios/chrome/browser/component_updater/ios_component_updater_configurator.h
(right):

https://codereview.chromium.org/1281913002/diff/120001/ios/chrome/browser/com...
ios/chrome/browser/component_updater/ios_component_updater_configurator.h:21:
scoped_refptr<update_client::Configurator> MakeIOSComponentUpdaterConfigurator(
nit, optional: to avoid the ambiguous name, you could also have called this
method MakeComponentUpdaterConfigurator in the global namespace (since it is in
an app, according to the style guide it should not use namespace).

[droger, 2015-08-13 13:14:18]

Patchset #6 (id:140001) has been deleted

[droger, 2015-08-13 13:16:02]

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
File components/update_client/configurator.h (right):

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
components/update_client/configurator.h:13: #include "base/version.h"
On 2015/08/13 12:20:00, sdefresne wrote:
> Why adding this #include, the class is already forward-declared below? If the
> #include is really required, at least remove the forward-declaration.

Oops sorry, this is a mistake.

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
File components/update_client/update_query_params.h (right):

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
components/update_client/update_query_params.h:58: static const char*
GetChannelString(version_info::Channel channel);
On 2015/08/13 12:20:00, sdefresne wrote:
> version_info::Channel is a "enum class" and thus can be forward-declared,
please
> do so and only include the header in the implementation file.
> 
> namespace version_info {
> enum class Channel;
> }

Done, using version_info instead, which actually simplifies the code a lot.
sorin: was there a good reason to have duplication here?

[droger, 2015-08-13 13:17:47]

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
File components/update_client/update_query_params.cc (right):

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
components/update_client/update_query_params.cc:136: const char*
UpdateQueryParams::GetChannelString(version_info::Channel channel) {
On 2015/08/13 12:20:00, sdefresne wrote:
> This is duplicating version_info::GetChannelString(). Please use the function
> from the version_info component instead. Or document why it is required not to
> use version_info::GetChannelString().

Done, using version_info instead, which actually simplifies the code a lot.
sorin: was there a good reason to have duplication here?

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
File components/update_client/update_query_params.h (right):

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
components/update_client/update_query_params.h:58: static const char*
GetChannelString(version_info::Channel channel);
On 2015/08/13 13:16:02, droger wrote:
> On 2015/08/13 12:20:00, sdefresne wrote:
> > version_info::Channel is a "enum class" and thus can be forward-declared,
> please
> > do so and only include the header in the implementation file.
> > 
> > namespace version_info {
> > enum class Channel;
> > }
> 
> Done, using version_info instead, which actually simplifies the code a lot.
> sorin: was there a good reason to have duplication here?

Sorry this was meant as a reply to another comment. I have replied at the right
place now.

[Sorin Jianu, 2015-08-13 18:24:11]

lgtm

Thank you!
I hope I did not miss anything due to overlapping comments.

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
File components/update_client/update_query_params.cc (right):

https://codereview.chromium.org/1281913002/diff/120001/components/update_clie...
components/update_client/update_query_params.cc:136: const char*
UpdateQueryParams::GetChannelString(version_info::Channel channel) {
On 2015/08/13 13:17:46, droger wrote:
> On 2015/08/13 12:20:00, sdefresne wrote:
> > This is duplicating version_info::GetChannelString(). Please use the
function
> > from the version_info component instead. Or document why it is required not
to
> > use version_info::GetChannelString().
> 
> Done, using version_info instead, which actually simplifies the code a lot.
> sorin: was there a good reason to have duplication here?

It appears that sdefresne@ componentized version_info recently (
https://codereview.chromium.org/1257633002) and for some reason, the duplicated
code in chrome_update_query_params_delegate.cc was not removed. At any rate,
this is excellent refactoring, we could eliminate the duplicated code.

Somehow related, the whole thing about query parameters and query parameters
delegate could be redone, but that is outside the scope of this change, and a
task for the owners of the respective modules.

[droger, 2015-08-17 08:20:05]

asargent, sdefresne: ping

[sdefresne, 2015-08-17 08:51:47]

lgtm for the new dependency on //components/version_info

[droger, 2015-08-18 12:34:48]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2015-08-18 12:35:04]

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

[commit-bot: I haz the power, 2015-08-18 12:41:54]

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

[commit-bot: I haz the power, 2015-08-18 12:41:55]

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

[droger, 2015-08-18 13:08:28]

droger@chromium.org changed reviewers:
+ cpu@chromium.org

[droger, 2015-08-18 13:08:29]

+cpu as OWNER of chrome/browser/update_client

[asargent_no_longer_on_chrome, 2015-08-18 17:21:26]

Sorry! I somehow missed this CL while out on leave last week. Taking a look now.

[Sorin Jianu, 2015-08-18 17:38:13]

As a general idea, cpu@ is onto different things, but asargent@ will grant the
owner's approval for this change.

[asargent_no_longer_on_chrome, 2015-08-18 17:54:25]

https://codereview.chromium.org/1281913002/diff/180001/components/component_u...
File components/component_updater/component_updater_url_constants.cc (right):

https://codereview.chromium.org/1281913002/diff/180001/components/component_u...
components/component_updater/component_updater_url_constants.cc:18: 
Does it really make sense for these google-specific URLs to be in the
components/ namespace? What if some other product built on top of chromium
source (eg opera, etc.) wanted to use component updater code to do something
similar, but use their own URLs? 

I would have expected to see these constants stay defined somewhere in chrome/
and then some interface with virtual methods defined in components/, and a
concrete implementation those methods returning these specific urls in chrome.
Had this kind of approach already been considered and rejected for some reason?

[blundell, 2015-08-18 18:02:38]

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

[blundell, 2015-08-18 18:02:38]

https://codereview.chromium.org/1281913002/diff/180001/components/component_u...
File components/component_updater/component_updater_url_constants.cc (right):

https://codereview.chromium.org/1281913002/diff/180001/components/component_u...
components/component_updater/component_updater_url_constants.cc:18: 
On 2015/08/18 17:54:25, Antony Sargent wrote:
> Does it really make sense for these google-specific URLs to be in the
> components/ namespace? What if some other product built on top of chromium
> source (eg opera, etc.) wanted to use component updater code to do something
> similar, but use their own URLs? 
> 
> I would have expected to see these constants stay defined somewhere in chrome/
> and then some interface with virtual methods defined in components/, and a
> concrete implementation those methods returning these specific urls in chrome.
> Had this kind of approach already been considered and rejected for some
reason?
> 
> 

We have discussed this kind of issue in the past, and here have been the
conclusions:

- The component is being created for the iOS port of Chrome to share code with
the other ports.
- As such, it is OK for the component to include Google-specific code, strings,
constants, or whatnot.
- If some other embedder later comes along and wants to share this code but with
s/Google/<foo>, the code can easily be generalized at that point.

TL;DR: The component is generalized only to the level that generalization is
currently required.

[asargent_no_longer_on_chrome, 2015-08-18 19:35:17]

On 2015/08/18 18:02:38, blundell wrote:
>
https://codereview.chromium.org/1281913002/diff/180001/components/component_u...
> File components/component_updater/component_updater_url_constants.cc (right):
> 
>
https://codereview.chromium.org/1281913002/diff/180001/components/component_u...
> components/component_updater/component_updater_url_constants.cc:18: 
> On 2015/08/18 17:54:25, Antony Sargent wrote:
> > Does it really make sense for these google-specific URLs to be in the
> > components/ namespace? What if some other product built on top of chromium
> > source (eg opera, etc.) wanted to use component updater code to do something
> > similar, but use their own URLs? 
> > 
> > I would have expected to see these constants stay defined somewhere in
chrome/
> > and then some interface with virtual methods defined in components/, and a
> > concrete implementation those methods returning these specific urls in
chrome.
> > Had this kind of approach already been considered and rejected for some
> reason?
> > 
> > 
> 
> We have discussed this kind of issue in the past, and here have been the
> conclusions:
> 
> - The component is being created for the iOS port of Chrome to share code with
> the other ports.
> - As such, it is OK for the component to include Google-specific code,
strings,
> constants, or whatnot.
> - If some other embedder later comes along and wants to share this code but
with
> s/Google/<foo>, the code can easily be generalized at that point.
> 
> TL;DR: The component is generalized only to the level that generalization is
> currently required.

Ok. I do think that where it's convenient while we're modifying something, it
would be nice to shoot for generalization, but I'll trust your judgement about
whether that's worth it in this case. 

lgtm

[droger, 2015-08-19 07:43:23]

On 2015/08/18 19:35:17, Antony Sargent wrote:
> On 2015/08/18 18:02:38, blundell wrote:
> >
>
https://codereview.chromium.org/1281913002/diff/180001/components/component_u...
> > File components/component_updater/component_updater_url_constants.cc
(right):
> > 
> >
>
https://codereview.chromium.org/1281913002/diff/180001/components/component_u...
> > components/component_updater/component_updater_url_constants.cc:18: 
> > On 2015/08/18 17:54:25, Antony Sargent wrote:
> > > Does it really make sense for these google-specific URLs to be in the
> > > components/ namespace? What if some other product built on top of chromium
> > > source (eg opera, etc.) wanted to use component updater code to do
something
> > > similar, but use their own URLs? 
> > > 
> > > I would have expected to see these constants stay defined somewhere in
> chrome/
> > > and then some interface with virtual methods defined in components/, and a
> > > concrete implementation those methods returning these specific urls in
> chrome.
> > > Had this kind of approach already been considered and rejected for some
> > reason?
> > > 
> > > 
> > 
> > We have discussed this kind of issue in the past, and here have been the
> > conclusions:
> > 
> > - The component is being created for the iOS port of Chrome to share code
with
> > the other ports.
> > - As such, it is OK for the component to include Google-specific code,
> strings,
> > constants, or whatnot.
> > - If some other embedder later comes along and wants to share this code but
> with
> > s/Google/<foo>, the code can easily be generalized at that point.
> > 
> > TL;DR: The component is generalized only to the level that generalization is
> > currently required.
> 
> Ok. I do think that where it's convenient while we're modifying something, it
> would be nice to shoot for generalization, but I'll trust your judgement about
> whether that's worth it in this case. 
> 
> lgtm

I agree with Colin here, lets generalize later if needed, this CL is already
very large.

[droger, 2015-08-19 07:43:34]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2015-08-19 07:43:44]

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

[droger, 2015-08-19 07:43:46]

Thanks you all for the reviews.

[commit-bot: I haz the power, 2015-08-19 07:48:46]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2015-08-19 07:49:29]

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