Extensions: Store disable reasons in Sync

chrome/browser/extensions/extension_sync_service.cc

Index: chrome/browser/extensions/extension_sync_service.cc
diff --git a/chrome/browser/extensions/extension_sync_service.cc b/chrome/browser/extensions/extension_sync_service.cc
index 9a8fddb81453f890135b095a10e945d8cfcadda9..fa4b5b1fd6dfdeef6ebcc0706c36e68956a5b71e 100644
--- a/chrome/browser/extensions/extension_sync_service.cc
+++ b/chrome/browser/extensions/extension_sync_service.cc
@@ -283,6 +283,7 @@ ExtensionSyncData ExtensionSyncService::GetExtensionSyncData(
   return ExtensionSyncData(
       extension,
       extension_service_->IsExtensionEnabled(extension.id()),
+      extension_prefs_->GetDisableReasons(extension.id()),
       extensions::util::IsIncognitoEnabled(extension.id(), profile_),
       extension_prefs_->HasDisableReason(extension.id(),
                                          Extension::DISABLE_REMOTE_INSTALL),
@@ -293,6 +294,7 @@ AppSyncData ExtensionSyncService::GetAppSyncData(
     const Extension& extension) const {
   return AppSyncData(
       extension, extension_service_->IsExtensionEnabled(extension.id()),
+      extension_prefs_->GetDisableReasons(extension.id()),
       extensions::util::IsIncognitoEnabled(extension.id(), profile_),
       extension_prefs_->HasDisableReason(extension.id(),
                                          Extension::DISABLE_REMOTE_INSTALL),
@@ -516,13 +518,21 @@ bool ExtensionSyncService::ProcessExtensionSyncDataHelper(
   if (extension_sync_data.enabled()) {
     extension_service_->EnableExtension(id);
   } else if (!IsPendingEnable(id)) {
+    int disable_reasons = extension_sync_data.disable_reasons();
     if (extension_sync_data.remote_install()) {
-      extension_service_->DisableExtension(id,
-                                           Extension::DISABLE_REMOTE_INSTALL);
-    } else {
-      extension_service_->DisableExtension(
-          id, Extension::DISABLE_UNKNOWN_FROM_SYNC);
+      // In the non-legacy case where we sync disable reasons,
+      // DISABLE_REMOTE_INSTALL should be there already.

[not at google - send to devlin, 2015/05/19 14:32:10]

It might be because I just woke up, but why is this the case? Might there not
already be synced data that sets remote install but didn't set the disable bit?
Or have we really never pushed a sync node that has remove install but not
disable reasons?

[Marc Treib, 2015/05/19 15:30:55]

What I'm checking is: If there are any disable reasons at all, then
REMOTE_INSTALL must be among them. Note the first line of the comment, which
tries (not very successfully) to explain that :)
I've tried to make the comment a bit clearer.

+      DCHECK(!disable_reasons ||
+             (disable_reasons & Extension::DISABLE_REMOTE_INSTALL));
+      disable_reasons |= Extension::DISABLE_REMOTE_INSTALL;
     }
+    if (!disable_reasons) {
+      // Legacy case, from before we synced disable reasons.

[not at google - send to devlin, 2015/05/19 14:32:10]

You might want to link to the bug# or mention the milestone or something - i.e.
define "legacy" in a concrete way - so that somebody reading the code doesn't
need to look too much into it. It will probably be legacy that we can never get
rid of.

// Legacy pre-M45 case, from before we synced disable reasons (see
crbug.com/...).

[Marc Treib, 2015/05/19 15:30:55]

Done.

+      disable_reasons = Extension::DISABLE_UNKNOWN_FROM_SYNC;

[not at google - send to devlin, 2015/05/12 18:04:17]

This is important to test, and rather than lumping it in the single new test
case you've added, try to test in isolation.

[Marc Treib, 2015/05/19 12:12:03]

"This" being the legacy case, where no disable reasons come in through Sync?
I've added a test for that. If that's not what you meant, please clarify :)

[not at google - send to devlin, 2015/05/19 14:32:10]

yeah that's what I mean, thanks.

+    }
+
+    extension_service_->DisableExtension(
+        id, Extension::DisableReason(disable_reasons));
   }
 
   // We need to cache some version information here because setting the