Componentize component_updater: Use Configurator to build query parameters.

Componentize component_updater: Use Configurator to build query parameters.

This also breaks a few more misc. bonds between chrome/common and component_updater.

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

[tommycli, 2014-07-08 23:34:53]

sorin/colin,

PTAL

I used the Configurator interface to break some more chrome/common dependencies
in the component updater.

Not quite ready to move a bunch of files wholesale over to
component/component_updater directory yet, but we're getting close.

Thanks!

[blundell, 2014-07-09 08:04:00]

I'm not convinced it makes sense to introduce a PPAPI dependency in the core
component_updater component. Not all platforms use PPAPI (e.g., iOS), and while
ppapi.gyp doesn't seem to have any problematic dependencies now, presumably it
could have some at some point (e.g., ipc).

I propose leaving the pepper flash-related code in
//chrome/browser/component_updater at the current time and waiting until such a
time as some embedder other than Chrome wants that functionality. Sorin, WDYT?

[Sorin Jianu, 2014-07-09 08:29:16]

Thank you Tommy.

I have concerns about the layering of dependencies on the Configurator in the
component_updater_utils. 

I would prefer to have those utils not depend on the Configurator, but take
additional parameters for the attributes that are controlled by the embedder.
Second, we'd like to keep building of XML in one module and avoid formatting
tags in other modules. Why? Because we usually spend time trying to figure out
why certain  request parameters are missing when users say "My component is not
updating".

https://codereview.chromium.org/375973003/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

