Differential updates for components.

chrome/browser/component_updater/component_updater_service.cc

Index: chrome/browser/component_updater/component_updater_service.cc
diff --git a/chrome/browser/component_updater/component_updater_service.cc b/chrome/browser/component_updater/component_updater_service.cc
index f3eee77c204791f46acb13e813f98b5b3b1f8a6b..b142f1d7ea0d4db9d5c764cf938416fc09f78446 100644
--- a/chrome/browser/component_updater/component_updater_service.cc
+++ b/chrome/browser/component_updater/component_updater_service.cc
@@ -10,8 +10,10 @@
 
 #include "base/at_exit.h"
 #include "base/bind.h"
+#include "base/compiler_specific.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
+#include "base/guid.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
@@ -19,13 +21,16 @@
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/sys_info.h"
 #include "base/timer.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/component_updater/component_patcher.h"
 #include "chrome/browser/component_updater/component_unpacker.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_utility_messages.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/omaha_query_params/omaha_query_params.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/utility_process_host.h"
@@ -47,21 +52,26 @@ using extensions::Extension;
 // base::Bind() calls are not refcounted.
 
 namespace {
+
+typedef ComponentUpdateService::Configurator Config;

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

This typedef appears to be used only in a couple of places below (whereas the
original type is used more often). I'd suggest getting rid of the typedef, since
it doesn't greatly enhance readability. If you are attached to it, please use it
everywhere. 

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

Done.

+
 // Manifest sources, from most important to least important.
 const CrxComponent::UrlSource kManifestSources[] = {
   CrxComponent::BANDAID,
   CrxComponent::CWS_PUBLIC,
-  CrxComponent::CWS_SANDBOX
+  CrxComponent::CWS_SANDBOX,
 };
 
 // Extends an omaha compatible update check url |query| string. Does
 // not mutate the string if it would be longer than |limit| chars.
 bool AddQueryString(const std::string& id,
                     const std::string& version,
+                    const std::string& fingerprint,
                     size_t limit,
                     std::string* query) {
   std::string additional =
-      base::StringPrintf("id=%s&v=%s&uc", id.c_str(), version.c_str());
+      base::StringPrintf("id=%s&v=%s&fp=%s&uc",
+                         id.c_str(), version.c_str(), fingerprint.c_str());
   additional = "x=" + net::EscapeQueryParamValue(additional, true);
   if ((additional.size() + query->size() + 1) > limit)
     return false;
@@ -163,34 +173,81 @@ void StartFetch(net::URLFetcher* fetcher,
   fetcher->Start();
 }
 
-// Returs true if the url request of |fetcher| was succesful.
+// Returns true if the url request of |fetcher| was succesful.
 bool FetchSuccess(const net::URLFetcher& fetcher) {
   return (fetcher.GetStatus().status() == net::URLRequestStatus::SUCCESS) &&
          (fetcher.GetResponseCode() == 200);
 }
 
+// Returns the error code which occured during the fetch. This function
+// is intended to be called for logging purposes only, since it folds different
+// types of errors to fit them in the returned type. The function returns 0 if
+// the fetch was successful. If errors happen, the function could return a
+// network error, an http response code, or the status of the fetch, if the
+// fetch is pending or canceled.
+int GetFetchError(const net::URLFetcher& fetcher) {
+  if (FetchSuccess(fetcher))
+    return 0;
+
+  const net::URLRequestStatus::Status status(fetcher.GetStatus().status());
+  if (status == net::URLRequestStatus::FAILED)
+    return fetcher.GetStatus().error();
+
+  if (status == net::URLRequestStatus::IO_PENDING ||
+      status == net::URLRequestStatus::CANCELED)
+    return status;
+
+  const int response_code(fetcher.GetResponseCode());
+  if (status == net::URLRequestStatus::SUCCESS && response_code != 200)
+    return response_code;
+
+  return -1;
+}
+
+
 // This is the one and only per-item state structure. Designed to be hosted
 // in a std::vector or a std::list. The two main members are |component|
 // which is supplied by the the component updater client and |status| which
 // is modified as the item is processed by the update pipeline. The expected
 // transition graph is:
-//                                error          error         error
-//              +--kNoUpdate<------<-------+------<------+------<------+
-//              |                          |             |             |
-//              V                  yes     |             |             |
-//  kNew --->kChecking-->[update?]----->kCanUpdate-->kDownloading-->kUpdating
-//              ^               |                                      |
-//              |               |no                                    |
-//              |--kUpToDate<---+                                      |
-//              |                                  success             |
-//              +--kUpdated<-------------------------------------------+
+//
+//                                 kNew
+//                                  |
+//                                  V
+//     +----------------------> kChecking -<---------+-----<-------+
+//     |                            |                |             |
+//     |                            V       no       |             |
+//  kNoUpdate                   [update?] ->---- kUpToDate     kUpdated
+//     ^                            |                              ^
+//     |                        yes |                              |
+//     |        diff=false          V                              |
+//     |          +-----------> kCanUpdate                         |
+//     |          |                 |                              |
+//     |          |                 V              no              |
+//     |          |        [differential update?]->----+           |
+//     |          |                 |                  |           |
+//     |          |             yes |                  |           |
+//     |          |   error         V                  |           |
+//     |          +---------<- kDownloadingDiff        |           |
+//     |          |                 |                  |           |
+//     |          |                 |                  |           |
+//     |          |   error         V                  |           |
+//     |          +---------<- kUpdatingDiff ->--------|-----------+ success
+//     |                                               |           |
+//     |              error                            V           |
+//     +----------------------------------------- kDownloading     |
+//     |                                               |           |
+//     |              error                            V           |
+//     +------------------------------------------ kUpdating ->----+ success
 //
 struct CrxUpdateItem {
   enum Status {
     kNew,
     kChecking,
     kCanUpdate,
+    kDownloadingDiff,
     kDownloading,
+    kUpdatingDiff,
     kUpdating,
     kUpdated,
     kUpToDate,
@@ -199,13 +256,55 @@ struct CrxUpdateItem {
   };
 
   Status status;
-  GURL crx_url;
   std::string id;
-  base::Time last_check;
   CrxComponent component;

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

In here we differ in philosophy. As you can see I took pains to minimize the per
item state, the new CL makes the per item state everything that you can possibly
care about.

[Sorin Jianu, 2013/06/18 20:46:39]

I am sorry if we are breaking the existing design. CrxUpdateItem is more
stateful since we need to remember some more state information regarding to
delta updates. We are also introducing completion pings which include some of
the state information. 

In general, what is the concern that made you minimize the per item state
maintained in the CrxUpdateItem? I thought that this was the purpose of the
CrxUpdateItem: hold the update state of the object as it runs through the state
machine driven by the UpdateService.

We have agreed that more state is needed and the state has to go somewhere. I am
open to discuss this, we want to do what is right.

-  Version next_version;
 
-  CrxUpdateItem() : status(kNew) {}
+  base::Time last_check;
+
+  // True if the update response includes an update for this component.
+  bool is_update_available;
+
+  // True is a completion ping has been queued for this component. If an update

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

"True is"?

[Sorin Jianu, 2013/06/18 20:46:39]

Done.

+  // is available for this component, one completion ping must be sent
+  // after the component has reached either the kNoUpdate or kUpdated states.
+  bool ping_queued;

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

maybe I got it wrong but you don't need this. If it CanPing() and it reached one
of the states, a ping will be made.

[Sorin Jianu, 2013/06/18 20:46:39]

We removed the queuing of pings

+
+  // These members are initialized with their corresponding values from the
+  // update server response.
+  GURL crx_url;
+  GURL diff_crx_url;
+  std::string package_hash;
+  std::string diff_package_hash;
+  int size;
+  int diff_size;

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

why do we need these sizes here? I failed to find where they are used. Also why
are they int? what does it mean to have a negative size?

[Sorin Jianu, 2013/06/18 20:46:39]

These are values that we parsed from the update response. In Omaha we use these
values for both validating the file integrity and for sending back quality of
service data.

We parse these values as ints. It does not mean sizes can be negative but we do
have a limit of how large the components can be. Is there a portable type in
Chromium for file sizes? What do you recommend?

+
+  // The from/to version and fingerprint values.
+  Version previous_version;
+  Version next_version;
+  std::string previous_fp;
+  std::string next_fp;
+
+  // True if the differential update failed for any reason.
+  bool diff_update_failed;
+
+  // The error information for full and differential updates.
+  int error_code;
+  int extra_code1;
+  int diff_error_code;
+  int diff_extra_code1;

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

sigh

[Sorin Jianu, 2013/06/18 20:46:39]

I understand. The challenge we have here is that the amount of data we want to
return is more than what an integer can fit, so there has be be a way to hold
more error state, unless we don't want to return extended error information.
Which is so important to understand why things break in production.

+
+  CrxUpdateItem()
+      : status(kNew),
+        is_update_available(false),
+        ping_queued(false),
+        size(0),
+        diff_size(0),
+        diff_update_failed(false),
+        error_code(0),
+        extra_code1(0),
+        diff_error_code(0),
+        diff_extra_code1(0) {
+  }
 
   // Function object used to find a specific component.
   class FindById {
@@ -220,9 +319,20 @@ struct CrxUpdateItem {
   };
 };
 
-}  // namespace.
+// Returns true if a differential update is available for the update item.
+bool IsDiffUpdateAvailable(const CrxUpdateItem* update_item) {
+  return update_item->diff_crx_url.is_valid();
+}
 
-typedef ComponentUpdateService::Configurator Config;
+// Returns true if a differential update is available, it has not failed yet,
+// and the configuration allows it.
+bool CanTryDiffUpdate(const CrxUpdateItem* update_item, const Config& config) {
+  return IsDiffUpdateAvailable(update_item) &&
+         !update_item->diff_update_failed &&
+         config.DeltasEnabled();
+}
+
+}  // namespace.
 
 CrxComponent::CrxComponent()
     : installer(NULL),
@@ -240,7 +350,7 @@ CrxComponent::~CrxComponent() {
 // rest of the browser, so even if we have many components registered and
 // eligible for update, we only do one thing at a time with pauses in between
 // the tasks. Also when we do network requests there is only one |url_fetcher_|
-// in flight at at a time.
+// in flight at a time.
 // There are no locks in this code, the main structure |work_items_| is mutated
 // only from the UI thread. The unpack and installation is done in the file
 // thread and the network requests are done in the IO thread and in the file
@@ -299,11 +409,18 @@ class CrxUpdateService : public ComponentUpdateService {
     UpdateContext() : start(base::Time::Now()) {}
   };
 
+  // Context for a completion ping. See DelegateWithContext above. The type
+  // is needed for template type deduction and it could be used in the
+  // future to pass arguments to the callback.
+  struct PingContext {
+  };
+
   // Context for a crx download url request. See DelegateWithContext above.
   struct CRXContext {
     ComponentInstaller* installer;
     std::vector<uint8> pk_hash;
     std::string id;
+    std::string fingerprint;
     CRXContext() : installer(NULL) {}
   };
 
@@ -313,14 +430,16 @@ class CrxUpdateService : public ComponentUpdateService {
   void OnURLFetchComplete(const net::URLFetcher* source,
                           CRXContext* context);
 
+  void OnURLFetchComplete(const net::URLFetcher* source,
+                          PingContext* context);
+
  private:
   // See ManifestParserBridge.
   void OnParseUpdateManifestSucceeded(
       const UpdateManifest::Results& results);
 
   // See ManifestParserBridge.
-  void OnParseUpdateManifestFailed(
-      const std::string& error_message);
+  void OnParseUpdateManifestFailed(const std::string& error_message);
 
   bool AddItemToUpdateCheck(CrxUpdateItem* item, std::string* query);
 
@@ -333,19 +452,36 @@ class CrxUpdateService : public ComponentUpdateService {
   void Install(const CRXContext* context, const base::FilePath& crx_path);
 
   void DoneInstalling(const std::string& component_id,
-                      ComponentUnpacker::Error error);
+                      ComponentUnpacker::Error error,
+                      int extended_error);
 
   size_t ChangeItemStatus(CrxUpdateItem::Status from,
                           CrxUpdateItem::Status to);
 
   CrxUpdateItem* FindUpdateItemById(const std::string& id);
 
+  // Returns true if completion pings are enabled.
+  bool CanPing() const;
+
+  void SendPing(const std::string& ping);
+
+  // These functions build a ping request. Building the ping changes the state
+  // of the update item to indicate that a completion ping has been collected
+  // and queued for this item.
+  std::string BuildPing() const;
+  std::string BuildPingApps() const;
+  static std::string BuildPingEvent(CrxUpdateItem* item);
+
   scoped_ptr<Config> config_;
 
+  scoped_ptr<ComponentPatcher> component_patcher_;
+
   scoped_ptr<net::URLFetcher> url_fetcher_;
 
-  typedef std::vector<CrxUpdateItem*> UpdateItems;
+  scoped_ptr<net::URLFetcher> ping_sender_;
+
   // A collection of every work item.
+  typedef std::vector<CrxUpdateItem*> UpdateItems;
   UpdateItems work_items_;
 
   // A particular set of items from work_items_, which should be checked ASAP.
@@ -353,7 +489,10 @@ class CrxUpdateService : public ComponentUpdateService {
 
   base::OneShotTimer<CrxUpdateService> timer_;
 
-  Version chrome_version_;
+  const Version chrome_version_;
+  const std::string prod_id_;
+  const std::string os_type_;
+  const std::string os_version_;
 
   bool running_;
 
@@ -362,10 +501,14 @@ class CrxUpdateService : public ComponentUpdateService {
 
 //////////////////////////////////////////////////////////////////////////////
 
-CrxUpdateService::CrxUpdateService(
-    ComponentUpdateService::Configurator* config)
+CrxUpdateService::CrxUpdateService(ComponentUpdateService::Configurator* config)
     : config_(config),
+      component_patcher_(config->CreateComponentPatcher()),
       chrome_version_(chrome::VersionInfo().Version()),
+      prod_id_(chrome::OmahaQueryParams::GetProdIdString(
+          chrome::OmahaQueryParams::CHROME)),
+      os_type_(chrome::VersionInfo().OSType()),

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

is this expensive to query, enough that we want to have in the object state?

You will find that one of my fixations is to minimize object state.

I am guessing that a lot of the new state is going to go somewhere once we
settle the item o line 986

[Sorin Jianu, 2013/06/18 20:46:39]

I agree that the state has to go somewhere or it has to be queried as needed.

Querying these members is not expensive, at least for |prod_id_| and |os_type_|.
My thinking is that since these members are used together, they are maintained
together as immutable members in the service.

+      os_version_(base::SysInfo().OperatingSystemVersion()),
       running_(false) {
 }
 
@@ -418,6 +561,12 @@ void CrxUpdateService::ScheduleNextRun(bool step_delay) {
   if (!running_)
     return;
 
+  if (!step_delay && CanPing()) {
+    const std::string ping(BuildPing());
+    if (!ping.empty())
+      SendPing(ping);
+  }
+
   // Keep the delay short if in the middle of an update (step_delay),
   // or there are new requested_work_items_ that have not been processed yet.
   int64 delay = (step_delay || requested_work_items_.size() > 0)
@@ -491,6 +640,7 @@ ComponentUpdateService::Status CrxUpdateService::RegisterComponent(
   uit = new CrxUpdateItem;
   uit->id.swap(id);
   uit->component = component;
+
   work_items_.push_back(uit);
   // If this is the first component registered we call Start to
   // schedule the first timer.
@@ -501,17 +651,30 @@ ComponentUpdateService::Status CrxUpdateService::RegisterComponent(
 }
 
 // Sets a component to be checked for updates.
-// The componet to add is |crxit| and the |query| string is modified with the
+// The component to add is |crxit| and the |query| string is modified with the
 // required omaha compatible query. Returns false when the query strings
 // is longer than specified by UrlSizeLimit().
 bool CrxUpdateService::AddItemToUpdateCheck(CrxUpdateItem* item,
                                             std::string* query) {
   if (!AddQueryString(item->id,
                       item->component.version.GetString(),
+                      item->component.fingerprint,
                       config_->UrlSizeLimit(), query))
     return false;
+
   item->status = CrxUpdateItem::kChecking;
   item->last_check = base::Time::Now();
+  item->is_update_available = false;
+  item->ping_queued = false;
+  item->previous_version = item->component.version;
+  item->next_version = Version();
+  item->previous_fp = item->component.fingerprint;
+  item->next_fp = "";
+  item->diff_update_failed = false;
+  item->error_code = 0;
+  item->extra_code1 = 0;
+  item->diff_error_code = 0;
+  item->diff_extra_code1 = 0;
   return true;
 }
 
@@ -535,16 +698,17 @@ ComponentUpdateService::Status CrxUpdateService::CheckForUpdateSoon(
 
   // Check if the request is too soon.
   base::TimeDelta delta = base::Time::Now() - uit->last_check;
-  if (delta < base::TimeDelta::FromSeconds(config_->OnDemandDelay())) {
+  if (delta < base::TimeDelta::FromSeconds(config_->OnDemandDelay()))
     return kError;
-  }
 
   switch (uit->status) {
     // If the item is already in the process of being updated, there is
     // no point in this call, so return kInProgress.
     case CrxUpdateItem::kChecking:
     case CrxUpdateItem::kCanUpdate:
+    case CrxUpdateItem::kDownloadingDiff:
     case CrxUpdateItem::kDownloading:
+    case CrxUpdateItem::kUpdatingDiff:
     case CrxUpdateItem::kUpdating:
       return kInProgress;
     // Otherwise the item was already checked a while back (or it is new),
@@ -583,13 +747,21 @@ void CrxUpdateService::ProcessPendingItems() {
     if (item->status != CrxUpdateItem::kCanUpdate)
       continue;
     // Found component to update, start the process.
-    item->status = CrxUpdateItem::kDownloading;
     CRXContext* context = new CRXContext;
     context->pk_hash = item->component.pk_hash;
     context->id = item->id;
     context->installer = item->component.installer;
+    context->fingerprint = item->next_fp;
+    GURL package_url;
+    if (CanTryDiffUpdate(item, *config_)) {
+      package_url = item->diff_crx_url;
+      item->status = CrxUpdateItem::kDownloadingDiff;
+    } else {
+      package_url = item->crx_url;
+      item->status = CrxUpdateItem::kDownloading;
+    }
     url_fetcher_.reset(net::URLFetcher::Create(
-        0, item->crx_url, net::URLFetcher::GET,
+        0, package_url, net::URLFetcher::GET,
         MakeContextDelegate(this, context)));
     StartFetch(url_fetcher_.get(), config_->RequestContext(), true);
     return;
@@ -700,11 +872,10 @@ void CrxUpdateService::ParseManifest(const std::string& xml) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (config_->InProcess()) {
     UpdateManifest manifest;
-    if (!manifest.Parse(xml)) {
+    if (!manifest.Parse(xml))
        CrxUpdateService::OnParseUpdateManifestFailed(manifest.errors());
-    } else {
+    else
        CrxUpdateService::OnParseUpdateManifestSucceeded(manifest.results());
-    }
   } else {
     UtilityProcessHost* host =
         UtilityProcessHost::Create(new ManifestParserBridge(this),
@@ -752,9 +923,16 @@ void CrxUpdateService::OnParseUpdateManifestSucceeded(
     }
     // All test passed. Queue an upgrade for this component and fire the
     // notifications.
+    crx->is_update_available = true;
     crx->crx_url = it->crx_url;
+    crx->package_hash = it->package_hash;
+    crx->size = it->size;
+    crx->diff_crx_url = it->diff_crx_url;
+    crx->diff_package_hash = it->diff_package_hash;
+    crx->diff_size = it->diff_size;
     crx->status = CrxUpdateItem::kCanUpdate;
     crx->next_version = Version(it->version);
+    crx->next_fp = it->package_fingerprint;
     ++update_pending;
 
     content::NotificationService::current()->Notify(
@@ -789,19 +967,31 @@ void CrxUpdateService::OnURLFetchComplete(const net::URLFetcher* source,
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   int error_code = net::OK;
 
+  CrxUpdateItem* crx = FindUpdateItemById(context->id);
+
   if (source->FileErrorOccurred(&error_code) || !FetchSuccess(*source)) {
-    size_t count = ChangeItemStatus(CrxUpdateItem::kDownloading,
-                                    CrxUpdateItem::kNoUpdate);
-    DCHECK_EQ(count, 1ul);
+    if (crx->status == CrxUpdateItem::kDownloadingDiff) {
+      crx->diff_error_code = GetFetchError(*source);
+      crx->diff_update_failed = true;
+      crx->status = CrxUpdateItem::kCanUpdate;
+      BrowserThread::PostTask(
+          BrowserThread::UI,
+          FROM_HERE,
+          base::Bind(&CrxUpdateService::ProcessPendingItems,
+                     base::Unretained(this)));
+      return;
+    }
+    crx->error_code = GetFetchError(*source);
+    crx->status = CrxUpdateItem::kNoUpdate;

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

Use ChangeItemStatus

[Sorin Jianu, 2013/06/18 20:46:39]

ChangeItemStatus changes the state of all items (from, to). We only want to
change the state for this item.

[cpu_(ooo_6.6-7.5), 2013/06/18 22:02:36]

Correct. The way it is used like in line 802 is because there can be only one
component in kDownloading, this is important for the operation of the component
updater of having only one component in flight. It is sort of a self discipline
to use when it can be used.

[Sorin Jianu, 2013/06/19 00:29:10]

Done by using ChangeItemStatus throughout this function.

     config_->OnEvent(Configurator::kNetworkError, CrxIdtoUMAId(context->id));

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

and here we get to the fun part, as you can see there is an existing error
reporting framework. I don't see a compelling reason to have two of them.

The choice of moving the error reporting out of the core and into the
configurator was intentional. How errors are processed and reported is not
'core' to this code.

I am willing to let go of UMA, even though that is how chrome manages reporting
and use your reporting on the view that is omaha folks monitoring it, but in
turn I want all of it out into the configurator. 

Ideally the only code you will see here is config_=>OnEvent(....).

[Sorin Jianu, 2013/06/18 20:46:39]

Let's discuss this and see what is reasonable to do. Let's see what we want to
do and what we agree on so far:
* we want to send completion pings for all users to monitor the quality of the
update service
* we agree that Omaha will do this and we can do outside UMA, just as we do it
for Chrome updates.
* we agree that the error handling and reporting should be decoupled from the
rest of the code.

We are happy to follow your advice and move the error handling and the pinging
logic out in the configurator. The state that needs to be sent to the server
must be encapsulated in some object. Right now that object is the CrxUpdateItem.
At this time, I see three options:
1. Introduce a dependency in ChromeConfigurator on the CrxUpdateItem so that an
item can be passed as an argument to the OnEvent.
2. Pass the state information in some other kind of object which becomes an
argument to OnEvent.
2. Pass the state information as standalone parameters to OnEvent, as in:
config_->OnEvent(..., id, diff_error_code, error_code, version,
previous_version,....)
Which way would you recommend we go? 

[cpu_(ooo_6.6-7.5), 2013/06/18 22:18:25]

Neat to have agreement here.

I like 2a or 2b. If it was me I would try 2b and see how it looks, I would try
that first because then in the configurator I can maintain my own vector of
UpdateItemStatus or whatever name. 

Basically lets nuke the existing UMA reporting and lets do your with the least
coupling that we can do.

I would also be interesting to have this pinging decoupled from the update
scheduler, like say only once every 8 hours or so bulk reporting of the whole
thing.

[Sorin Jianu, 2013/06/19 00:29:10]

I am mailing the comments and uploading the fixes I have so far except this one,
which I am working on it at the moment.

     url_fetcher_.reset();
     ScheduleNextRun(false);
   } else {
     base::FilePath temp_crx_path;
     CHECK(source->GetResponseAsFilePath(true, &temp_crx_path));
-    size_t count = ChangeItemStatus(CrxUpdateItem::kDownloading,
-                                    CrxUpdateItem::kUpdating);
-    DCHECK_EQ(count, 1ul);
+    crx->status = (crx->status == CrxUpdateItem::kDownloadingDiff) ?
+        CrxUpdateItem::kUpdatingDiff : CrxUpdateItem::kUpdating;
+
     url_fetcher_.reset();
 
     content::NotificationService::current()->Notify(
@@ -829,17 +1019,19 @@ void CrxUpdateService::Install(const CRXContext* context,
                                const base::FilePath& crx_path) {
   // This function owns the |crx_path| and the |context| object.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  ComponentUnpacker
-      unpacker(context->pk_hash, crx_path, context->installer);
-  if (!file_util::Delete(crx_path, false)) {
+  ComponentUnpacker unpacker(context->pk_hash,
+                             crx_path,
+                             context->fingerprint,
+                             component_patcher_.get(),
+                             context->installer);
+  if (!file_util::Delete(crx_path, false))
     NOTREACHED() << crx_path.value();
-  }
   // Why unretained? See comment at top of file.
   BrowserThread::PostDelayedTask(
       BrowserThread::UI,
       FROM_HERE,
       base::Bind(&CrxUpdateService::DoneInstalling, base::Unretained(this),
-                 context->id, unpacker.error()),
+                 context->id, unpacker.error(), unpacker.extended_error()),
       base::TimeDelta::FromMilliseconds(config_->StepDelay()));
   delete context;
 }
@@ -847,14 +1039,37 @@ void CrxUpdateService::Install(const CRXContext* context,
 // Installation has been completed. Adjust the component status and
 // schedule the next check.
 void CrxUpdateService::DoneInstalling(const std::string& component_id,
-                                      ComponentUnpacker::Error error) {
+                                      ComponentUnpacker::Error error,
+                                      int extra_code) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   CrxUpdateItem* item = FindUpdateItemById(component_id);
+  if (item->status == CrxUpdateItem::kUpdatingDiff) {
+    if (error != ComponentUnpacker::kNone) {
+      item->diff_error_code = error;
+      item->diff_extra_code1 = extra_code;
+      item->diff_update_failed = true;
+      item->status = CrxUpdateItem::kCanUpdate;
+      BrowserThread::PostTask(
+          BrowserThread::UI,
+          FROM_HERE,
+          base::Bind(&CrxUpdateService::ProcessPendingItems,
+                     base::Unretained(this)));
+      return;
+    }
+  }
+
+  if (error != ComponentUnpacker::kNone) {
+    item->error_code = error;
+    item->extra_code1 = extra_code;
+  }
+
   item->status = (error == ComponentUnpacker::kNone) ? CrxUpdateItem::kUpdated :
                                                        CrxUpdateItem::kNoUpdate;
-  if (item->status == CrxUpdateItem::kUpdated)
+  if (item->status == CrxUpdateItem::kUpdated) {
     item->component.version = item->next_version;
+    item->component.fingerprint = item->next_fp;
+  }
 
   Configurator::Events event;
   switch (error) {
@@ -873,6 +1088,112 @@ void CrxUpdateService::DoneInstalling(const std::string& component_id,
   ScheduleNextRun(false);
 }
 
+bool CrxUpdateService::CanPing() const {
+  return config_->PingUrl().is_valid();
+}
+
+void CrxUpdateService::SendPing(const std::string& ping) {
+  VLOG(3) << "Sending ping. " << ping;

[cpu_(ooo_6.6-7.5), 2013/06/18 17:02:06]

please remove all the VLOGing, instrument your local build if you want to printf
debug.

[Sorin Jianu, 2013/06/18 20:46:39]

Done.

+  const std::string ping_url(config_->PingUrl().spec());
+  ping_sender_.reset(net::URLFetcher::Create(
+      0, GURL(ping_url), net::URLFetcher::POST,
+      MakeContextDelegate(this, new PingContext())));
+  ping_sender_->SetUploadData(std::string("application/xml"), ping);
+  StartFetch(ping_sender_.get(), config_->RequestContext(), false);
+}
+
+void CrxUpdateService::OnURLFetchComplete(const net::URLFetcher* source,
+                                          PingContext* context) {
+  VLOG(3) << "Sending component update ping returned "
+          << FetchSuccess(*source);
+  ping_sender_.reset();
+}
+
+// Builds a ping message for the update items that have completed. Returns
+// an empty string if there are no completed items.
+std::string CrxUpdateService::BuildPing() const {
+  const std::string apps(BuildPingApps());
+  if (apps.empty())
+    return std::string();
+
+  const char response_format[] =
+      "<o:gupdate xmlns:o=\"http://www.google.com/update2/request\" "
+      "protocol=\"2.0\" version=\"%s-%s\" requestid=\"{%s}\"> "
+      "<o:os platform=\"%s\" version=\"%s\"/> "
+      "%s"
+      "</o:gupdate>";
+  const std::string response_envelope(
+      base::StringPrintf(response_format,
+                         prod_id_.c_str(),
+                         chrome_version_.GetString().c_str(),
+                         base::GenerateGUID().c_str(),
+                         os_type_.c_str(),
+                         os_version_.c_str(),
+                         apps.c_str()));
+  return response_envelope;
+}
+
+// Returns a string containing the sequence of app elements inside the
+// ping message.
+std::string CrxUpdateService::BuildPingApps() const {
+  const char app_format[] = "<o:app appid=\"%s\" version=\"%s\">%s</o:app>";
+
+  std::string ping_apps;
+  for (UpdateItems::const_iterator it = work_items_.begin();
+       it != work_items_.end(); ++it) {
+    CrxUpdateItem* item = *it;
+    // Only ping once if an update was available and the item is completed.
+    if (item->is_update_available && !item->ping_queued &&
+        (item->status == CrxUpdateItem::kNoUpdate ||
+         item->status == CrxUpdateItem::kUpdated)) {
+      const std::string version = item->component.version.GetString().c_str();
+      ping_apps += base::StringPrintf(app_format,
+                                      item->id.c_str(),
+                                      version.c_str(),
+                                      BuildPingEvent(item).c_str());
+    }
+  }
+  return ping_apps;
+}
+
+// Returns a string representing one ping event for an update item.
+std::string CrxUpdateService::BuildPingEvent(CrxUpdateItem* item) {
+  DCHECK(item->status == CrxUpdateItem::kNoUpdate ||
+         item->status == CrxUpdateItem::kUpdated);
+  DCHECK(item->is_update_available);
+
+  using base::StringAppendF;
+
+  std::string ping_event("<o:event eventtype=\"3\"");
+  const int event_result = item->status == CrxUpdateItem::kUpdated;
+  StringAppendF(&ping_event, " eventresult=\"%d\"", event_result);
+  StringAppendF(&ping_event, " previousversion=\"%s\"",
+                item->previous_version.GetString().c_str());
+  StringAppendF(&ping_event, " nextversion=\"%s\"",
+                item->next_version.GetString().c_str());
+  if (item->error_code)
+    StringAppendF(&ping_event, " errorcode=\"%d\"", item->error_code);
+  if (item->extra_code1)
+    StringAppendF(&ping_event, " extracode1=\"%d\"", item->extra_code1);
+  if (IsDiffUpdateAvailable(item))
+    StringAppendF(&ping_event, " diffresult=\"%d\"", !item->diff_update_failed);
+  if (item->diff_error_code)
+    StringAppendF(&ping_event, " differrorcode=\"%d\"", item->diff_error_code);
+  if (item->diff_extra_code1) {
+    StringAppendF(&ping_event,
+                  " diffextracode1=\"%d\"",
+                  item->diff_extra_code1);
+  }
+  if (!item->previous_fp.empty())
+    StringAppendF(&ping_event, " previousfp=\"%s\"", item->previous_fp.c_str());
+  if (!item->next_fp.empty())
+    StringAppendF(&ping_event, " nextfp=\"%s\"", item->next_fp.c_str());
+  StringAppendF(&ping_event, "/>");
+  item->ping_queued = true;
+  return ping_event;
+}
+
+
 // The component update factory. Using the component updater as a singleton
 // is the job of the browser process.
 ComponentUpdateService* ComponentUpdateServiceFactory(