If the default theme is synced to the server, make new sync clients with custom themes "win" at model association time.

BUG=48155
TEST=manually tested

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

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

4 files changed, 58 insertions, 59 deletions

diff --git a/chrome/browser/sync/glue/theme_change_processor.cc b/chrome/browser/sync/glue/theme_change_processor.cc
index 2ca8a238..58c3df3 100644
--- a/chrome/browser/sync/glue/theme_change_processor.cc
+++ b/chrome/browser/sync/glue/theme_change_processor.cc
@@ -5,7 +5,6 @@
 #include "chrome/browser/sync/glue/theme_change_processor.h"
 
 #include "base/logging.h"
-#include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_theme_provider.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/sync/engine/syncapi.h"
@@ -24,19 +23,6 @@ std::string GetThemeId(Extension* current_theme) {
   }
   return current_theme ? current_theme->id() : "default/system";
 }
-
-void UpdateThemeSyncNode(Profile* profile, sync_api::WriteNode* node) {
-  sync_pb::ThemeSpecifics old_theme_specifics = node->GetThemeSpecifics();
-  // Make sure to base new_theme_specifics on old_theme_specifics so
-  // we preserve the state of use_system_theme_by_default.
-  sync_pb::ThemeSpecifics new_theme_specifics = old_theme_specifics;
-  GetThemeSpecificsFromCurrentTheme(profile, &new_theme_specifics);
-  // Do a write only if something actually changed so as to guard
-  // against cycles.
-  if (!AreThemeSpecificsEqual(old_theme_specifics, new_theme_specifics)) {
-    node->SetThemeSpecifics(new_theme_specifics);
-  }
-}
 }  // namespace
 
 ThemeChangeProcessor::ThemeChangeProcessor(
@@ -124,33 +110,25 @@ void ThemeChangeProcessor::Observe(NotificationType type,
 
   sync_api::WriteTransaction trans(share_handle());
   sync_api::WriteNode node(&trans);
-  if (node.InitByClientTagLookup(syncable::THEMES,
-                                 kCurrentThemeClientTag)) {
-    UpdateThemeSyncNode(profile_, &node);
-  } else if (profile_->GetTheme() ||
-             (UseSystemTheme(profile_) &&
-              IsSystemThemeDistinctFromDefaultTheme())) {
-    // The sync node might not exist yet if this is the first time the user
-    // switches away from the default theme.
-    sync_api::ReadNode root(&trans);
-    if (!root.InitByTagLookup(kThemesTag)) {
-      std::string err = "Server did not create the top-level themes node. We "
-                        "might be running against an out-of-date server.";
-      error_handler()->OnUnrecoverableError(FROM_HERE, err);
-      return;
-    }
-    sync_api::WriteNode new_node(&trans);
-    if (!new_node.InitUniqueByCreation(syncable::THEMES, root,
-                                       kCurrentThemeClientTag)) {
-      std::string err = "Could not create node with client tag: ";
-      error_handler()->OnUnrecoverableError(FROM_HERE,
-                                            err + kCurrentThemeClientTag);
-      return;
-    }
-    new_node.SetIsFolder(false);
-    new_node.SetTitle(UTF8ToWide(kCurrentThemeNodeTitle));
-    UpdateThemeSyncNode(profile_, &new_node);
+  if (!node.InitByClientTagLookup(syncable::THEMES,
+                                  kCurrentThemeClientTag)) {
+    std::string err = "Could not create node with client tag: ";
+    error_handler()->OnUnrecoverableError(FROM_HERE,
+                                          err + kCurrentThemeClientTag);
+    return;
   }
+
+  sync_pb::ThemeSpecifics old_theme_specifics = node.GetThemeSpecifics();
+  // Make sure to base new_theme_specifics on old_theme_specifics so
+  // we preserve the state of use_system_theme_by_default.
+  sync_pb::ThemeSpecifics new_theme_specifics = old_theme_specifics;
+  GetThemeSpecificsFromCurrentTheme(profile_, &new_theme_specifics);
+  // Do a write only if something actually changed so as to guard
+  // against cycles.
+  if (!AreThemeSpecificsEqual(old_theme_specifics, new_theme_specifics)) {
+    node.SetThemeSpecifics(new_theme_specifics);
+  }
+  return;
 }
 
 void ThemeChangeProcessor::ApplyChangesFromSyncModel(
@@ -177,6 +155,12 @@ void ThemeChangeProcessor::ApplyChangesFromSyncModel(
   }
   const sync_api::SyncManager::ChangeRecord& change =
       changes[change_count - 1];
+  if (change.action != sync_api::SyncManager::ChangeRecord::ACTION_UPDATE &&
+      change.action != sync_api::SyncManager::ChangeRecord::ACTION_DELETE) {
+    std::string err = "strange theme change.action " + change.action;
+    error_handler()->OnUnrecoverableError(FROM_HERE, err);
+    return;
+  }
   sync_pb::ThemeSpecifics theme_specifics;
   // If the action is a delete, simply use the default values for
   // ThemeSpecifics, which would cause the default theme to be set.
diff --git a/chrome/browser/sync/glue/theme_model_associator.cc b/chrome/browser/sync/glue/theme_model_associator.cc
index 9280c13..3936f62 100644
--- a/chrome/browser/sync/glue/theme_model_associator.cc
+++ b/chrome/browser/sync/glue/theme_model_associator.cc
@@ -8,7 +8,6 @@
 #include "base/logging.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_theme_provider.h"
-#include "chrome/browser/profile.h"
 #include "chrome/browser/sync/engine/syncapi.h"
 #include "chrome/browser/sync/glue/sync_backend_host.h"
 #include "chrome/browser/sync/glue/theme_util.h"
@@ -19,6 +18,9 @@ namespace browser_sync {
 
 namespace {
 
+static const char kThemesTag[] = "google_chrome_themes";
+static const char kCurrentThemeNodeTitle[] = "Current Theme";
+
 static const char kNoThemesFolderError[] =
     "Server did not create the top-level themes node. We "
     "might be running against an out-of-date server.";
@@ -43,7 +45,7 @@ bool ThemeModelAssociator::AssociateModels() {
   }
 
   Profile* profile = sync_service_->profile();
-  sync_api::ReadNode node(&trans);
+  sync_api::WriteNode node(&trans);
   // TODO(akalin): When we have timestamps, we may want to do
   // something more intelligent than preferring the sync data over our
   // local data.
@@ -52,12 +54,12 @@ bool ThemeModelAssociator::AssociateModels() {
     // TODO(akalin): If the sync data does not have
     // use_system_theme_by_default and we do, update that flag on the
     // sync data.
-    SetCurrentThemeFromThemeSpecificsIfNecessary(
-        node.GetThemeSpecifics(), profile);
-  } else if (profile->GetTheme() || (UseSystemTheme(profile) &&
-                                     IsSystemThemeDistinctFromDefaultTheme())) {
-    // Set the sync data from the current theme, iff the current theme isn't the
-    // default.
+    sync_pb::ThemeSpecifics theme_specifics = node.GetThemeSpecifics();
+    if (UpdateThemeSpecificsOrSetCurrentThemeIfNecessary(profile,
+                                                         &theme_specifics))
+      node.SetThemeSpecifics(theme_specifics);
+  } else {
+    // Set the sync data from the current theme.
     sync_api::WriteNode node(&trans);
     if (!node.InitUniqueByCreation(syncable::THEMES, root,
                                    kCurrentThemeClientTag)) {
diff --git a/chrome/browser/sync/glue/theme_util.cc b/chrome/browser/sync/glue/theme_util.cc
index 4ed596f..d4ed722 100644
--- a/chrome/browser/sync/glue/theme_util.cc
+++ b/chrome/browser/sync/glue/theme_util.cc
@@ -26,8 +26,8 @@
 namespace browser_sync {
 
 const char kCurrentThemeClientTag[] = "current_theme";
-const char kCurrentThemeNodeTitle[] = "Current Theme";
-const char kThemesTag[] = "google_chrome_themes";
+
+namespace {
 
 bool IsSystemThemeDistinctFromDefaultTheme() {
 #if defined(TOOLKIT_USES_GTK)
@@ -45,6 +45,8 @@ bool UseSystemTheme(Profile* profile) {
 #endif
 }
 
+}  // namespace
+
 bool AreThemeSpecificsEqual(const sync_pb::ThemeSpecifics& a,
                             const sync_pb::ThemeSpecifics& b) {
   return AreThemeSpecificsEqualHelper(
@@ -152,6 +154,19 @@ void SetCurrentThemeFromThemeSpecifics(
   }
 }
 
+bool UpdateThemeSpecificsOrSetCurrentThemeIfNecessary(
+    Profile* profile, sync_pb::ThemeSpecifics* theme_specifics) {
+  if (!theme_specifics->use_custom_theme() &&
+      (profile->GetTheme() || (UseSystemTheme(profile) &&
+                               IsSystemThemeDistinctFromDefaultTheme()))) {
+    GetThemeSpecificsFromCurrentTheme(profile, theme_specifics);
+    return true;
+  } else {
+    SetCurrentThemeFromThemeSpecificsIfNecessary(*theme_specifics, profile);
+    return false;
+  }
+}
+
 void GetThemeSpecificsFromCurrentTheme(
     Profile* profile,
     sync_pb::ThemeSpecifics* theme_specifics) {
diff --git a/chrome/browser/sync/glue/theme_util.h b/chrome/browser/sync/glue/theme_util.h
index 0f70a50..1cc6f39 100644
--- a/chrome/browser/sync/glue/theme_util.h
+++ b/chrome/browser/sync/glue/theme_util.h
@@ -15,15 +15,6 @@ class ThemeSpecifics;
 namespace browser_sync {
 
 extern const char kCurrentThemeClientTag[];
-extern const char kCurrentThemeNodeTitle[];
-extern const char kThemesTag[];
-
-// Returns whether the the system them (eg. GTK) is different from the default
-// theme.
-bool IsSystemThemeDistinctFromDefaultTheme();
-
-// Returns whether |profile| is configured to use the system theme.
-bool UseSystemTheme(Profile* profile);
 
 // Returns true iff two ThemeSpecifics indicate the same theme.
 bool AreThemeSpecificsEqual(const sync_pb::ThemeSpecifics& a,
@@ -59,6 +50,13 @@ void GetThemeSpecificsFromCurrentThemeHelper(
 void SetCurrentThemeFromThemeSpecificsIfNecessary(
     const sync_pb::ThemeSpecifics& theme_specifics, Profile* profile);
 
+// Like SetCurrentThemeFromThemeSpecifics() except that in the case where
+// |theme_specifics| uses the default theme and |profile| does not, the local
+// data "wins" against the server's and |theme_specifics| is updated with the
+// custom theme. Returns true iff |theme_specifics| is updated.
+bool UpdateThemeSpecificsOrSetCurrentThemeIfNecessary(
+    Profile* profile, sync_pb::ThemeSpecifics* theme_specifics);
+
 }  // namespace browser_sync
 
 #endif  // CHROME_BROWSER_SYNC_GLUE_THEME_UTIL_H_