Create and remove sync nodes whenever preferences flip their managed flag

BUG=none
TEST=none

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

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

5 files changed, 167 insertions, 102 deletions

diff --git a/chrome/browser/sync/glue/preference_change_processor.cc b/chrome/browser/sync/glue/preference_change_processor.cc
index 76338d1..ec0eeb3 100644
--- a/chrome/browser/sync/glue/preference_change_processor.cc
+++ b/chrome/browser/sync/glue/preference_change_processor.cc
@@ -7,6 +7,7 @@
 #include <set>
 #include <string>
 
+#include "base/auto_reset.h"
 #include "base/json/json_reader.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/chrome_thread.h"
@@ -25,7 +26,8 @@ PreferenceChangeProcessor::PreferenceChangeProcessor(
     UnrecoverableErrorHandler* error_handler)
     : ChangeProcessor(error_handler),
       pref_service_(NULL),
-      model_associator_(model_associator) {
+      model_associator_(model_associator),
+      processing_pref_change_(false) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(model_associator);
   DCHECK(error_handler);
@@ -43,27 +45,31 @@ void PreferenceChangeProcessor::Observe(NotificationType type,
   DCHECK(NotificationType::PREF_CHANGED == type);
   DCHECK_EQ(pref_service_, Source<PrefService>(source).ptr());
 
+  // Avoid recursion.
+  if (processing_pref_change_)
+    return;
+
+  AutoReset<bool> guard(&processing_pref_change_, true);
   std::string* name = Details<std::string>(details).ptr();
   const PrefService::Preference* preference =
       pref_service_->FindPreference((*name).c_str());
   DCHECK(preference);
-
-  // TODO(mnissler): Detect preference->IsUserControlled() state changes here
-  // and call into PreferenceModelAssociator to associate/disassociate sync
-  // nodes when the state changes.
-
-  // Do not pollute sync data with values coming from policy, extensions or the
-  // commandline.
-  if (!preference->IsUserModifiable())
+  int64 sync_id = model_associator_->GetSyncIdFromChromeId(*name);
+  bool user_modifiable = preference->IsUserModifiable();
+  if (!user_modifiable) {
+    // We do not track preferences the user cannot change.
+    model_associator_->Disassociate(sync_id);
     return;
+  }
 
   sync_api::WriteTransaction trans(share_handle());
   sync_api::WriteNode node(&trans);
 
-  // Since we don't create sync nodes for preferences that still have
-  // their default values, this changed preference may not have a sync
-  // node yet.  If not, create it.
-  int64 sync_id = model_associator_->GetSyncIdFromChromeId(*name);
+  // Since we don't create sync nodes for preferences that are not under control
+  // of the user or still have their default value, this changed preference may
+  // not have a sync node yet. If so, we create a node. Similarly, a preference
+  // may become user-modifiable (e.g. due to laxer policy configuration), in
+  // which case we also need to create a sync node and associate it.
   if (sync_api::kInvalidId == sync_id) {
     sync_api::ReadNode root(&trans);
     if (!root.InitByTagLookup(browser_sync::kPreferencesTag)) {
@@ -71,20 +77,21 @@ void PreferenceChangeProcessor::Observe(NotificationType type,
       return;
     }
 
-    if (!node.InitUniqueByCreation(syncable::PREFERENCES, root, *name)) {
-      error_handler()->OnUnrecoverableError(
-          FROM_HERE,
-          "Failed to create preference sync node.");
-      return;
-    }
-
-    model_associator_->Associate(preference, node.GetId());
-  } else {
-    if (!node.InitByIdLookup(sync_id)) {
+    // InitPrefNodeAndAssociate takes care of writing the value to the sync
+    // node if appropriate.
+    if (!model_associator_->InitPrefNodeAndAssociate(&trans,
+                                                     root,
+                                                     preference)) {
       error_handler()->OnUnrecoverableError(FROM_HERE,
-                                            "Preference node lookup failed.");
-      return;
+                                            "Can't create sync node.");
     }
+    return;
+  }
+
+  if (!node.InitByIdLookup(sync_id)) {
+    error_handler()->OnUnrecoverableError(FROM_HERE,
+                                          "Preference node lookup failed.");
+    return;
   }
 
   if (!PreferenceModelAssociator::WritePreferenceToNode(
diff --git a/chrome/browser/sync/glue/preference_change_processor.h b/chrome/browser/sync/glue/preference_change_processor.h
index ae02708..93c4f6a 100644
--- a/chrome/browser/sync/glue/preference_change_processor.h
+++ b/chrome/browser/sync/glue/preference_change_processor.h
@@ -58,6 +58,9 @@ class PreferenceChangeProcessor : public ChangeProcessor,
   // The two models should be associated according to this ModelAssociator.
   PreferenceModelAssociator* model_associator_;
 
+  // Whether we are currently processing a preference change notification.
+  bool processing_pref_change_;
+
   DISALLOW_COPY_AND_ASSIGN(PreferenceChangeProcessor);
 };
 
diff --git a/chrome/browser/sync/glue/preference_model_associator.cc b/chrome/browser/sync/glue/preference_model_associator.cc
index 013696b..86c6db4 100644
--- a/chrome/browser/sync/glue/preference_model_associator.cc
+++ b/chrome/browser/sync/glue/preference_model_associator.cc
@@ -43,6 +43,68 @@ PreferenceModelAssociator::~PreferenceModelAssociator() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 }
 
+bool PreferenceModelAssociator::InitPrefNodeAndAssociate(
+    sync_api::WriteTransaction* trans,
+    const sync_api::BaseNode& root,
+    const PrefService::Preference* pref) {
+  DCHECK(pref);
+
+  PrefService* pref_service = sync_service_->profile()->GetPrefs();
+  base::JSONReader reader;
+  std::string tag = pref->name();
+  sync_api::WriteNode node(trans);
+  if (node.InitByClientTagLookup(syncable::PREFERENCES, tag)) {
+    // The server has a value for the preference.
+    const sync_pb::PreferenceSpecifics& preference(
+        node.GetPreferenceSpecifics());
+    DCHECK_EQ(tag, preference.name());
+
+    if (pref->IsUserModifiable()) {
+      scoped_ptr<Value> value(
+          reader.JsonToValue(preference.value(), false, false));
+      std::string pref_name = preference.name();
+      if (!value.get()) {
+        LOG(ERROR) << "Failed to deserialize preference value: "
+                   << reader.GetErrorMessage();
+        return false;
+      }
+
+      // Merge the server value of this preference with the local value.
+      scoped_ptr<Value> new_value(MergePreference(*pref, *value));
+
+      // Update the local preference based on what we got from the
+      // sync server.
+      if (!pref->GetValue()->Equals(new_value.get()))
+        pref_service->Set(pref_name.c_str(), *new_value);
+
+      AfterUpdateOperations(pref_name);
+
+      // If the merge resulted in an updated value, write it back to
+      // the sync node.
+      if (!value->Equals(new_value.get()) &&
+          !WritePreferenceToNode(pref->name(), *new_value, &node))
+        return false;
+    }
+    Associate(pref, node.GetId());
+  } else if (pref->IsUserControlled()) {
+    // The server doesn't have a value, but we have a user-controlled value,
+    // so we push it to the server.
+    sync_api::WriteNode write_node(trans);
+    if (!write_node.InitUniqueByCreation(syncable::PREFERENCES, root, tag)) {
+      LOG(ERROR) << "Failed to create preference sync node.";
+      return false;
+    }
+
+    // Update the sync node with the local value for this preference.
+    if (!WritePreferenceToNode(pref->name(), *pref->GetValue(), &write_node))
+      return false;
+
+    Associate(pref, write_node.GetId());
+  }
+
+  return true;
+}
+
 bool PreferenceModelAssociator::AssociateModels() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   PrefService* pref_service = sync_service_->profile()->GetPrefs();
@@ -63,63 +125,11 @@ bool PreferenceModelAssociator::AssociateModels() {
     return false;
   }
 
-  base::JSONReader reader;
   for (std::set<std::string>::iterator it = synced_preferences_.begin();
        it != synced_preferences_.end(); ++it) {
-    const std::string& tag = *it;
     const PrefService::Preference* pref =
-        pref_service->FindPreference(tag.c_str());
-    DCHECK(pref);
-
-    sync_api::WriteNode node(&trans);
-    if (node.InitByClientTagLookup(syncable::PREFERENCES, tag)) {
-      // The server has a value for the preference.
-      const sync_pb::PreferenceSpecifics& preference(
-          node.GetPreferenceSpecifics());
-      DCHECK_EQ(tag, preference.name());
-
-      if (pref->IsUserModifiable()) {
-        scoped_ptr<Value> value(
-            reader.JsonToValue(preference.value(), false, false));
-        std::string pref_name = preference.name();
-        if (!value.get()) {
-          LOG(ERROR) << "Failed to deserialize preference value: "
-                     << reader.GetErrorMessage();
-          return false;
-        }
-
-        // Merge the server value of this preference with the local value.
-        scoped_ptr<Value> new_value(MergePreference(*pref, *value));
-
-        // Update the local preference based on what we got from the
-        // sync server.
-        if (!pref->GetValue()->Equals(new_value.get()))
-          pref_service->Set(pref_name.c_str(), *new_value);
-
-        AfterUpdateOperations(pref_name);
-
-        // If the merge resulted in an updated value, write it back to
-        // the sync node.
-        if (!value->Equals(new_value.get()) &&
-            !WritePreferenceToNode(pref->name(), *new_value, &node))
-          return false;
-      }
-      Associate(pref, node.GetId());
-    } else if (pref->IsUserControlled()) {
-      // The server doesn't have a value, but we have a user-controlled value,
-      // so we push it to the server.
-      sync_api::WriteNode write_node(&trans);
-      if (!write_node.InitUniqueByCreation(syncable::PREFERENCES, root, tag)) {
-        LOG(ERROR) << "Failed to create preference sync node.";
-        return false;
-      }
-
-      // Update the sync node with the local value for this preference.
-      if (!WritePreferenceToNode(pref->name(), *pref->GetValue(), &write_node))
-        return false;
-
-      Associate(pref, write_node.GetId());
-    }
+        pref_service->FindPreference((*it).c_str());
+    InitPrefNodeAndAssociate(&trans, root, pref);
   }
   return true;
 }
diff --git a/chrome/browser/sync/glue/preference_model_associator.h b/chrome/browser/sync/glue/preference_model_associator.h
index fd2915c..0b3bfaa 100644
--- a/chrome/browser/sync/glue/preference_model_associator.h
+++ b/chrome/browser/sync/glue/preference_model_associator.h
@@ -21,6 +21,7 @@ class Value;
 
 namespace sync_api {
 class WriteNode;
+class WriteTransaction;
 }
 
 namespace browser_sync {
@@ -46,6 +47,12 @@ class PreferenceModelAssociator
     return synced_preferences_;
   }
 
+  // Create an association for a given preference. A sync node is created if
+  // necessary and the value is read from or written to the node as appropriate.
+  bool InitPrefNodeAndAssociate(sync_api::WriteTransaction* trans,
+                                const sync_api::BaseNode& root,
+                                const PrefService::Preference* pref);
+
   // PerDataTypeAssociatorInterface implementation.
   //
   // Iterates through the sync model looking for matched pairs of items.
diff --git a/chrome/browser/sync/profile_sync_service_preference_unittest.cc b/chrome/browser/sync/profile_sync_service_preference_unittest.cc
index 454d56c..52e832b 100644
--- a/chrome/browser/sync/profile_sync_service_preference_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_preference_unittest.cc
@@ -7,8 +7,8 @@
 
 #include "base/json/json_reader.h"
 #include "base/stl_util-inl.h"
+#include "base/string_piece.h"
 #include "base/task.h"
-#include "base/utf_string_conversions.h"  // TODO(viettrungluu): remove
 #include "chrome/browser/sync/abstract_profile_sync_service_test.h"
 #include "chrome/browser/sync/engine/syncapi.h"
 #include "chrome/browser/sync/glue/preference_change_processor.h"
@@ -117,39 +117,33 @@ class ProfileSyncServicePreferenceTest
     return reader.JsonToValue(specifics.value(), false, false);
   }
 
+  int64 WriteSyncedValue(sync_api::WriteNode& node,
+                         const std::string& name,
+                         const Value& value) {
+    if (!PreferenceModelAssociator::WritePreferenceToNode(name, value, &node))
+      return sync_api::kInvalidId;
+    return node.GetId();
+  }
+
   int64 SetSyncedValue(const std::string& name, const Value& value) {
     sync_api::WriteTransaction trans(backend()->GetUserShareHandle());
     sync_api::ReadNode root(&trans);
     if (!root.InitByTagLookup(browser_sync::kPreferencesTag))
       return sync_api::kInvalidId;
 
+    sync_api::WriteNode tag_node(&trans);
     sync_api::WriteNode node(&trans);
 
     int64 node_id = model_associator_->GetSyncIdFromChromeId(name);
     if (node_id == sync_api::kInvalidId) {
-      if (!node.InitUniqueByCreation(syncable::PREFERENCES,
-                                     root,
-                                     name)) {
-        return sync_api::kInvalidId;
-      }
-    } else {
-      if (!node.InitByIdLookup(node_id)) {
-        return sync_api::kInvalidId;
-      }
+      if (tag_node.InitByClientTagLookup(syncable::PREFERENCES, name))
+        return WriteSyncedValue(tag_node, name, value);
+      if (node.InitUniqueByCreation(syncable::PREFERENCES, root, name))
+        return WriteSyncedValue(node, name, value);
+    } else if (node.InitByIdLookup(node_id)) {
+      return WriteSyncedValue(node, name, value);
     }
-
-    std::string serialized;
-    JSONStringValueSerializer json(&serialized);
-    EXPECT_TRUE(json.Serialize(value));
-
-    sync_pb::PreferenceSpecifics preference;
-    preference.set_name(name);
-    preference.set_value(serialized);
-    node.SetPreferenceSpecifics(preference);
-    // TODO(viettrungluu): remove conversion and header
-    node.SetTitle(UTF8ToWide(name));
-
-    return node.GetId();
+    return sync_api::kInvalidId;
   }
 
   SyncManager::ChangeRecord* MakeChangeRecord(const std::string& name,
@@ -447,3 +441,47 @@ TEST_F(ProfileSyncServicePreferenceTest, ManagedPreferences) {
   EXPECT_TRUE(user_value->Equals(
       prefs_->GetUserPref(prefs::kHomePage)));
 }
+
+TEST_F(ProfileSyncServicePreferenceTest, DynamicManagedPreferences) {
+  CreateRootTask task(this, syncable::PREFERENCES);
+  ASSERT_TRUE(StartSyncService(&task, false));
+  ASSERT_TRUE(task.success());
+
+  scoped_ptr<Value> initial_value(
+      Value::CreateStringValue("http://example.com/initial"));
+  profile_->GetPrefs()->Set(prefs::kHomePage, *initial_value);
+  scoped_ptr<const Value> actual(GetSyncedValue(prefs::kHomePage));
+  EXPECT_TRUE(initial_value->Equals(actual.get()));
+
+  // Switch kHomePage to managed and set a different value.
+  scoped_ptr<Value> managed_value(
+      Value::CreateStringValue("http://example.com/managed"));
+  profile_->GetPrefs()->SetManagedPref(prefs::kHomePage,
+                                       managed_value->DeepCopy());
+
+  // Sync node should be gone.
+  EXPECT_EQ(sync_api::kInvalidId,
+            model_associator_->GetSyncIdFromChromeId(prefs::kHomePage));
+
+  // Change the sync value.
+  scoped_ptr<Value> sync_value(
+      Value::CreateStringValue("http://example.com/sync"));
+  int64 node_id = SetSyncedValue(prefs::kHomePage, *sync_value);
+  ASSERT_NE(node_id, sync_api::kInvalidId);
+  scoped_ptr<SyncManager::ChangeRecord> record(new SyncManager::ChangeRecord);
+  record->action = SyncManager::ChangeRecord::ACTION_ADD;
+  record->id = node_id;
+  {
+    sync_api::WriteTransaction trans(backend()->GetUserShareHandle());
+    change_processor_->ApplyChangesFromSyncModel(&trans, record.get(), 1);
+  }
+
+  // The pref value should still be the one dictated by policy.
+  EXPECT_TRUE(managed_value->Equals(&GetPreferenceValue(prefs::kHomePage)));
+
+  // Switch kHomePage back to unmanaged.
+  profile_->GetPrefs()->RemoveManagedPref(prefs::kHomePage);
+
+  // Sync value should be picked up.
+  EXPECT_TRUE(sync_value->Equals(&GetPreferenceValue(prefs::kHomePage)));
+}