Differential updates for components.

chrome/browser/component_updater/component_updater_configurator.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/component_updater/component_updater_configurator.h"
 
 #include <algorithm>
 #include <string>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/metrics/histogram.h"
 #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;
 
 // Debug values you can pass to --component-updater-debug=value1,value2.
 // Speed up component checking.
 const char kDebugFastUpdate[] = "fast-update";
 // Force out-of-process-xml parsing.
 const char kDebugOutOfProcess[] = "out-of-process";
 // Add "testrequest=1" parameter to the update check query.
 const char kDebugRequestParam[] = "test-request";
 
 // The urls from which an update manifest can be fetched.
 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;
   return (std::find(vec.begin(), vec.end(), test) != vec.end());
 }
 
 }  // namespace
 
 class ChromeConfigurator : public ComponentUpdateService::Configurator {
  public:
   ChromeConfigurator(const CommandLine* cmdline,
                      net::URLRequestContextGetter* url_request_getter);
 
   virtual ~ChromeConfigurator() {}
 
   virtual int InitialDelay() OVERRIDE;
   virtual int NextCheckDelay() OVERRIDE;
   virtual int StepDelay() OVERRIDE;
   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),
       ",", &debug_values);
   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)
   if (base::win::OSInfo::GetInstance()->wow64_status() ==
       base::win::OSInfo::WOW64_ENABLED)
     extra_info_ += "&wow64=1";
 #endif
   if (HasDebugValue(debug_values, kDebugRequestParam))
     extra_info_ += "&testrequest=1";
 }
@@ skipping 15 matching lines @@
 }
 
 int ChromeConfigurator::OnDemandDelay() {
   return fast_update_ ? 2 : (30 * kDelayOneMinute);
 }
 
 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();
 }
 
 size_t ChromeConfigurator::UrlSizeLimit() {
   return 1024ul;
 }
 
 net::URLRequestContextGetter* ChromeConfigurator::RequestContext() {
   return url_request_getter_;
@@ skipping 22 matching lines @@
       break;
     case kInstallerError:
       UMA_HISTOGRAM_ENUMERATION("ComponentUpdater.InstallError", val, 100);
       break;
     default:
       NOTREACHED();
       break;
   }
 }
 
+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);
 }