https://codereview.chromium.org/375973003/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_configurator.cc:236:
std::string ChromeConfigurator::RequestOSTag() const {
I'll say up front I don't know of a good design here. That being said, the
current implementation couples the serialization of XML parameters too tightly
with the configurator, here and above. We'd like to consolidate the xml
serialization in one place, so that we can see and test what is being
serialized. Therefore, I would prefer a design where the configurator provides
low level ambient parameters instead of pre-formatted XML string. Would it be
possible to have something like this?

https://codereview.chromium.org/375973003/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_configurator.h (right):

https://codereview.chromium.org/375973003/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_configurator.h:10: #include
"base/version.h"
A forward decl for base::Version would be sufficient (the declaration of A f();
does not need the definition of A) , please try to see if it compiles.

https://codereview.chromium.org/375973003/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_utils.h (right):

https://codereview.chromium.org/375973003/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_utils.h:48: std::string
BuildProtocolRequest(const Configurator& config,
I would prefer a design where these low level formatting utilities don't take a
dependency of the configurator, instead, they take the parameters needed to
build the request. These utilities are called by objects that need to send
network requests, such as PingManager/PingSender, and the UpdateChecker. These
objects can take a dependency on the configurator, retrieve the ambient
parameters needed (including the new ones), then build the request by formatting
the tags.

[Sorin Jianu, 2014-07-09 08:34:08]

re PPAPI dependency question: My thinking is that the less code we move around,
the better. Therefore, I support the idea of PPAPI to remain as-is for the
moment, if that is possible.

[tommycli, 2014-07-09 19:48:11]

PTAL

sorin: Made the Configurator just provide the arguments and kept the XML
generation inside tho component_updater_utils.

blundell: Reverted the pepper flash changes.

Thanks guys!

https://codereview.chromium.org/375973003/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

https://codereview.chromium.org/375973003/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_configurator.cc:236:
std::string ChromeConfigurator::RequestOSTag() const {
On 2014/07/09 08:29:16, Sorin Jianu wrote:
> I'll say up front I don't know of a good design here. That being said, the
> current implementation couples the serialization of XML parameters too tightly
> with the configurator, here and above. We'd like to consolidate the xml
> serialization in one place, so that we can see and test what is being
> serialized. Therefore, I would prefer a design where the configurator provides
> low level ambient parameters instead of pre-formatted XML string. Would it be
> possible to have something like this?

Done.

https://codereview.chromium.org/375973003/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_configurator.h (right):

https://codereview.chromium.org/375973003/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_configurator.h:10: #include
"base/version.h"
On 2014/07/09 08:29:16, Sorin Jianu wrote:
> A forward decl for base::Version would be sufficient (the declaration of A
f();
> does not need the definition of A) , please try to see if it compiles.

Done. Had no idea that this was possible. I guess you learn something new
everyday.

https://codereview.chromium.org/375973003/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_utils.h (right):

https://codereview.chromium.org/375973003/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_utils.h:48: std::string
BuildProtocolRequest(const Configurator& config,
On 2014/07/09 08:29:16, Sorin Jianu wrote:
> I would prefer a design where these low level formatting utilities don't take
a
> dependency of the configurator, instead, they take the parameters needed to
> build the request. These utilities are called by objects that need to send
> network requests, such as PingManager/PingSender, and the UpdateChecker. These
> objects can take a dependency on the configurator, retrieve the ambient
> parameters needed (including the new ones), then build the request by
formatting
> the tags.

Done.

[blundell, 2014-07-10 08:38:10]

Structure LGTM, thanks! I'll defer to Sorin for the detailed review.

[Sorin Jianu, 2014-07-10 12:27:29]

Thank you Tommy. Small fry below.

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
chrome/browser/component_updater/component_updater_configurator.cc:197: 
extra empty line

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_configurator.h (right):

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
chrome/browser/component_updater/component_updater_configurator.h:57: // Version
of the application. Used to compare to component manifests.
s/to/the ?

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
chrome/browser/component_updater/component_updater_configurator.h:57: // Version
of the application. Used to compare to component manifests.
Is the ApplicationVersion actually the Chrome browser version? If yes, maybe we
could be more specific here.

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_utils.cc (right):

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
chrome/browser/component_updater/component_updater_utils.cc:42: std::string
BuildProtocolRequest(const Configurator& config,
I was suggesting to not take the dependency on Configurator here. Instead, the
caller of this function breaks down the config and provides the arguments to
this function at the call site. This is a small deal but I am just hoping to
avoid the Configurator spilling into every single module of the component
updater. I don't have a strong opinion about this, maybe waffles@ has one.

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
chrome/browser/component_updater/component_updater_utils.cc:63:
app_version.c_str(),               // "version"
I suggesting keeping the old naming. "app version" usually means the "version"
found in the "app" element.

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
chrome/browser/component_updater/component_updater_utils.cc:67:
config.GetChannelString().c_str(), // "updaterchannel"
we could also keep the local variable "channel", and maybe the "lang" too.

[tommycli, 2014-07-10 16:08:14]

sorin: I changed the BuildProtocolRequest method to take individual arguments.
thanks!

blundell: thx!

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
chrome/browser/component_updater/component_updater_configurator.cc:197: 
On 2014/07/10 12:27:28, Sorin Jianu wrote:
> extra empty line

Done.

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_configurator.h (right):

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
chrome/browser/component_updater/component_updater_configurator.h:57: // Version
of the application. Used to compare to component manifests.
On 2014/07/10 12:27:28, Sorin Jianu wrote:
> s/to/the ?

Done.

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
chrome/browser/component_updater/component_updater_configurator.h:57: // Version
of the application. Used to compare to component manifests.
On 2014/07/10 12:27:28, Sorin Jianu wrote:
> Is the ApplicationVersion actually the Chrome browser version? If yes, maybe
we
> could be more specific here.

Done.

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_utils.cc (right):

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
chrome/browser/component_updater/component_updater_utils.cc:42: std::string
BuildProtocolRequest(const Configurator& config,
On 2014/07/10 12:27:28, Sorin Jianu wrote:
> I was suggesting to not take the dependency on Configurator here. Instead, the
> caller of this function breaks down the config and provides the arguments to
> this function at the call site. This is a small deal but I am just hoping to
> avoid the Configurator spilling into every single module of the component
> updater. I don't have a strong opinion about this, maybe waffles@ has one.

Done.

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
chrome/browser/component_updater/component_updater_utils.cc:63:
app_version.c_str(),               // "version"
On 2014/07/10 12:27:28, Sorin Jianu wrote:
> I suggesting keeping the old naming. "app version" usually means the "version"
> found in the "app" element.

Done. Changed to browser_version since it's technically not supposed to be
'chrome' specific.

https://codereview.chromium.org/375973003/diff/210017/chrome/browser/componen...
chrome/browser/component_updater/component_updater_utils.cc:67:
config.GetChannelString().c_str(), // "updaterchannel"
On 2014/07/10 12:27:28, Sorin Jianu wrote:
> we could also keep the local variable "channel", and maybe the "lang" too.

Done.

[Sorin Jianu, 2014-07-10 16:25:23]

lgtm

Thank you Tommy! Please ping waffles@ for a review as well.

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

https://codereview.chromium.org/375973003/diff/230001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_ping_manager.cc:105:
config.GetChannel(), config.GetLang(),
small nit, maybe align vertically the arguments, since in this case they appear
to run together. Here and at one more call site.

[Sorin Jianu, 2014-07-10 16:25:25]

lgtm

Thank you Tommy! Please ping waffles@ for a review as well.

[tommycli, 2014-07-10 17:46:36]

waffles: May I have a review? Thanks

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

https://codereview.chromium.org/375973003/diff/230001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_ping_manager.cc:105:
config.GetChannel(), config.GetLang(),
On 2014/07/10 16:25:23, Sorin Jianu wrote:
> small nit, maybe align vertically the arguments, since in this case they
appear
> to run together. Here and at one more call site.

Done.

[waffles, 2014-07-10 18:12:32]

lgtm

[tommycli, 2014-07-10 19:05:48]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-07-10 19:06:47]

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

[commit-bot: I haz the power, 2014-07-10 22:47:43]

Change committed as 282444