Implement support for fallback update check urls in the component updater

components/component_updater/component_updater_ping_manager.cc

Index: components/component_updater/component_updater_ping_manager.cc
diff --git a/components/component_updater/component_updater_ping_manager.cc b/components/component_updater/component_updater_ping_manager.cc
index b07b1b4989ab01df65eb74a0d9cda1f931b8029f..148234a38a5929a09e8e4ea14b6ff9f6b3325404 100644
--- a/components/component_updater/component_updater_ping_manager.cc
+++ b/components/component_updater/component_updater_ping_manager.cc
@@ -5,23 +5,26 @@
 #include "components/component_updater/component_updater_ping_manager.h"
 
 #include <string>
+#include <vector>
 
 #include "base/compiler_specific.h"
-#include "base/guid.h"
+#include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "components/component_updater/component_updater_configurator.h"
 #include "components/component_updater/component_updater_utils.h"
 #include "components/component_updater/crx_update_item.h"
-#include "net/url_request/url_fetcher.h"
-#include "net/url_request/url_fetcher_delegate.h"
+#include "components/component_updater/request_sender.h"
 #include "url/gurl.h"
 
 namespace component_updater {
 
+namespace {
+
 // Returns a string literal corresponding to the value of the downloader |d|.
 const char* DownloaderToString(CrxDownloader::DownloadMetrics::Downloader d) {
   switch (d) {
@@ -34,86 +37,9 @@ const char* DownloaderToString(CrxDownloader::DownloadMetrics::Downloader d) {
   }
 }
 
-// Sends a fire and forget ping. The instances of this class have no
-// ownership and they self-delete upon completion.
-class PingSender : public net::URLFetcherDelegate {
- public:
-  PingSender();
-
-  void SendPing(const Configurator& config,
-                net::URLRequestContextGetter* url_request_context_getter,
-                const CrxUpdateItem* item);
-
- private:
-  virtual ~PingSender();
-
-  // Overrides for URLFetcherDelegate.
-  virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;
-
-  static std::string BuildPing(const Configurator& config,
-                               const CrxUpdateItem* item);
-  static std::string BuildDownloadCompleteEventElements(
-      const CrxUpdateItem* item);
-  static std::string BuildUpdateCompleteEventElement(const CrxUpdateItem* item);
-
-  scoped_ptr<net::URLFetcher> url_fetcher_;
-
-  DISALLOW_COPY_AND_ASSIGN(PingSender);
-};
-
-PingSender::PingSender() {
-}
-
-PingSender::~PingSender() {
-}
-
-void PingSender::OnURLFetchComplete(const net::URLFetcher* source) {
-  delete this;
-}
-
-void PingSender::SendPing(
-    const Configurator& config,
-    net::URLRequestContextGetter* url_request_context_getter,
-    const CrxUpdateItem* item) {
-  DCHECK(item);
-
-  if (!config.PingUrl().is_valid())
-    return;
-
-  url_fetcher_.reset(SendProtocolRequest(config.PingUrl(),
-                                         BuildPing(config, item),
-                                         this,
-                                         url_request_context_getter));
-}
-
-// Builds a ping message for the specified update item.
-std::string PingSender::BuildPing(const Configurator& config,
-                                  const CrxUpdateItem* item) {
-  const char app_element_format[] =
-      "<app appid=\"%s\" version=\"%s\" nextversion=\"%s\">"
-      "%s"
-      "%s"
-      "</app>";
-  const std::string app_element(base::StringPrintf(
-      app_element_format,
-      item->id.c_str(),                                    // "appid"
-      item->previous_version.GetString().c_str(),          // "version"
-      item->next_version.GetString().c_str(),              // "nextversion"
-      BuildUpdateCompleteEventElement(item).c_str(),       // update event
-      BuildDownloadCompleteEventElements(item).c_str()));  // download events
-
-  return BuildProtocolRequest(config.GetBrowserVersion().GetString(),
-                              config.GetChannel(),
-                              config.GetLang(),
-                              config.GetOSLongName(),
-                              app_element,
-                              "");
-}
-
 // Returns a string representing a sequence of download complete events
 // corresponding to each download metrics in |item|.
-std::string PingSender::BuildDownloadCompleteEventElements(
-    const CrxUpdateItem* item) {
+std::string BuildDownloadCompleteEventElements(const CrxUpdateItem* item) {
   using base::StringAppendF;
   std::string download_events;
   for (size_t i = 0; i != item->download_metrics.size(); ++i) {
@@ -153,8 +79,7 @@ std::string PingSender::BuildDownloadCompleteEventElements(
 }
 
 // Returns a string representing one ping event xml element for an update item.
-std::string PingSender::BuildUpdateCompleteEventElement(
-    const CrxUpdateItem* item) {
+std::string BuildUpdateCompleteEventElement(const CrxUpdateItem* item) {
   DCHECK(item->status == CrxUpdateItem::kNoUpdate ||
          item->status == CrxUpdateItem::kUpdated);
 
@@ -189,6 +114,80 @@ std::string PingSender::BuildUpdateCompleteEventElement(
   return ping_event;
 }
 
+// Builds a ping message for the specified update item.
+std::string BuildPing(const Configurator& config, const CrxUpdateItem* item) {
+  const char app_element_format[] =
+      "<app appid=\"%s\" version=\"%s\" nextversion=\"%s\">"
+      "%s"
+      "%s"
+      "</app>";
+  const std::string app_element(base::StringPrintf(
+      app_element_format,
+      item->id.c_str(),                                    // "appid"
+      item->previous_version.GetString().c_str(),          // "version"
+      item->next_version.GetString().c_str(),              // "nextversion"
+      BuildUpdateCompleteEventElement(item).c_str(),       // update event
+      BuildDownloadCompleteEventElements(item).c_str()));  // download events
+
+  return BuildProtocolRequest(config.GetBrowserVersion().GetString(),
+                              config.GetChannel(),
+                              config.GetLang(),
+                              config.GetOSLongName(),
+                              app_element,
+                              "");
+}
+
+// Sends a fire and forget ping. The instances of this class have no
+// ownership and they self-delete upon completion. One instance of this class
+// can send only one ping.
+class PingSender {
+ public:
+  explicit PingSender(const Configurator& config);
+  ~PingSender();
+
+  bool SendPing(const CrxUpdateItem* item);
+
+ private:
+  void OnRequestSenderComplete(const net::URLFetcher* source);

[erikwright (departed), 2014/09/15 18:37:11]

forward-decl required?

[Sorin Jianu, 2014/09/15 22:17:57]

This function is part of the public interface for the RequestSender, whose
header is included fully.

[erikwright (departed), 2014/09/16 17:38:11]

Sure, but the only case where you are not required to IWYU would be if the type
only appeared in the declaration of an OVERRIDE of a virtual parent class
method.

In this case, indirectly, we know that this must already be forward-decl'd
because it corresponds to a callback type referenced in a class that is fully
defined and used.

In most cases we can rationalize that a type must already be fully defined, but
then we have to decide if, by definition, the usage of the type would disappear
if the rational later becomes invalid. To make things simpler, we don't do this
rationalization and instead use very simple rules.

Please do the forward-decl or include depending on whether this compilation unit
requires a full or partial type.

[Sorin Jianu, 2014/09/16 19:53:31]

Thank you. I declared the forward type.

+
+  const Configurator& config_;
+  scoped_ptr<RequestSender> request_sender_;
+  base::ThreadChecker thread_checker_;

[erikwright (departed), 2014/09/15 18:37:11]

missing include?

[Sorin Jianu, 2014/09/15 22:17:57]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(PingSender);
+};
+
+PingSender::PingSender(const Configurator& config) : config_(config) {
+}
+
+PingSender::~PingSender() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
+
+void PingSender::OnRequestSenderComplete(const net::URLFetcher* source) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  delete this;
+}
+
+bool PingSender::SendPing(const CrxUpdateItem* item) {
+  DCHECK(item);
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  std::vector<GURL> urls(config_.PingUrl());
+
+  if (urls.empty())
+    return false;
+
+  request_sender_.reset(new RequestSender(config_));
+  request_sender_->Send(
+      BuildPing(config_, item),
+      urls,
+      base::Bind(&PingSender::OnRequestSenderComplete, base::Unretained(this)));

[erikwright (departed), 2014/09/15 18:37:11]

base/bind.h and base/bind_helpers.h required

[Sorin Jianu, 2014/09/15 22:17:57]

Done.

+  return true;
+}
+
+}  // namespace
+
 PingManager::PingManager(const Configurator& config) : config_(config) {
 }
 
@@ -196,10 +195,11 @@ PingManager::~PingManager() {
 }
 
 // Sends a fire and forget ping when the updates are complete. The ping
-// sender object self-deletes after sending the ping.
+// sender object self-deletes after sending the ping has completed asynchrously.
 void PingManager::OnUpdateComplete(const CrxUpdateItem* item) {
-  PingSender* ping_sender(new PingSender);
-  ping_sender->SendPing(config_, config_.RequestContext(), item);
+  PingSender* ping_sender(new PingSender(config_));
+  if (!ping_sender->SendPing(item))
+    delete ping_sender;
 }
 
 }  // namespace component_updater