[Sync] Rip out overly-complicated ExtensionData class

[Sync] Rip out overly-complicated ExtensionData class

Rip out sync code that took great pains to update client/server only when needed.  We now rely on the underlying layers to handle redundant updates intelligently (which both the extension service and syncapi do).

Add ExtensionSyncData::Merge() method.

Clean up now-unneeded code in extension_util.{h,cc} and
extension_sync.{h,cc}.

BUG=72659
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=83221

[akalin, 2011-04-26 23:38:54]

+asargent for extensions stuff, +zea for everything else

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

http://codereview.chromium.org/6902054/diff/1/chrome/browser/sync/glue/extens...
File chrome/browser/sync/glue/extension_sync.cc (right):

http://codereview.chromium.org/6902054/diff/1/chrome/browser/sync/glue/extens...
chrome/browser/sync/glue/extension_sync.cc:51: result.first->second.Merge(*it);
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));

?

[Nicolas Zea, 2011-04-27 17:34:41]

sync changes LGTM

[akalin, 2011-04-27 17:53:24]

http://codereview.chromium.org/6902054/diff/1/chrome/browser/sync/glue/extens...
File chrome/browser/sync/glue/extension_sync.cc (right):

http://codereview.chromium.org/6902054/diff/1/chrome/browser/sync/glue/extens...
chrome/browser/sync/glue/extension_sync.cc:51: result.first->second.Merge(*it);
On 2011/04/27 16:45:19, Antony Sargent wrote:
> 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));
> 
> ?

(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?

[asargent_no_longer_on_chrome, 2011-04-27 18:48:42]

Ok, that makes sense, thanks for the explanation. LGTM w/ one nit.

http://codereview.chromium.org/6902054/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_sync_data.cc (right):

http://codereview.chromium.org/6902054/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_sync_data.cc:15: DCHECK_EQ(id, new_data.id);
nit: change this to a CHECK_EQ

[akalin, 2011-04-27 21:25:32]

Addressed comments, checking in.

http://codereview.chromium.org/6902054/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_sync_data.cc (right):

http://codereview.chromium.org/6902054/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_sync_data.cc:15: DCHECK_EQ(id, new_data.id);
On 2011/04/27 18:48:43, Antony Sargent wrote:
> nit: change this to a CHECK_EQ

Done.  Also changed the other DCHECKs.