Extensions: Store disable reasons in Sync

chrome/browser/extensions/extension_sync_service.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_sync_service.h"
 
 #include <iterator>
 
 #include "base/basictypes.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 265 matching lines @@
   extension_prefs_->app_sorting()->FixNTPOrdinalCollisions();
 
   return syncer::SyncError();
 }
 
 ExtensionSyncData ExtensionSyncService::GetExtensionSyncData(
     const Extension& extension) const {
   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),
       GetAllowedOnAllUrlsOptionalBoolean(extension.id(), profile_));
 }
 
 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),
       GetAllowedOnAllUrlsOptionalBoolean(extension.id(), profile_),
       extension_prefs_->app_sorting()->GetAppLaunchOrdinal(extension.id()),
       extension_prefs_->app_sorting()->GetPageOrdinal(extension.id()),
       extensions::GetLaunchTypePrefValue(extension_prefs_, extension.id()));
 }
 
 std::vector<ExtensionSyncData>
@@ skipping 203 matching lines @@
   // been installed yet, so we don't know if the disable reason was a
   // permissions increase.  That will be updated once CheckPermissionsIncrease
   // is called for it.
   // However if the extension is marked as a remote install in sync, we know
   // what the disable reason is, so set it to that directly. Note that when
   // CheckPermissionsIncrease runs, it might still add permissions increase
   // as a disable reason for the extension.
   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
   // incognito flag invalidates the |extension| pointer (it reloads the
   // extension).
   bool extension_installed = (extension != NULL);
   int version_compare_result = extension ?
       extension->version()->CompareTo(extension_sync_data.version()) : 0;
 
   // If the target extension has already been installed ephemerally, it can
@@ skipping 53 matching lines @@
       app_sync_bundle_.SyncChangeIfNeeded(extension);
     else if (extension_service_->is_ready() && !flare_.is_null())
       flare_.Run(syncer::APPS);
   } else if (extensions::util::ShouldSyncExtension(&extension, profile_)) {
     if (extension_sync_bundle_.IsSyncing())
       extension_sync_bundle_.SyncChangeIfNeeded(extension);
     else if (extension_service_->is_ready() && !flare_.is_null())
       flare_.Run(syncer::EXTENSIONS);
   }
 }