Differential updates for components.

chrome/browser/component_updater/component_updater_configurator.cc

Index: chrome/browser/component_updater/component_updater_configurator.cc
diff --git a/chrome/browser/component_updater/component_updater_configurator.cc b/chrome/browser/component_updater/component_updater_configurator.cc
index 16fa3e9619752730b02326dba994faf4c14aef10..fe6c4ca3d5bbd950b68cb9618cc117056cfe634d 100644
--- a/chrome/browser/component_updater/component_updater_configurator.cc
+++ b/chrome/browser/component_updater/component_updater_configurator.cc
@@ -14,11 +14,17 @@
 #include "base/strings/string_util.h"
 #include "base/win/windows_version.h"
 #include "build/build_config.h"
+#include "chrome/browser/component_updater/component_patcher.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/omaha_query_params/omaha_query_params.h"
 #include "net/url_request/url_request_context_getter.h"
 
+#if defined(OS_WIN)
+#include "chrome/browser/component_updater/component_patcher_win.h"
+#endif
+
 namespace {
+
 // Default time constants.
 const int kDelayOneMinute = 60;
 const int kDelayOneHour = kDelayOneMinute * 60;
@@ -35,9 +41,13 @@ const char kDebugRequestParam[] = "test-request";
 const char* kUrlSources[] = {
   "http://clients2.google.com/service/update2/crx",       // BANDAID
   "http://omaha.google.com/service/update2/crx",          // CWS_PUBLIC
-  "http://omaha.sandbox.google.com/service/update2/crx"   // CWS_SANDBOX
+  "http://omaha.sandbox.google.com/service/update2/crx",   // CWS_SANDBOX
 };
 
+// The url to send the completion pings to. Completion pings are sent when
+// updates have completed, either successfully or with errors.
+const char kUrlPing[] = "http://tools.google.com/service/update2";
+
 bool HasDebugValue(const std::vector<std::string>& vec, const char* test) {
   if (vec.empty())
     return 0;
@@ -59,24 +69,32 @@ class ChromeConfigurator : public ComponentUpdateService::Configurator {
   virtual int MinimumReCheckWait() OVERRIDE;
   virtual int OnDemandDelay() OVERRIDE;
   virtual GURL UpdateUrl(CrxComponent::UrlSource source) OVERRIDE;
+  virtual GURL PingUrl() OVERRIDE;
   virtual const char* ExtraRequestParams() OVERRIDE;
   virtual size_t UrlSizeLimit() OVERRIDE;
   virtual net::URLRequestContextGetter* RequestContext() OVERRIDE;
   virtual bool InProcess() OVERRIDE;
   virtual void OnEvent(Events event, int val) OVERRIDE;
+  virtual ComponentPatcher* CreateComponentPatcher() OVERRIDE;
+  virtual bool PingsEnabled() const OVERRIDE;

[cpu_(ooo_6.6-7.5), 2013/06/17 20:27:01]

I feel we have duplicated state here, I propose that PingEnabled and PingUrl
both can be made into one, if PingUrl returns a valid url then pings are
enabled.

[waffles, 2013/06/17 23:51:07]

Done.

+  virtual bool DeltasEnabled() const OVERRIDE;
 
  private:
   net::URLRequestContextGetter* url_request_getter_;
   std::string extra_info_;
   bool fast_update_;
   bool out_of_process_;
+  bool pings_enabled_;
+  bool deltas_enabled_;
 };
 
 ChromeConfigurator::ChromeConfigurator(const CommandLine* cmdline,
     net::URLRequestContextGetter* url_request_getter)
       : url_request_getter_(url_request_getter),
         extra_info_(chrome::OmahaQueryParams::Get(
-            chrome::OmahaQueryParams::CHROME)) {
+            chrome::OmahaQueryParams::CHROME)),
+        pings_enabled_(false),

[robertshield, 2013/06/17 17:47:00]

don't need these in the initializer list if they are unconditionally initialized
below.

[Sorin Jianu, 2013/06/17 22:21:09]

We prefer to initialize any member in all cases even they get assign to later
on. Is that ok with you? I can change it but we are more afraid of the
initialization bugs and Heisenbugs when code changes later on than the runtime
cost of defensively initializing everything.

[robertshield, 2013/06/18 13:27:07]

Sounds good. In that case, please initialize fast_update_ and out_of_process_ to
false as well.

[Sorin Jianu, 2013/06/18 17:00:11]

Done.

+        deltas_enabled_(false) {
   // Parse comma-delimited debug flags.
   std::vector<std::string> debug_values;
   Tokenize(cmdline->GetSwitchValueASCII(switches::kComponentUpdaterDebug),
@@ -84,6 +102,14 @@ ChromeConfigurator::ChromeConfigurator(const CommandLine* cmdline,
   fast_update_ = HasDebugValue(debug_values, kDebugFastUpdate);
   out_of_process_ = HasDebugValue(debug_values, kDebugOutOfProcess);
 
+  // Parse other command-ling flags.
+  pings_enabled_ = cmdline->HasSwitch(switches::kEnableComponentUpdatePings);
+#if defined(OS_WIN)
+  deltas_enabled_ = cmdline->HasSwitch(switches::kEnableComponentUpdateDeltas);

[cpu_(ooo_6.6-7.5), 2013/06/17 20:27:01]

if deltas enabled are an option, it does not seem necessary to have #if win here

[Sorin Jianu, 2013/06/17 22:21:09]

People could turn on deltas on platforms that we don't support at the moment if
we remove the #if. Do we have a concern with that?

+#else
+  deltas_enabled_ = false;
+#endif
+
   // Make the extra request params, they are necessary so omaha does
   // not deliver components that are going to be rejected at install time.
 #if defined(OS_WIN)
@@ -119,6 +145,10 @@ GURL ChromeConfigurator::UpdateUrl(CrxComponent::UrlSource source) {
   return GURL(kUrlSources[source]);
 }
 
+GURL ChromeConfigurator::PingUrl() {
+  return GURL(kUrlPing);
+}
+
 const char* ChromeConfigurator::ExtraRequestParams() {
   return extra_info_.c_str();
 }
@@ -161,6 +191,22 @@ void ChromeConfigurator::OnEvent(Events event, int val) {
   }
 }
 
+ComponentPatcher* ChromeConfigurator::CreateComponentPatcher() {
+#if defined(OS_WIN)
+  return new ComponentPatcherWin();

[cpu_(ooo_6.6-7.5), 2013/06/17 20:27:01]

this DI is nice but I do wonder if the configurator has the best information to
decide which patcher strategy to use. I can argue it both ways, maybe chrome
knows better, maybe the component updater knows better.

I guess for testing this is a convenient place, if we really like it here I
wonder if we should add a cmdline swith to force the crossplatform one on
windows.

[Sorin Jianu, 2013/06/17 22:21:09]

Right on, I needed this primarily for DI and this was the easier to hack it in.
It is the service locator pattern. WE hope that next iteration gets comes up
with an implementation of the cross-platform patcher and this code goes away
altogether.

+#else
+  return new ComponentPatcherCrossPlatform();
+#endif
+}
+
+bool ChromeConfigurator::DeltasEnabled() const {
+  return deltas_enabled_;
+}
+
+bool ChromeConfigurator::PingsEnabled() const {
+  return pings_enabled_;
+}
+
 ComponentUpdateService::Configurator* MakeChromeComponentUpdaterConfigurator(
     const CommandLine* cmdline, net::URLRequestContextGetter* context_getter) {
   return new ChromeConfigurator(cmdline, context_getter);