Apps/Extensions Sync refactoring -- delete most of the old glue, implement new sync API.

Apps/Extensions Sync refactoring -- delete most of the old glue, implement new sync API.

This should maintain current behavior; there are still a number of edge cases around sync + network issues/changing extension versions/etc. that are not fixed by this change.

Reviewers:

akalin: everything
asargent: sanity check around ExtensionService/Extension/ExtensionSyncData

BUG=83983
TEST=sync tests pass, browser tests pass, manually adding/removing extensions/apps works.

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

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

[akalin, 2011-08-12 03:35:01]

This is awesome!

A bunch of initial comments.  Also please run through the
linux_sync,mac_sync,win_sync trybots.  Probably want to run through
linux_clang,mac_gcc, too.

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (left):

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1713: LOG(WARNING) << "Could not
add pending extension for " << id;
From the comments re. when AddFromSync returns false, I think we can conclude
the extension is already pending, so maybe returning is the wrong thing here.

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1743: NOTREACHED();
may as well just do NOTREACHED() << "Got " ...;
Also return a suitable SyncError() here

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1773: DCHECK(bundle);
change to CHECK, since you're dereferencing in next line anyway

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1780: DCHECK(bundle);
CHECK

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1783: for (int i = 0; i <
static_cast<int>(extension_sync_data.size()); ++i) {
may as well use iterators here, too

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1797: DCHECK(bundle);
CHECK

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1882: // TODO(bolms): Install
disabled if it came in from sync disabled.
i believe the DisableExtension(id) call above should do exactly that already

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (left):

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:439: // Sync methods
implementation.
Sync methods -> SyncableService

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:543: SyncBundle() :
filter(IsSyncableNone), sync_processor(NULL) { }
clang will probably complain about complex inline constructor, so you may want
to move them into the .cc file

Also, it'll probably complain about implicit destructor, so make it explicit and
move it to .cc file

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:593: const SyncBundle*
GetSyncBundleForModelType(syncable::ModelType type) const;
maybe append Const to this fn name to avoid overloading (which is against google
style)

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_sync_data.h (right):

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_data.h:17: class SyncData;
need to #include sync_data.h, since you're returning a value of type SyncData

http://codereview.chromium.org/7564037/diff/11001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/7564037/diff/11001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:2857: // We only want to sync user scripts
with update URLs.
clarify that we want to sync user scripts with gallery update URLs

Maybe add DCHECK(update_url() == GalleryUpdateUrl(true) || update_url() ==
GalleryUpdateUrl(false)); in the else clause

[asargent_no_longer_on_chrome, 2011-08-12 20:50:39]

Seems great overall, with a few nits and a couple of questions for you.

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1719: return NULL;
nit: Looks like these two have the same body. Maybe use a macro or templates or
something to eliminate the duplication?

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1743: NOTREACHED();
On 2011/08/12 03:35:02, akalin wrote:
> may as well just do NOTREACHED() << "Got " ...;
> Also return a suitable SyncError() here

consider LOG(FATAL)

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1746: bundle->sync_processor =
sync_processor;
consider adding a CHECK(sync_processor) right above this

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1762:
sync_change_list.push_back(SyncChange(SyncChange::ACTION_UPDATE, *i));
I was a bit surprised not to see you check here whether the local data about
this extension differs from what sync has, and if not then avoid adding an entry
to sync_change_list. Does the sync system skip pushing unnecessary changes up to
the server for you?

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1782: SyncDataList
rv(extension_sync_data.size());
naming nit: "rv" is too abbreviated - maybe use "result" or something like that.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:585: // Notifies Sync (if needed)
of a newly-installed or updated extension.
In the .cc file I see you also call this when an extension is enabled or
disabled or changes incognito setting, so perhaps update the comment here to
that effect. Also, a thought for you to consider - would a better name for the
function be SyncExtensionChangeIfNeeded? When I see the word "Update" I tend to
think of "an updated version of the extension was installed" but I might just be
weird since I've done a lot of the autoupdate stuff.

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:611: void
ProcessExtensionSyncData(
optional naming suggestion: consider just "ProcessSyncData"

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:797: SyncBundle
extension_sync_bundle_;
A question for you to ponder - would it be easier to just have one SyncBundle
where we store both apps and extensions? It seems like there's some complexity
in several places in extension_service.cc around getting the right SyncBundle,
having an ExtensionFilter etc. that could maybe just be replaced with one or two
"if (extension->is_app())" statements.

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_sync_data.cc (right):

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_data.cc:106:
sync_pb::ExtensionSpecifics es;
style nit: "es" and "ent_spec" are probably too short. 

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...

Maybe s/es/specifics/ and s/ent_spec/entity/ ?

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_sync_data.h (right):

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_data.h:36: std::string id() const {
return id_; }
nit: should return "const std::string&"

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_data.h:48: Version version() const {
return version_; }
nit: should return "const Version&"

http://codereview.chromium.org/7564037/diff/11001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/7564037/diff/11001/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:108: enum SyncType {
Do these literal values end up hardcoded into protocol buffers or anything in
the sync system? If so include a warning about not removing / changing order of
items (similar to the above one for Type). 

Also, from looking at the implementation of GetSyncType(), more items than you
might think end up with SYNC_TYPE_NONE - ie it's not just a sentinel value, but
one that many extensions have for a valid reason. It might be worth having a
brief comment about that.

http://codereview.chromium.org/7564037/diff/11001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/7564037/diff/11001/chrome/common/extensions/ex...
chrome/common/extensions/extension_unittest.cc:861: #endif
I think a while back I made it so that you can pass an empty FilePath to
Extension::Create(), so you might be able to get rid of these ugly
FILE_PATH_LITERAL's here.

http://codereview.chromium.org/7564037/diff/11001/chrome/common/extensions/ex...
chrome/common/extensions/extension_unittest.cc:946: #endif
nit: add comment here-

#endif  // !defined(CHROMEOS)

[Ben Olmstead, 2011-08-15 20:20:12]

Updated per review comments.

Try jobs for the current patch are running now.

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (left):

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1713: LOG(WARNING) << "Could not
add pending extension for " << id;
On 2011/08/12 03:35:02, akalin wrote:
> From the comments re. when AddFromSync returns false, I think we can conclude
> the extension is already pending, so maybe returning is the wrong thing here.

Looking at the code, it looks like this can only happen in fairly strange cases.
 The most likely scenario is a new extension coming in both through enterprise
policy and sync on Chrome startup.  Thinking through the cases, it does look
like it should get added to pending_sync_data, although it will stay there
forever.

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1762:
sync_change_list.push_back(SyncChange(SyncChange::ACTION_UPDATE, *i));
On 2011/08/12 20:50:39, Antony Sargent wrote:
> I was a bit surprised not to see you check here whether the local data about
> this extension differs from what sync has, and if not then avoid adding an
entry
> to sync_change_list. Does the sync system skip pushing unnecessary changes up
to
> the server for you?

According to Fred, yes it does.  I considered the case, but did not want to add
more code if the sync engine already handled it.

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1882: // TODO(bolms): Install
disabled if it came in from sync disabled.
On 2011/08/12 03:35:02, akalin wrote:
> i believe the DisableExtension(id) call above should do exactly that already

You are correct--just verified.

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:611: void
ProcessExtensionSyncData(
On 2011/08/12 20:50:39, Antony Sargent wrote:
> optional naming suggestion: consider just "ProcessSyncData"

Unfortunately, SyncData is a different class used by the sync engine--I want to
keep this separate for clarity.

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:797: SyncBundle
extension_sync_bundle_;
On 2011/08/12 20:50:39, Antony Sargent wrote:
> A question for you to ponder - would it be easier to just have one SyncBundle
> where we store both apps and extensions? It seems like there's some complexity
> in several places in extension_service.cc around getting the right SyncBundle,
> having an ExtensionFilter etc. that could maybe just be replaced with one or
two
> "if (extension->is_app())" statements. 

That's actually where I started, but I added the SyncBundle class when I started
implementing GetAllSyncData() and StopSyncing()--they end up having to parse out
synced_extensions and pending_sync_data to decide what to return or remove,
respectively.  It also doesn't save much, since we still have to find the
appropriate SyncChangeProcessor for a particular extension.

I do have a pile of helper methods to find the correct bundle, mostly because I
was repeating the same (or very similar) ten lines of code in many places.

The ExtensionFilters are artifacts of the original code, but they still seem
convenient for a few places--particularly the PendingExtensionManager,
GetAllSyncData(), and GetSyncBundleForExtension().  It wouldn't be too big of a
deal to pass an Extension::SyncType and compare to Extension::GetSyncType() in
those cases, though.

[asargent_no_longer_on_chrome, 2011-08-15 23:34:29]

LGTM w/ 1 nit

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/7564037/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:797: SyncBundle
extension_sync_bundle_;
On 2011/08/15 20:20:12, Ben Olmstead wrote:
> On 2011/08/12 20:50:39, Antony Sargent wrote:
> > A question for you to ponder - would it be easier to just have one
SyncBundle
> > where we store both apps and extensions? It seems like there's some
complexity
> > in several places in extension_service.cc around getting the right
SyncBundle,
> > having an ExtensionFilter etc. that could maybe just be replaced with one or
> two
> > "if (extension->is_app())" statements. 
> 
> That's actually where I started, but I added the SyncBundle class when I
started
> implementing GetAllSyncData() and StopSyncing()--they end up having to parse
out
> synced_extensions and pending_sync_data to decide what to return or remove,
> respectively.  It also doesn't save much, since we still have to find the
> appropriate SyncChangeProcessor for a particular extension.
> 
> I do have a pile of helper methods to find the correct bundle, mostly because
I
> was repeating the same (or very similar) ten lines of code in many places.
> 
> The ExtensionFilters are artifacts of the original code, but they still seem
> convenient for a few places--particularly the PendingExtensionManager,
> GetAllSyncData(), and GetSyncBundleForExtension().  It wouldn't be too big of
a
> deal to pass an Extension::SyncType and compare to Extension::GetSyncType() in
> those cases, though.

Ok

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_sync_data.h (right):

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_data.h:49: Version version() const {
return version_; }
nit: looks like this still returns Version and not const Version&

[akalin, 2011-08-17 17:44:24]

LGTM

all comments are suggestions

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:920: SyncChangeList
sync_change_list(1);
you can do SyncChangeList sync_change_list(1, sync_change);

or maybe

...->ProcessSyncChanges(FROM_HERE, SyncChangeList(1, sync_change))

(everywhere else)

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1659: };
no semicolon after }.  it should be:

namespace {
...
}  // namespace

also, if it's in an anon namespace, no 'static' keyword needed

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1712: switch (type) { \
wrap with do { ... } while(0) to require a semicolon

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1732: 
#undef macro here

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:539: bool HasExtensionId(const
std::string& id) const {
since you're de-inlining the constructor/destructor, may as well de-inline these
functions, too

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_unittest.cc:3597: SyncChangeList
list(1);
SyncChangeList list(1, sync_change)

or ->ProcessSyncChanges(FROM_HERE, SyncChangeList(1, sync_change))

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_sync_data.cc (right):

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_data.cc:13: : uninstalled_(false),
enabled_(false), incognito_enabled_(false) {}
initialize type_ here, too

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_data.cc:18: : uninstalled_(false),
enabled_(false), incognito_enabled_(false) {
initialize type_ here, too

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_data.cc:32: uninstalled_ = false;
prefer putting everything in initialization list

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_data.cc:65: NOTREACHED();
NOTREACHED() only LOG(ERROR)s on release, so probably want to return an invalid
SyncData() after

or change to LOG(FATAL)

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_data.cc:81: NOTREACHED();
early-return here for release or LOG(FATAL)

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_data.cc:87: NOTREACHED();
early return or LOG(FATAL)

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_data.cc:93: NOTREACHED();
early return or LOG(FATAL)

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_data.cc:115: NOTREACHED();
early return or LOG(FATAL)

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_sync_data.h (right):

http://codereview.chromium.org/7564037/diff/19002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_data.h:50: GURL update_url() const {
return update_url_; }
may as well return const GURL&

[commit-bot: I haz the power, 2011-08-23 07:17:36]

Change committed as 97829