[Sync] Rip out overly-complicated ExtensionData class

chrome/browser/sync/glue/extension_sync.cc

 // Copyright (c) 2011 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/sync/glue/extension_sync.h"
 
 #include <utility>
 
 #include "base/logging.h"
-#include "chrome/browser/extensions/extension_updater.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_sync_data.h"
-#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/engine/syncapi.h"
-#include "chrome/browser/sync/glue/extension_data.h"
 #include "chrome/browser/sync/glue/extension_sync_traits.h"
 #include "chrome/browser/sync/glue/extension_util.h"
-#include "chrome/browser/sync/profile_sync_service.h"
+#include "chrome/browser/sync/protocol/extension_specifics.pb.h"
 
 namespace browser_sync {
 
 bool RootNodeHasChildren(const char* tag,
                          sync_api::UserShare* user_share,
                          bool* has_children) {
   CHECK(has_children);
   *has_children = false;
   sync_api::ReadTransaction trans(user_share);
   sync_api::ReadNode node(&trans);
   if (!node.InitByTagLookup(tag)) {
     LOG(ERROR) << "Root node with tag " << tag << " does not exist";
     return false;
   }
   *has_children = node.GetFirstChildId() != sync_api::kInvalidId;
   return true;
 }
 
 namespace {
 
-// Updates the value in |extension_data_map| from the given data,
-// creating an entry if necessary.  Returns a pointer to the
-// updated/created ExtensionData object.
-ExtensionData* SetOrCreateExtensionData(
-    ExtensionDataMap* extension_data_map,
-    ExtensionData::Source source,
-    bool merge_user_properties,
-    const sync_pb::ExtensionSpecifics& data) {
-  DcheckIsExtensionSpecificsValid(data);
-  const std::string& extension_id = data.id();
-  std::pair<ExtensionDataMap::iterator, bool> result =
-      extension_data_map->insert(
-          std::make_pair(extension_id,
-                         ExtensionData::FromData(source, data)));
-  ExtensionData* extension_data = &result.first->second;
-  if (result.second) {
-    // The value was just inserted, so it shouldn't need an update
-    // from source.
-    DCHECK(!extension_data->NeedsUpdate(source));
-  } else {
-    extension_data->SetData(source, merge_user_properties, data);
-  }
-  return extension_data;
-}
-
 // Fills in |extension_data_map| with data from
 // extension_service.GetSyncDataList().
 void SlurpClientData(
     IsValidAndSyncablePredicate is_valid_and_syncable,
     const ExtensionServiceInterface& extension_service,
     ExtensionDataMap* extension_data_map) {
   std::vector<ExtensionSyncData> sync_data_list =
       extension_service.GetSyncDataList(is_valid_and_syncable);
   for (std::vector<ExtensionSyncData>::const_iterator it =
            sync_data_list.begin();
        it != sync_data_list.end(); ++it) {
-    sync_pb::ExtensionSpecifics client_specifics;
-    SyncDataToSpecifics(*it, &client_specifics);
-    const ExtensionData& extension_data =
-        *SetOrCreateExtensionData(
-            extension_data_map, ExtensionData::CLIENT,
-            true, client_specifics);
-    DcheckIsExtensionSpecificsValid(extension_data.merged_data());
+    std::pair<ExtensionDataMap::iterator, bool> result =
+        extension_data_map->insert(std::make_pair(it->id, *it));
+    if (!result.second) {
+      // The value wasn't inserted, so merge it in.
+      result.first->second.Merge(*it);

[asargent_no_longer_on_chrome, 2011/04/27 16:45:19]

It's not really clear to me why you need this Merge method. Why don't you just
do something like:

ExtensionDataMap::iterator existing = extension_data_map->find(it->id);
if (existing != extension_data_map->end())
  extension_data_map->erase(it->id);
extension_data_map.insert(std::make_pair(it->id, *it));

?

[akalin, 2011/04/27 17:53:25]

(The above is equivalent to "(*extension_data_map)[it->id] = *it;")

It's possible for a client to have an old version of an extension when they set
up sync.  In that case, we want the sync data to keep the version-specific data
for the newer version.  It doesn't matter much now (since the version-specific
data is just version, name and update_url) but it gives us the nice behavior of
being able to nudge the auto-updater even if there are no new extensions to
download, but just outdated ones.

I plan to add more version-specific properties, too.  For example,
has_npapi_plugin.  It should be a version-specific property since an extension
can conceivably not have one for early versions but have one for later.  If we
sync that property, we can sync extension data for all extensions and clients
can simply not auto-update those with has_npapi set.  If has_npapi isn't set but
the download extension does end up having an npapi plugin (maybe it got
updated), then the ShouldInstallPredicate would still catch that, but the
has_npapi flag lets clients avoid downloading the extension only to find out it
shouldn't be installed.  So the merge behavior would save some work in that
case.

Does that make sense?

+    }
   }
 }
 
 // Gets the boilerplate error message for not being able to find a
 // root node.
 //
 // TODO(akalin): Put this somewhere where all data types can use it.
 std::string GetRootNodeDoesNotExistError(const char* root_node_tag) {
   return
       std::string("Server did not create the top-level ") +
       root_node_tag +
       " node. We might be running against an out-of-date server.";
 }
 
 // Gets the data from the server for extensions to be synced and
-// updates |extension_data_map|.  Skips all extensions in
-// |unsynced_extensions|.
+// updates |extension_data_map|.
 bool SlurpServerData(
     const char* root_node_tag,
     const ExtensionSpecificsGetter extension_specifics_getter,
     sync_api::UserShare* user_share,
     ExtensionDataMap* extension_data_map) {
   sync_api::WriteTransaction trans(user_share);
   sync_api::ReadNode root(&trans);
   if (!root.InitByTagLookup(root_node_tag)) {
     LOG(ERROR) << GetRootNodeDoesNotExistError(root_node_tag);
     return false;
   }
 
   int64 id = root.GetFirstChildId();
   while (id != sync_api::kInvalidId) {
     sync_api::ReadNode sync_node(&trans);
     if (!sync_node.InitByIdLookup(id)) {
       LOG(ERROR) << "Failed to fetch sync node for id " << id;
       return false;
     }
     const sync_pb::ExtensionSpecifics& server_data =
         (*extension_specifics_getter)(sync_node);
-    if (!IsExtensionSpecificsValid(server_data)) {
+    ExtensionSyncData sync_data;
+    if (!SpecificsToSyncData(server_data, &sync_data)) {
       LOG(ERROR) << "Invalid extensions specifics for id " << id;
       return false;
     }
-    // Pass in false for merge_user_properties so client user
-    // settings always take precedence.
-    const ExtensionData& extension_data =
-        *SetOrCreateExtensionData(
-            extension_data_map, ExtensionData::SERVER, false, server_data);
-    DcheckIsExtensionSpecificsValid(extension_data.merged_data());
+    (*extension_data_map)[sync_data.id] = sync_data;
     id = sync_node.GetSuccessorId();
   }
   return true;
 }
 
 }  // namespace
 
 bool SlurpExtensionData(const ExtensionSyncTraits& traits,
                         const ExtensionServiceInterface& extension_service,
                         sync_api::UserShare* user_share,
                         ExtensionDataMap* extension_data_map) {
-  // Read client-side data first so server data takes precedence.
-  SlurpClientData(
-      traits.is_valid_and_syncable, extension_service,
-      extension_data_map);
-
+  // Read server-side data first so client user settings take
+  // precedence.
   if (!SlurpServerData(
           traits.root_node_tag, traits.extension_specifics_getter,
           user_share, extension_data_map)) {
     return false;
   }
+
+  SlurpClientData(
+      traits.is_valid_and_syncable, extension_service,
+      extension_data_map);
   return true;
 }
 
 namespace {
 
-// Updates the server data from the given extension data.
-// extension_data->ServerNeedsUpdate() must hold before this function
-// is called.  Returns whether or not the update was successful.  If
-// the update was successful, extension_data->ServerNeedsUpdate() will
-// be false after this function is called.  This function leaves
-// extension_data->ClientNeedsUpdate() unchanged.
+// Updates the server data from the given extension data.  Returns
+// whether or not the update was successful.
 bool UpdateServer(
     const ExtensionSyncTraits& traits,
-    ExtensionData* extension_data,
+    const ExtensionSyncData& data,
     sync_api::WriteTransaction* trans) {
-  DCHECK(extension_data->NeedsUpdate(ExtensionData::SERVER));
-  const sync_pb::ExtensionSpecifics& specifics =
-      extension_data->merged_data();
-  const std::string& id = specifics.id();
+  sync_pb::ExtensionSpecifics specifics;
+  SyncDataToSpecifics(data, &specifics);
+  const std::string& id = data.id;
   sync_api::WriteNode write_node(trans);
   if (write_node.InitByClientTagLookup(traits.model_type, id)) {
     (*traits.extension_specifics_setter)(specifics, &write_node);
   } else {
     sync_api::ReadNode root(trans);
     if (!root.InitByTagLookup(traits.root_node_tag)) {
       LOG(ERROR) << GetRootNodeDoesNotExistError(traits.root_node_tag);
       return false;
     }
     sync_api::WriteNode create_node(trans);
     if (!create_node.InitUniqueByCreation(traits.model_type, root, id)) {
       LOG(ERROR) << "Could not create node for extension " << id;
       return false;
     }
     (*traits.extension_specifics_setter)(specifics, &create_node);
   }
-  bool old_client_needs_update =
-      extension_data->NeedsUpdate(ExtensionData::CLIENT);
-  extension_data->ResolveData(ExtensionData::SERVER);
-  DCHECK(!extension_data->NeedsUpdate(ExtensionData::SERVER));
-  DCHECK_EQ(extension_data->NeedsUpdate(ExtensionData::CLIENT),
-            old_client_needs_update);
   return true;
 }
 
 }  // namespace
 
 bool FlushExtensionData(const ExtensionSyncTraits& traits,
                         const ExtensionDataMap& extension_data_map,
                         ExtensionServiceInterface* extension_service,
                         sync_api::UserShare* user_share) {
   sync_api::WriteTransaction trans(user_share);
   sync_api::ReadNode root(&trans);
   if (!root.InitByTagLookup(traits.root_node_tag)) {
     LOG(ERROR) << GetRootNodeDoesNotExistError(traits.root_node_tag);
     return false;
   }
 
   // Update server and client as necessary.
   for (ExtensionDataMap::const_iterator it = extension_data_map.begin();
        it != extension_data_map.end(); ++it) {
-    ExtensionData extension_data = it->second;
-    // Update server first.
-    if (extension_data.NeedsUpdate(ExtensionData::SERVER)) {
-      if (!UpdateServer(traits, &extension_data, &trans)) {
-        LOG(ERROR) << "Could not update server data for extension "
-                   << it->first;
-        return false;
-      }
-    }
-    DCHECK(!extension_data.NeedsUpdate(ExtensionData::SERVER));
-    ExtensionSyncData sync_data;
-    if (!SpecificsToSyncData(extension_data.merged_data(), &sync_data)) {
-      // TODO(akalin): Should probably recover or drop.
-      NOTREACHED();
+    const ExtensionSyncData& extension_data = it->second;
+    if (!UpdateServer(traits, extension_data, &trans)) {
+      LOG(ERROR) << "Could not update server data for extension "
+                 << it->first;
       return false;
     }
-    extension_service->ProcessSyncData(sync_data,
+    extension_service->ProcessSyncData(extension_data,
                                        traits.is_valid_and_syncable);
   }
   return true;
 }
 
 bool UpdateServerData(const ExtensionSyncTraits& traits,
                       const std::string& id,
                       const ExtensionServiceInterface& extension_service,
                       sync_api::UserShare* user_share,
                       std::string* error) {
   ExtensionSyncData data;
   if (!extension_service.GetSyncData(
           id, traits.is_valid_and_syncable, &data)) {
     *error =
         std::string("UpdateServerData() called for invalid or "
                     "unsyncable extension ") + id;
     LOG(DFATAL) << *error;
     return false;
   }
-  sync_pb::ExtensionSpecifics client_data;
-  SyncDataToSpecifics(data, &client_data);
-  DcheckIsExtensionSpecificsValid(client_data);
-  ExtensionData extension_data =
-      ExtensionData::FromData(ExtensionData::CLIENT, client_data);
 
   sync_api::WriteTransaction trans(user_share);
-
-  sync_api::ReadNode node(&trans);
-  if (node.InitByClientTagLookup(traits.model_type, id)) {
-    sync_pb::ExtensionSpecifics server_data =
-        (*traits.extension_specifics_getter)(node);
-    if (IsExtensionSpecificsValid(server_data)) {
-      // If server node exists and is valid, update |extension_data|
-      // from it (but with it taking precedence).
-      extension_data =
-          ExtensionData::FromData(ExtensionData::SERVER, server_data);
-      extension_data.SetData(ExtensionData::CLIENT, true, client_data);
-    } else {
-      LOG(ERROR) << "Invalid extensions specifics for id " << id
-                 << "; treating as empty";
-    }
+  if (!UpdateServer(traits, data, &trans)) {
+    *error =
+        std::string("Could not update server data for extension ") + id;
+    LOG(ERROR) << *error;
+    return false;
   }
-
-  if (extension_data.NeedsUpdate(ExtensionData::SERVER)) {
-    if (!UpdateServer(traits, &extension_data, &trans)) {
-      *error =
-          std::string("Could not update server data for extension ") + id;
-      LOG(ERROR) << *error;
-      return false;
-    }
-  }
-  DCHECK(!extension_data.NeedsUpdate(ExtensionData::SERVER));
-  // Client may still need updating, e.g. if we disable an extension
-  // while it's being auto-updated.  If so, then we'll be called
-  // again once the auto-update is finished.
-  //
-  // TODO(akalin): Figure out a way to tell when the above happens,
-  // so we know exactly what NeedsUpdate(CLIENT) should return.
   return true;
 }
 
 void RemoveServerData(const ExtensionSyncTraits& traits,
                       const std::string& id,
                       sync_api::UserShare* user_share) {
   sync_api::WriteTransaction trans(user_share);
   sync_api::WriteNode write_node(&trans);
   if (write_node.InitByClientTagLookup(traits.model_type, id)) {
     write_node.Remove();
   } else {
     LOG(ERROR) << "Server data does not exist for extension " << id;
   }
 }
 
 }  // namespace browser_sync