Sync the launch type pref for apps.

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/threading/sequenced_worker_pool.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/extensions/app_sync_data.h"
 #include "chrome/browser/extensions/extension_error_ui.h"
 #include "chrome/browser/extensions/extension_prefs.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_sync_data.h"
 #include "chrome/browser/extensions/extension_sync_service_factory.h"
 #include "chrome/browser/extensions/extension_util.h"
+#include "chrome/browser/extensions/launch_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/glue/sync_start_util.h"
 #include "chrome/browser/sync/sync_prefs.h"
 #include "chrome/common/extensions/sync_helper.h"
 #include "content/public/browser/browser_thread.h"
 #include "extensions/browser/app_sorting.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/feature_switch.h"
 #include "extensions/common/manifest_constants.h"
 #include "sync/api/sync_change.h"
@@ skipping 203 matching lines @@
       extension_util::IsIncognitoEnabled(extension.id(), extension_service_));
 }
 
 extensions::AppSyncData ExtensionSyncService::GetAppSyncData(
     const Extension& extension) const {
   return extensions::AppSyncData(
       extension,
       extension_service_->IsExtensionEnabled(extension.id()),
       extension_util::IsIncognitoEnabled(extension.id(), extension_service_),
       extension_prefs_->app_sorting()->GetAppLaunchOrdinal(extension.id()),
-      extension_prefs_->app_sorting()->GetPageOrdinal(extension.id()));
+      extension_prefs_->app_sorting()->GetPageOrdinal(extension.id()),
+      extensions::HasPreferredLaunchType(extension_prefs_, &extension)
+          ? extensions::GetLaunchType(extension_prefs_, &extension)
+          : extensions::LAUNCH_TYPE_INVALID);
+
 }
 
 std::vector<extensions::ExtensionSyncData>
   ExtensionSyncService::GetExtensionSyncDataList() const {
   std::vector<extensions::ExtensionSyncData> extension_sync_list;
   extension_sync_bundle_.GetExtensionSyncDataListHelper(
       extension_service_->extensions(), &extension_sync_list);
   extension_sync_bundle_.GetExtensionSyncDataListHelper(
       extension_service_->disabled_extensions(), &extension_sync_list);
   extension_sync_bundle_.GetExtensionSyncDataListHelper(
@@ skipping 47 matching lines @@
   if (app_sync_data.app_launch_ordinal().IsValid() &&
       app_sync_data.page_ordinal().IsValid()) {
     extension_prefs_->app_sorting()->SetAppLaunchOrdinal(
         id,
         app_sync_data.app_launch_ordinal());
     extension_prefs_->app_sorting()->SetPageOrdinal(
         id,
         app_sync_data.page_ordinal());
   }
 
+  if (app_sync_data.launch_type() >= extensions::LAUNCH_TYPE_FIRST &&
+      app_sync_data.launch_type() < extensions::NUM_LAUNCH_TYPES) {

[Nicolas Zea, 2013/12/19 19:57:01]

I suspect this is actually going to result in automatically overwriting the sync
data if it contains a value that the local version doesn't understand (launch
type > num_launch_types).

In other datatypes we typically have a scoped value that's true in
ProcessSyncChanges, which prevents triggering the sync code from sync-initiated
changes. From what I can tell extensions/apps don't have this, but it might be
they should (i.e. return immediately in SyncExtensionChangeIfNeeded if the value
is set).

Otherwise what happens is that, as soon as any change comes in, the extension
update code gets triggered, which itself calls back into sync, but generates the
sync data from the in-memory representation, which won't match for launch_type.

Some unit tests that verify the future launch type case would probably be
useful.

[calamity, 2013/12/30 06:13:32]

Ack, it's tricky to test this case because there are so many checks around it
not happening. In particular, since we're using an enum for the LaunchType in
AppSpecifics, I can't even sync an invalid value to test this case.

Currently, the system will be robust to down-syncing bad values because protobuf
will treat the invalid value as an unknown message as outlined at
https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#enum.

The ExtensionSyncService ignores invalid values and doesn't trigger a resync.

I wrote a test that processes an invalid AppSyncData. Let me know if you can
think of anything else.

+    extensions::SetLaunchType(extension_service_, id,
+        static_cast<extensions::LaunchType>(app_sync_data.launch_type()));
+  }
+
   if (!ProcessExtensionSyncDataHelper(app_sync_data.extension_sync_data(),
                                       syncer::APPS)) {
     app_sync_bundle_.AddPendingApp(id, app_sync_data);
     extension_service_->CheckForUpdatesSoon();
     return false;
   }
 
   return true;
 }
 
@@ skipping 122 matching lines @@
       app_sync_bundle_.SyncChangeIfNeeded(extension);
     else if (extension_service_->is_ready() && !flare_.is_null())
       flare_.Run(syncer::APPS);
   } else if (extensions::sync_helper::IsSyncableExtension(&extension)) {
     if (extension_sync_bundle_.IsSyncing())
       extension_sync_bundle_.SyncChangeIfNeeded(extension);
     else if (extension_service_->is_ready() && !flare_.is_null())
       flare_.Run(syncer::EXTENSIONS);
   }
 }