Massive refactoring of extensions sync code

Massive refactoring of extensions sync code

In preparation for apps sync, refactored out all the common extensions
sync code into extensions_sync_util.{h,cc} and extension_sync_traits.{h,cc}.

BUG=46512
TEST=manual

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

[akalin, 2010-08-12 02:16:10]

+nick for review

+asargent for extensions_service.cc changes.

[asargent_no_longer_on_chrome, 2010-08-12 16:21:05]

extensions_service.cc changes lgtm

http://codereview.chromium.org/3110008/diff/26001/27001
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3110008/diff/26001/27001#newcode967
chrome/browser/extensions/extensions_service.cc:967:
browser_sync::GetExtensionSyncTraits();
Would it look a little cleaner to have a "using browser_sync;" line at the top
of the file, so this code can omit the "browser_sync::" prefix in several places
here? Your call.

[ncarter (slow), 2010-08-12 17:48:24]

I'm wondering if you sent me this review because you knew that deep down I'm a C
programmer who secretly hates organizing code into classes.

http://codereview.chromium.org/3110008/diff/26001/27001
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3110008/diff/26001/27001#newcode967
chrome/browser/extensions/extensions_service.cc:967:
browser_sync::GetExtensionSyncTraits();
On 2010/08/12 16:21:05, Antony Sargent wrote:
> Would it look a little cleaner to have a "using browser_sync;" line at the top
> of the file, so this code can omit the "browser_sync::" prefix in several
places
> here? Your call. 
> 

isn't "using namespace;" forbidden by the style guide?  (you can import
individual identifiers, but you can't blanket import everything?)

http://codereview.chromium.org/3110008/diff/26001/27002
File chrome/browser/sync/glue/extension_change_processor.cc (right):

http://codereview.chromium.org/3110008/diff/26001/27002#newcode54
chrome/browser/sync/glue/extension_change_processor.cc:54: Extension* extension
= Details<Extension>(details).ptr();
[unchanged by your refactor, so ignore freely] do the extension APIs allow this
be const?  It being const would make the cached id ref a little easier to prove
safe.

http://codereview.chromium.org/3110008/diff/26001/27002#newcode112
chrome/browser/sync/glue/extension_change_processor.cc:112:
(*traits_.extension_specifics_getter)(node);
I see where this is headed.  Clever.

http://codereview.chromium.org/3110008/diff/26001/27007
File chrome/browser/sync/glue/extension_sync_traits.h (right):

http://codereview.chromium.org/3110008/diff/26001/27007#newcode63
chrome/browser/sync/glue/extension_sync_traits.h:63:
ExtensionSpecificsEntityGetter extension_specifics_entity_getter;
could also be const?

http://codereview.chromium.org/3110008/diff/26001/27008
File chrome/browser/sync/glue/extension_sync_util.cc (right):

