[Sync] Move some extension-sync-related logic to ExtensionService

[Sync] Move some extension-sync-related logic to ExtensionService

Add ExtensionSyncData class to hold the data that goes between the
extension service and extension sync.

Add ProcessSyncData() method to ExtensionService (with unit tests!).

Remove now-unneeded extension sync functions.

BUG=77995
TEST=

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

[akalin, 2011-04-14 20:59:49]

+asargent for extension stuff, +zea for sync stuff

[asargent_no_longer_on_chrome, 2011-04-14 23:32:14]

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1199:
browser_sync::IsValidAndSyncableApp(extension);
 I'm not crazy about seeing the ExtensionService using sync code here. Can we
instead pass a PendingExtensionInfo::ShouldAllowInstallPredicate into the
ProcessSyncData method?

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1212: LOG(WARNING) << "Sync tried
to uninstall nonexistent extension " << id;
Note that it's possible we couldn't uninstall because the extension is
policy-controlled:

http://dev.chromium.org/administrators/policy-list-3#Extensions

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.h:90: // sync.
Can you a bug for this and link to it here? The bug should have a quick
explanation of what's going to be different about app sync.

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.h:96: //   GetInitialSyncData(bool
(*filter)(Extension),
It's not immediately obvious to me what this would mean - could you expand the
comment a little to explain what the method would do?

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.h:360: const ExtensionSyncData&
extension_sync_data);
Please include a comment describing what the method does. (Eventually we should
probably clean this up too to move such comments up to the interface for all
methods in the interface).

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_sync_data.h (right):

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_sync_data.h:14: struct ExtensionSyncData {
nit: Even though this is just a struct, it would be helpful if you had a brief
overview comment here ala
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Comments

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_sync_data.h:27: // Version-dependent
properties.
Isn't "version" version-dependent? :)

Actually I'm a little confused by what you mean by version-dependent and
version-independent. Do you mean that the update url could change between 2
versions of an extension, but it's not possible that sync would also notice a
change in the enabled or incognito setting in the same sync "transaction"?

[akalin, 2011-04-15 01:17:11]

All comments addressed, please take another look

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1199:
browser_sync::IsValidAndSyncableApp(extension);
On 2011/04/14 23:32:14, Antony Sargent wrote:
>  I'm not crazy about seeing the ExtensionService using sync code here. Can we
> instead pass a PendingExtensionInfo::ShouldAllowInstallPredicate into the
> ProcessSyncData method? 

Done.

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1212: LOG(WARNING) << "Sync tried
to uninstall nonexistent extension " << id;
On 2011/04/14 23:32:14, Antony Sargent wrote:
> Note that it's possible we couldn't uninstall because the extension is
> policy-controlled:
> 
> http://dev.chromium.org/administrators/policy-list-3#Extensions

Ah, okay.  Fixed.

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.h:90: // sync.
On 2011/04/14 23:32:14, Antony Sargent wrote:
> Can you a bug for this and link to it here? The bug should have a quick
> explanation of what's going to be different about app sync.

There are already a couple of bugs filed: added them.

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.h:96: //   GetInitialSyncData(bool
(*filter)(Extension),
On 2011/04/14 23:32:14, Antony Sargent wrote:
> It's not immediately obvious to me what this would mean - could you expand the
> comment a little to explain what the method would do?

Done.

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.h:360: const ExtensionSyncData&
extension_sync_data);
On 2011/04/14 23:32:14, Antony Sargent wrote:
> Please include a comment describing what the method does. (Eventually we
should
> probably clean this up too to move such comments up to the interface for all
> methods in the interface).

Done, but added it to the interface method (might as well start now).

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_sync_data.h (right):

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_sync_data.h:14: struct ExtensionSyncData {
On 2011/04/14 23:32:14, Antony Sargent wrote:
> nit: Even though this is just a struct, it would be helpful if you had a brief
> overview comment here ala
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Comments

Done.

http://codereview.chromium.org/6852029/diff/7002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_sync_data.h:27: // Version-dependent
properties.
On 2011/04/14 23:32:14, Antony Sargent wrote:
> Isn't "version" version-dependent? :)
> 
> Actually I'm a little confused by what you mean by version-dependent and
> version-independent. Do you mean that the update url could change between 2
> versions of an extension, but it's not possible that sync would also notice a
> change in the enabled or incognito setting in the same sync "transaction"? 

Added clarifying comments.

[Nicolas Zea, 2011-04-15 04:41:10]

Sync change LGTM

[asargent_no_longer_on_chrome, 2011-04-15 18:42:30]

extension parts lgtm

http://codereview.chromium.org/6852029/diff/9006/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/6852029/diff/9006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1195: should_allow_install) {
Nit: consider just calling this "allow" or something to avoid this weird
line-wrapping. Or, perhaps shorten ShouldAllowInstallPredicate to just
"AllowPredicate" or "AllowFunc"?

http://codereview.chromium.org/6852029/diff/9006/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_sync_data.h (right):

http://codereview.chromium.org/6852029/diff/9006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_sync_data.h:5: // A struct that encapsulates
the synced properties of an Extension.
nit: move this to be right above the "struct" declaration on line 15.

[akalin, 2011-04-15 21:19:39]

Checking in as soon as trybots pass

http://codereview.chromium.org/6852029/diff/9006/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/6852029/diff/9006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1195: should_allow_install) {
On 2011/04/15 18:42:30, Antony Sargent wrote:
> Nit: consider just calling this "allow" or something to avoid this weird
> line-wrapping. Or, perhaps shorten ShouldAllowInstallPredicate to just
> "AllowPredicate" or "AllowFunc"? 
> 
> 

Done.

http://codereview.chromium.org/6852029/diff/9006/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_sync_data.h (right):

http://codereview.chromium.org/6852029/diff/9006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_sync_data.h:5: // A struct that encapsulates
the synced properties of an Extension.
On 2011/04/15 18:42:30, Antony Sargent wrote:
> nit: move this to be right above the "struct" declaration on line 15.

Done.