[Sync] Add support for automatic enabling of syncing tab favicons.

We add the sync_tab_favicons field to the nigori node and add support
for automatically enabling the feature when we receive a new nigori node.
Once we do enable the feature, the browser will only start writing favicons
after the next restart.

BUG=92728
TEST=using python testserver to enable sync tab favicons, then restarting.


Review URL: http://codereview.chromium.org/10235013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@134184 0039d316-1c4b-4281-b951-d872f2087c98

chrome/app/generated_resources.grd

@@ -5282,6 +5282,12 @@ Keep your key file in a safe place. You will need it to create new versions of y
       <message name="IDS_FLAGS_SYNC_TABS_DESCRIPTION" desc="Description for the flag to enable syncing the open tabs datatype">
         Enable open tabs in the sync settings. This allows syncing your open tabs to other clients.
       </message>
+      <message name="IDS_FLAGS_SYNC_TAB_FAVICONS_NAME" desc="Title for the flag to enable syncing tab favicons with tab sync">
+        Enable tab favicon sync.
+      </message>
+      <message name="IDS_FLAGS_SYNC_TAB_FAVICONS_DESCRIPTION" desc="Description for the flag to enable syncing tab favicons with tab sync">
+        Enable syncing the favicons of open tabs as part of Open Tab sync.
+      </message>
       <message name="IDS_FLAGS_SYNC_APP_NOTIFICATIONS_NAME" desc="Title for the flag to disable syncing the app notifications datatype">
         Disable syncing app notifications
       </message>

chrome/browser/about_flags.cc

@@ -350,6 +350,13 @@ const Experiment kExperiments[] = {
     kOsAll,
     SINGLE_VALUE_TYPE(switches::kEnableSyncTabs)
   },
