Theme sync: Delay the creation of the theme sync node until the user switches

away from the default theme. This is to avoid the server-wins logic from
overwriting custom themes for new sync clients. With this patch, the first
non-default theme that gets synced will be propagated to all clients.

BUG=48155
TEST=manually tested

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

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

4 files changed, 56 insertions, 33 deletions

diff --git a/chrome/browser/sync/glue/theme_change_processor.cc b/chrome/browser/sync/glue/theme_change_processor.cc
index 58c3df3..2ca8a238 100644
--- a/chrome/browser/sync/glue/theme_change_processor.cc
+++ b/chrome/browser/sync/glue/theme_change_processor.cc
@@ -5,6 +5,7 @@
 #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"
@@ -23,6 +24,19 @@ 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(
@@ -110,25 +124,33 @@ void ThemeChangeProcessor::Observe(NotificationType type,
 
   sync_api::WriteTransaction trans(share_handle());
   sync_api::WriteNode node(&trans);
-  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);
+  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);
   }
-  return;
 }
 
 void ThemeChangeProcessor::ApplyChangesFromSyncModel(
@@ -155,12 +177,6 @@ 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 a20e669..9280c13 100644
--- a/chrome/browser/sync/glue/theme_model_associator.cc
+++ b/chrome/browser/sync/glue/theme_model_associator.cc
@@ -8,6 +8,7 @@
 #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"
@@ -18,9 +19,6 @@ 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.";
@@ -56,8 +54,10 @@ bool ThemeModelAssociator::AssociateModels() {
     // sync data.
     SetCurrentThemeFromThemeSpecificsIfNecessary(
         node.GetThemeSpecifics(), profile);
-  } else {
-    // Set the sync data from the current theme.
+  } else if (profile->GetTheme() || (UseSystemTheme(profile) &&
+                                     IsSystemThemeDistinctFromDefaultTheme())) {
+    // Set the sync data from the current theme, iff the current theme isn't the
+    // default.
     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 1bd078e..4ed596f 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";
-
-namespace {
+const char kCurrentThemeNodeTitle[] = "Current Theme";
+const char kThemesTag[] = "google_chrome_themes";
 
 bool IsSystemThemeDistinctFromDefaultTheme() {
 #if defined(TOOLKIT_USES_GTK)
@@ -45,8 +45,6 @@ bool UseSystemTheme(Profile* profile) {
 #endif
 }
 
-}  // namespace
-
 bool AreThemeSpecificsEqual(const sync_pb::ThemeSpecifics& a,
                             const sync_pb::ThemeSpecifics& b) {
   return AreThemeSpecificsEqualHelper(
diff --git a/chrome/browser/sync/glue/theme_util.h b/chrome/browser/sync/glue/theme_util.h
index 2821663..0f70a50 100644
--- a/chrome/browser/sync/glue/theme_util.h
+++ b/chrome/browser/sync/glue/theme_util.h
@@ -15,6 +15,15 @@ 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,