ExtensionSyncService cleanup: process uninstalls in one step

ExtensionSyncService cleanup: process uninstalls in one step

Before, the process was split into PrepareToSyncUninstallExtension and
ProcessSyncUninstallExtension for no good reason.

BUG=None

Committed: https://crrev.com/59ef2589e04cebc6d8a3c7d13c6b844b1362714b
Cr-Commit-Position: refs/heads/master@{#339003}

[Marc Treib, 2015-07-15 11:33:28]

treib@chromium.org changed reviewers:
+ kalman@chromium.org

[Marc Treib, 2015-07-15 11:33:28]

Another small and (hopefully) uncontroversial cleanup before commencing with the
bigger changes. PTAL!

[not at google - send to devlin, 2015-07-15 17:22:42]

lgtm. I cannot figure out why this was split into 2 functions. The only reason
would be that PrepareToSync uses state that is about to be invalidated, but it
doesn't look that way to me.

[not at google - send to devlin, 2015-07-15 17:49:14]

Optional more cleanup which would be really nice:

https://codereview.chromium.org/1236363002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/1236363002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_service.cc:134:
bundle->PushSyncDeletion(id, CreateSyncData(extension).GetSyncData());
Btw it would be nice if this function could mostly be replaced by
"this->SyncExtensionChangeIfNeeded". Conceptually it's the same thing, just one
pushes a delete and the other pushes an add/update.

The weird thing is that bundle->PushSyncChangeIfNeeded doesn't take the SyncData
as an argument and instead constructs its own, whereas on the other hand
bundle->PushSyncDeletion *does*. Instead it turns around and calls
CreateSyncData on *this*. It would be much simpler to just pass the sync data
into bundle->PushSyncChangeIfNeeded.

The other weird thing is that it's called "PushSyncChangeIfNeeded" whereas it
actually always pushes the change. A better name would be "PushSyncAddOrUpdate".

So, a further nice cleanup would be to:
1. Rename bundle->PushSyncChangeIfNeeded to PushSyncAddOrUpdate, and make it
take the SyncData as an argument rather than grab it from the sync service.
2. Combine this->SyncUninstallExtension/SyncExtensionChangeIfNeeded to call a
single function like "SyncExtensionChangeIfNeedImpl" which takes a boolean
whether it's a removal, and uses that to decide whether to call
bundle->PushSyncDeletion or bundle->PushSyncAddOrUpdate. (it would be even
better if these classes could figure out whether it's a deletion but I don't
want to get crazy here).

[Marc Treib, 2015-07-16 08:09:53]

On 2015/07/15 17:22:42, kalman wrote:
> lgtm. I cannot figure out why this was split into 2 functions. The only reason
> would be that PrepareToSync uses state that is about to be invalidated, but it
> doesn't look that way to me.

Yup. My best guess is that that used to be the case at some point in the past?

[Marc Treib, 2015-07-16 08:12:29]

https://codereview.chromium.org/1236363002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/1236363002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_service.cc:134:
bundle->PushSyncDeletion(id, CreateSyncData(extension).GetSyncData());
On 2015/07/15 17:49:13, kalman wrote:
> Btw it would be nice if this function could mostly be replaced by
> "this->SyncExtensionChangeIfNeeded". Conceptually it's the same thing, just
one
> pushes a delete and the other pushes an add/update.
> 
> The weird thing is that bundle->PushSyncChangeIfNeeded doesn't take the
SyncData
> as an argument and instead constructs its own, whereas on the other hand
> bundle->PushSyncDeletion *does*. Instead it turns around and calls
> CreateSyncData on *this*. It would be much simpler to just pass the sync data
> into bundle->PushSyncChangeIfNeeded.

Yup, the other CL does just that :)
It's not (easily) possible here, because PendingEnables also calls
bundle->PushSyncChangeIfNeeded, and it only has an Extension* and can't easily
get the SyncData from it.

> The other weird thing is that it's called "PushSyncChangeIfNeeded" whereas it
> actually always pushes the change. A better name would be
"PushSyncAddOrUpdate".

Done.

> So, a further nice cleanup would be to:
> 1. Rename bundle->PushSyncChangeIfNeeded to PushSyncAddOrUpdate, and make it
> take the SyncData as an argument rather than grab it from the sync service.

See above: Not yet possible, but the other CL does that.

> 2. Combine this->SyncUninstallExtension/SyncExtensionChangeIfNeeded to call a
> single function like "SyncExtensionChangeIfNeedImpl" which takes a boolean
> whether it's a removal, and uses that to decide whether to call
> bundle->PushSyncDeletion or bundle->PushSyncAddOrUpdate. (it would be even
> better if these classes could figure out whether it's a deletion but I don't
> want to get crazy here).

Without 1. this would be a bit awkward, since PushSyncAddOrUpdate and
PushSyncDeletion still work differently. I'll do the cleanup in the other CL
though :)

[Marc Treib, 2015-07-16 08:12:40]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-07-16 08:12:40]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1236363002/#ps20001
(title: "review")

[commit-bot: I haz the power, 2015-07-16 08:12:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236363002/20001

[commit-bot: I haz the power, 2015-07-16 08:55:09]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-07-16 08:56:04]

Patchset 2 (id:??) landed as
https://crrev.com/59ef2589e04cebc6d8a3c7d13c6b844b1362714b
Cr-Commit-Position: refs/heads/master@{#339003}