+  {
+    "sync-tab-favicons",
+    IDS_FLAGS_SYNC_TAB_FAVICONS_NAME,
+    IDS_FLAGS_SYNC_TAB_FAVICONS_DESCRIPTION,
+    kOsAll,
+    SINGLE_VALUE_TYPE(switches::kSyncTabFavicons)
+  },
   {
     "sync-app-notifications",
     IDS_FLAGS_SYNC_APP_NOTIFICATIONS_NAME,

chrome/browser/sync/glue/sync_backend_host.cc

@@ -45,6 +45,7 @@
 #include "sync/notifier/sync_notifier.h"
 #include "sync/protocol/encryption.pb.h"
 #include "sync/protocol/sync.pb.h"
+#include "sync/util/experiments.h"
 #include "sync/util/nigori.h"
 
 static const int kSaveChangesIntervalSeconds = 10;
@@ -1212,9 +1213,9 @@ void SyncBackendHost::Core::SaveChanges() {
 
 void SyncBackendHost::AddExperimentalTypes() {
   CHECK(initialized());
-  syncable::ModelTypeSet to_add;
-  if (core_->sync_manager()->ReceivedExperimentalTypes(&to_add))
-    frontend_->OnDataTypesChanged(to_add);
+  Experiments experiments;
+  if (core_->sync_manager()->ReceivedExperiment(&experiments))
+    frontend_->OnExperimentsChanged(experiments);
 }
 
 void SyncBackendHost::OnNigoriDownloadRetry() {

chrome/browser/sync/glue/sync_backend_host.h

@@ -38,6 +38,7 @@ class Profile;
 namespace browser_sync {
 
 class ChangeProcessor;
+struct Experiments;
 class JsBackend;
 class JsEventHandler;
 class SyncBackendRegistrar;
@@ -121,7 +122,8 @@ class SyncFrontend {
       syncable::ModelTypeSet types) = 0;
 
   // Inform the Frontend that new datatypes are available for registration.
-  virtual void OnDataTypesChanged(syncable::ModelTypeSet to_add) = 0;
+  virtual void OnExperimentsChanged(
+      const browser_sync::Experiments& experiments) = 0;
 
   // Called when the sync cycle returns there is an user actionable error.
   virtual void OnActionableError(

chrome/browser/sync/glue/sync_backend_host_unittest.cc

@@ -17,6 +17,7 @@
 #include "sync/protocol/encryption.pb.h"
 #include "sync/protocol/sync_protocol_error.h"
 #include "sync/syncable/model_type.h"
+#include "sync/util/experiments.h"
 #include "sync/util/test_unrecoverable_error_handler.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -46,7 +47,8 @@ class MockSyncFrontend : public SyncFrontend {
                void(syncable::ModelTypeSet, bool));
   MOCK_METHOD0(OnEncryptionComplete, void());
   MOCK_METHOD1(OnMigrationNeededForTypes, void(syncable::ModelTypeSet));
-  MOCK_METHOD1(OnDataTypesChanged, void(syncable::ModelTypeSet));
+  MOCK_METHOD1(OnExperimentsChanged,
+      void(const browser_sync::Experiments&));
   MOCK_METHOD1(OnActionableError,
       void(const browser_sync::SyncProtocolError& sync_error));
   MOCK_METHOD0(OnSyncConfigureRetry, void());

chrome/browser/sync/profile_sync_service.cc

@@ -61,6 +61,7 @@
 #include "sync/js/js_arg_list.h"
 #include "sync/js/js_event_details.h"
 #include "sync/util/cryptographer.h"
+#include "sync/util/experiments.h"
 #include "ui/base/l10n/l10n_util.h"
 
 using browser_sync::ChangeProcessor;
@@ -707,28 +708,30 @@ void ProfileSyncService::OnSyncCycleCompleted() {
   NotifyObservers();
 }
 
-// TODO(sync): eventually support removing datatypes too.
-void ProfileSyncService::OnDataTypesChanged(
-    syncable::ModelTypeSet to_add) {
+void ProfileSyncService::OnExperimentsChanged(
+    const browser_sync::Experiments& experiments) {
+  if (current_experiments.Matches(experiments))
+    return;
+
   // If this is a first time sync for a client, this will be called before
   // OnBackendInitialized() to ensure the new datatypes are available at sync
   // setup. As a result, the migrator won't exist yet. This is fine because for
   // first time sync cases we're only concerned with making the datatype
   // available.
   if (migrator_.get() &&
       migrator_->state() != browser_sync::BackendMigrator::IDLE) {
-    DVLOG(1) << "Dropping OnDataTypesChanged due to migrator busy.";
+    DVLOG(1) << "Dropping OnExperimentsChanged due to migrator busy.";
     return;
   }
 
-  DVLOG(2) << "OnDataTypesChanged called with types: "
-           << syncable::ModelTypeSetToString(to_add);
-
   const syncable::ModelTypeSet registered_types = GetRegisteredDataTypes();
-
+  syncable::ModelTypeSet to_add;
+  if (experiments.sync_tabs)
+    to_add.Put(syncable::SESSIONS);
   const syncable::ModelTypeSet to_register =
       Difference(to_add, registered_types);
-
+  DVLOG(2) << "OnExperimentsChanged called with types: "
+           << syncable::ModelTypeSetToString(to_add);
   DVLOG(2) << "Enabling types: " << syncable::ModelTypeSetToString(to_register);
 
   for (syncable::ModelTypeSet::Iterator it = to_register.First();
@@ -768,6 +771,16 @@ void ProfileSyncService::OnDataTypesChanged(
       OnMigrationNeededForTypes(to_register);
     }
   }
+
+  // Now enable any non-datatype features.
+  if (experiments.sync_tab_favicons) {
+    DVLOG(1) << "Enabling syncing of tab favicons.";
+    about_flags::SetExperimentEnabled(g_browser_process->local_state(),
+                                      "sync-tab-favicons",
+                                      true);
+  }
+
+  current_experiments = experiments;
 }
 
 void ProfileSyncService::UpdateAuthErrorState(

chrome/browser/sync/profile_sync_service.h

@@ -35,6 +35,7 @@
 #include "sync/engine/model_safe_worker.h"
 #include "sync/js/sync_js_controller.h"
 #include "sync/syncable/model_type.h"
+#include "sync/util/experiments.h"
 #include "sync/util/unrecoverable_error_handler.h"
 
 class Profile;
@@ -235,8 +236,8 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
   virtual void OnEncryptionComplete() OVERRIDE;
   virtual void OnMigrationNeededForTypes(
       syncable::ModelTypeSet types) OVERRIDE;
-  virtual void OnDataTypesChanged(
-      syncable::ModelTypeSet to_add) OVERRIDE;
+  virtual void OnExperimentsChanged(
+      const browser_sync::Experiments& experiments) OVERRIDE;
   virtual void OnActionableError(
       const browser_sync::SyncProtocolError& error) OVERRIDE;
 
@@ -741,6 +742,9 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
   // data types.
   bool setup_in_progress_;
 
+  // The set of currently enabled sync experiments.
+  browser_sync::Experiments current_experiments;
+
   DISALLOW_COPY_AND_ASSIGN(ProfileSyncService);
 };
 

sync/internal_api/sync_manager.cc

@@ -52,6 +52,7 @@
 #include "sync/syncable/model_type_payload_map.h"
 #include "sync/syncable/syncable.h"
 #include "sync/util/cryptographer.h"
+#include "sync/util/experiments.h"
 #include "sync/util/get_session_name.h"
 #include "sync/util/time.h"
 
@@ -2478,19 +2479,24 @@ syncable::ModelTypeSet SyncManager::GetEncryptedDataTypesForTest() const {
   return GetEncryptedTypes(&trans);
 }
 
-bool SyncManager::ReceivedExperimentalTypes(syncable::ModelTypeSet* to_add)
+bool SyncManager::ReceivedExperiment(browser_sync::Experiments* experiments)
     const {
   ReadTransaction trans(FROM_HERE, GetUserShare());
   ReadNode node(&trans);
   if (node.InitByTagLookup(kNigoriTag) != sync_api::BaseNode::INIT_OK) {
     DVLOG(1) << "Couldn't find Nigori node.";
     return false;
   }
+  bool found_experiment = false;
   if (node.GetNigoriSpecifics().sync_tabs()) {
-    to_add->Put(syncable::SESSIONS);
-    return true;
+    experiments->sync_tabs = true;
+    found_experiment = true;
+  }
+  if (node.GetNigoriSpecifics().sync_tab_favicons()) {
+    experiments->sync_tab_favicons = true;
+    found_experiment = true;
   }
-  return false;
+  return found_experiment;
 }
 
 bool SyncManager::HasUnsyncedItems() const {

sync/internal_api/sync_manager.h

@@ -25,6 +25,7 @@
 
 namespace browser_sync {
 class Encryptor;
+struct Experiments;
 class ExtensionsActivityMonitor;
 class JsBackend;
 class JsEventHandler;
@@ -591,10 +592,10 @@ class SyncManager {
   // Note: opens a transaction.  May be called from any thread.
   syncable::ModelTypeSet GetEncryptedDataTypesForTest() const;
 
-  // Reads the nigori node to determine if any experimental types should be
-  // enabled.
+  // Reads the nigori node to determine if any experimental features should
+  // be enabled.
   // Note: opens a transaction.  May be called on any thread.
-  bool ReceivedExperimentalTypes(syncable::ModelTypeSet* to_add) const;
+  bool ReceivedExperiment(browser_sync::Experiments* experiments) const;
 
   // Uses a read-only transaction to determine if the directory being synced has
   // any remaining unsynced items.  May be called on any thread.

sync/protocol/nigori_specifics.proto

@@ -90,5 +90,8 @@ message NigoriSpecifics {
   // User device information. Contains information about each device that has a
   // sync-enabled Chrome browser connected to the user account.
   repeated DeviceInformation device_information = 28;
+
+  // Enable syncing favicons as part of tab sync.
+  optional bool sync_tab_favicons = 29;
 }
 

sync/protocol/proto_value_conversions.cc

@@ -310,6 +310,7 @@ DictionaryValue* NigoriSpecificsToValue(
   SET_BOOL(sync_tabs);
   SET_BOOL(encrypt_everything);
   SET_REP(device_information, DeviceInformationToValue);
+  SET_BOOL(sync_tab_favicons);
   return value;
 }
 

sync/sync.gyp

@@ -157,6 +157,7 @@
         'util/data_type_histogram.h',
         'util/encryptor.h',
         'util/enum_set.h',
+        'util/experiments.h',
         'util/extensions_activity_monitor.cc',
         'util/extensions_activity_monitor.h',
         'util/get_session_name.cc',

sync/util/experiments.h

@@ -0,0 +1,32 @@
+// Copyright (c) 2012 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.
+
+#ifndef SYNC_UTIL_EXPERIMENTS_
+#define SYNC_UTIL_EXPERIMENTS_
+#pragma once
+
+#include "sync/syncable/model_type.h"
+
+namespace browser_sync {
+
+// A structure to hold the enable status of experimental sync features.
+struct Experiments {
+  Experiments() : sync_tabs(false), sync_tab_favicons(false) {}
+
+  bool Matches(const Experiments& rhs) {
+    return (sync_tabs == rhs.sync_tabs) &&
+           (sync_tab_favicons == rhs.sync_tab_favicons);
+  }
+
+  // Enable the tab sync (SESSIONS) datatype.
+  bool sync_tabs;
+
+  // Enable syncing of favicons within tab sync (only has an effect if tab sync
+  // is already enabled). This takes effect on the next restart.
+  bool sync_tab_favicons;
+};
+
+}
+
+#endif  // SYNC_UTIL_EXPERIMENTS_