http://codereview.chromium.org/3110008/diff/26001/27008#newcode35
chrome/browser/sync/glue/extension_sync_util.cc:35: ExtensionsService*
GetExtensionsServiceFromProfile(
What kinds of errors is this getter-wrapper designed to catch?

http://codereview.chromium.org/3110008/diff/26001/27011
File chrome/browser/sync/glue/extension_util.h (right):

http://codereview.chromium.org/3110008/diff/26001/27011#newcode8
chrome/browser/sync/glue/extension_util.h:8: 
I don't see a strong distinction in purpose between
browser/sync/glue/extension_sync_util.h and browser/sync/glue/extension_util.h.

I recommend either merging the two files, or coming up with a categorization
that's more clearly distinct.  Seems like a common property of the current
"extension sync util" is the fact that they operate on traits.  I suppose it
would be a weird design to organize any of these as non-virtual methods of the
traits class?  Many of the functions in this file, "extension util", operate on
specifics, or bridge the representation between the ExtensionService types and
the sync Specifics types.

[asargent_no_longer_on_chrome, 2010-08-12 18:28:44]

http://codereview.chromium.org/3110008/diff/26001/27001
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3110008/diff/26001/27001#newcode967
chrome/browser/extensions/extensions_service.cc:967:
browser_sync::GetExtensionSyncTraits();
> isn't "using namespace;" forbidden by the style guide?  (you can import
> individual identifiers, but you can't blanket import everything?)

Oh yes, good point. You could import the individual identifiers if you want.

[akalin, 2010-08-12 21:55:47]

Addressed all comments.  Please take another look!

http://codereview.chromium.org/3110008/diff/26001/27001
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3110008/diff/26001/27001#newcode967
chrome/browser/extensions/extensions_service.cc:967:
browser_sync::GetExtensionSyncTraits();
On 2010/08/12 18:28:44, Antony Sargent wrote:
> > isn't "using namespace;" forbidden by the style guide?  (you can import
> > individual identifiers, but you can't blanket import everything?)
> 
> Oh yes, good point. You could import the individual identifiers if you want. 
> 

I don't know if that saves much space, so I decided not to import anything.

Also, realized that the is_theme() check is important here.  Added it back in.

http://codereview.chromium.org/3110008/diff/26001/27002
File chrome/browser/sync/glue/extension_change_processor.cc (right):

http://codereview.chromium.org/3110008/diff/26001/27002#newcode54
chrome/browser/sync/glue/extension_change_processor.cc:54: Extension* extension
= Details<Extension>(details).ptr();
On 2010/08/12 17:48:24, ncarter wrote:
> [unchanged by your refactor, so ignore freely] do the extension APIs allow
this
> be const?  It being const would make the cached id ref a little easier to
prove
> safe.

you mean Details<const Extension>?  I think it's okay, but it prefer to leave it
as Details<Extension> for consistency.  I made the pointer const, though.

http://codereview.chromium.org/3110008/diff/26001/27011
File chrome/browser/sync/glue/extension_util.h (right):

http://codereview.chromium.org/3110008/diff/26001/27011#newcode8
chrome/browser/sync/glue/extension_util.h:8: 
On 2010/08/12 17:48:24, ncarter wrote:
> I don't see a strong distinction in purpose between
> browser/sync/glue/extension_sync_util.h and
browser/sync/glue/extension_util.h.
> 
> I recommend either merging the two files, or coming up with a categorization
> that's more clearly distinct.  Seems like a common property of the current
> "extension sync util" is the fact that they operate on traits.  I suppose it
> would be a weird design to organize any of these as non-virtual methods of the
> traits class?  Many of the functions in this file, "extension util", operate
on
> specifics, or bridge the representation between the ExtensionService types and
> the sync Specifics types.

Added comments to both files, renamed extension_sync_util.{h,cc} to
extension_sync.{h,cc}, since I think it reflects the core logic for extensions
sync.

[ncarter (slow), 2010-08-13 00:07:49]

Thanks for the changes.  LGTM after #include nits.

http://codereview.chromium.org/3110008/diff/36002/14008
File chrome/browser/sync/glue/extension_sync.h (right):

http://codereview.chromium.org/3110008/diff/36002/14008#newcode13
chrome/browser/sync/glue/extension_sync.h:13: #include <set>
don't need.

http://codereview.chromium.org/3110008/diff/36002/14008#newcode17
chrome/browser/sync/glue/extension_sync.h:17: #include
"chrome/browser/sync/glue/extension_util.h"
You shouldn't need anything besides extension_data.h, map, and string?

[akalin, 2010-08-13 01:53:26]

On 2010/08/13 00:07:49, ncarter wrote:
> Thanks for the changes.  LGTM after #include nits.
> 
> http://codereview.chromium.org/3110008/diff/36002/14008
> File chrome/browser/sync/glue/extension_sync.h (right):
> 
> http://codereview.chromium.org/3110008/diff/36002/14008#newcode13
> chrome/browser/sync/glue/extension_sync.h:13: #include <set>
> don't need.
> 
> http://codereview.chromium.org/3110008/diff/36002/14008#newcode17
> chrome/browser/sync/glue/extension_sync.h:17: #include
> "chrome/browser/sync/glue/extension_util.h"
> You shouldn't need anything besides extension_data.h, map, and string?

Done, submitting.