Avatar syncing for supervised users

chrome/browser/managed_mode/managed_user_sync_service.cc

Index: chrome/browser/managed_mode/managed_user_sync_service.cc
diff --git a/chrome/browser/managed_mode/managed_user_sync_service.cc b/chrome/browser/managed_mode/managed_user_sync_service.cc
index 3728e854e3c9a9359ad7360139822316753a2a32..a2b5081e6c7b71bceafe3ac1b6b57e42c5a0862a 100644
--- a/chrome/browser/managed_mode/managed_user_sync_service.cc
+++ b/chrome/browser/managed_mode/managed_user_sync_service.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/managed_mode/managed_user_sync_service.h"
 
 #include "base/bind.h"
+#include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/profiles/profile.h"
@@ -36,14 +37,19 @@ namespace {
 const char kAcknowledged[] = "acknowledged";
 const char kName[] = "name";
 const char kMasterKey[] = "masterKey";
+const char kAvatar[] = "avatar";

[Bernhard Bauer, 2013/09/04 12:28:33]

Keep these alphabetized please.

[ibra, 2013/09/04 16:19:45]

Done.

+const char kAvatarPrefix[] = "chrome-avatar-index:";

[Bernhard Bauer, 2013/09/04 12:28:33]

How would this be used on Chrome OS?

[ibra, 2013/09/04 16:19:45]

My understanding is that when the Chrome OS guys want to provide the support for
supervised users profile pictures sync they will enclose this in:

#if defined(OS_CHROMEOS)
const char kAvatarPrefix = "chromeos-avatar-index:";
#elif
const char kAvatarPrefix = "chrome-avatar-index:";
#endif

or do you mean that we shall add this in this CL? but it won't be used until
they support profile picture sync.

[Bernhard Bauer, 2013/09/04 16:33:10]

But this would mean that Chrome on different platforms would have different
invariants about the prefix for an avatar. If we say that the above prefix is
what we prepend when creating a managed user and what we expect when syncing one
down, syncing from Chrome to Chrome OS or the other way around would violate
that invariant.

Basically, we said we would have two separate images, one for Chrome OS and one
for Chrome, so that would mean that we'd also have two keys for them, right?
You don't need to add that in this CL; my question was more aimed at making it
possible to add Chrome OS support without breaking things.

 
 SyncData CreateLocalSyncData(const std::string& id,
                              const std::string& name,
                              bool acknowledged,
-                             const std::string& master_key) {
+                             const std::string& master_key,
+                             const std::string& avatar) {
   ::sync_pb::EntitySpecifics specifics;
   specifics.mutable_managed_user()->set_id(id);
   specifics.mutable_managed_user()->set_name(name);
+  if (!avatar.empty())
+    specifics.mutable_managed_user()->set_avatar(avatar);
   if (!master_key.empty())
     specifics.mutable_managed_user()->set_master_key(master_key);
   if (acknowledged)
@@ -51,6 +57,25 @@ SyncData CreateLocalSyncData(const std::string& id,
   return SyncData::CreateLocalData(id, name, specifics);
 }
 
+SyncData CreateSyncDataFromDictionaryEntry(
+    const DictionaryValue::Iterator& it) {
+  const DictionaryValue* dict = NULL;
+  bool success = it.value().GetAsDictionary(&dict);
+  DCHECK(success);
+  bool acknowledged = false;
+  dict->GetBoolean(kAcknowledged, &acknowledged);
+  std::string name;
+  dict->GetString(kName, &name);
+  DCHECK(!name.empty());
+  std::string master_key;
+  dict->GetString(kMasterKey, &master_key);
+  std::string avatar;
+  dict->GetString(kAvatar, &avatar);
+
+  return CreateLocalSyncData(it.key(), name, acknowledged,
+                             master_key, avatar);
+}
+
 }  // namespace
 
 ManagedUserSyncService::ManagedUserSyncService(PrefService* prefs)
@@ -84,12 +109,15 @@ void ManagedUserSyncService::RemoveObserver(
 
 void ManagedUserSyncService::AddManagedUser(const std::string& id,
                                             const std::string& name,
-                                            const std::string& master_key) {
+                                            const std::string& master_key,
+                                            int avatar_index) {
   DictionaryPrefUpdate update(prefs_, prefs::kManagedUsers);
   DictionaryValue* dict = update.Get();
   DictionaryValue* value = new DictionaryValue;
   value->SetString(kName, name);
   value->SetString(kMasterKey, master_key);
+  std::string avatar = base::StringPrintf("%s%d", kAvatarPrefix, avatar_index);

[Bernhard Bauer, 2013/09/04 12:28:33]

Are you planning to extract the avatar index here too? If not, you could
directly use "chrome-avatar-index:%d" as the format string.

[ibra, 2013/09/04 16:19:45]

The above comment basically replies to this also.

However, does it make sense to add in this class a public static method that
basically takes the string formatted avatar and returns only the extracted
index? in this case the UI will just need to call this method and won't worry
about stripping any prefixes.

[Bernhard Bauer, 2013/09/04 16:33:10]

Sure, that sounds reasonable. It's not required for this CL though (but feel
free to add it here if you want to; I'm fine either way).

+  value->SetString(kAvatar, avatar);
   DCHECK(!dict->HasKey(id));
   dict->SetWithoutPathExpansion(id, value);
 
@@ -101,7 +129,7 @@ void ManagedUserSyncService::AddManagedUser(const std::string& id,
   change_list.push_back(SyncChange(
       FROM_HERE,
       SyncChange::ACTION_ADD,
-      CreateLocalSyncData(id, name, false, master_key)));
+      CreateLocalSyncData(id, name, false, master_key, avatar)));
   SyncError error =
       sync_processor_->ProcessSyncChanges(FROM_HERE, change_list);
   DCHECK(!error.IsSet()) << error.ToString();
@@ -152,6 +180,7 @@ SyncMergeResult ManagedUserSyncService::MergeDataAndStartSyncing(
     value->SetString(kName, managed_user.name());
     value->SetBoolean(kAcknowledged, managed_user.acknowledged());
     value->SetString(kMasterKey, managed_user.master_key());
+    value->SetString(kAvatar, managed_user.avatar());
     if (dict->HasKey(managed_user.id()))
       num_items_modified++;
     else
@@ -164,19 +193,8 @@ SyncMergeResult ManagedUserSyncService::MergeDataAndStartSyncing(
     if (seen_ids.find(it.key()) != seen_ids.end())
       continue;
 
-    const DictionaryValue* dict = NULL;
-    bool success = it.value().GetAsDictionary(&dict);
-    DCHECK(success);
-    bool acknowledged = false;
-    dict->GetBoolean(kAcknowledged, &acknowledged);
-    std::string name;
-    dict->GetString(kName, &name);
-    std::string master_key;
-    dict->GetString(kMasterKey, &master_key);
-    DCHECK(!name.empty());
-    change_list.push_back(
-        SyncChange(FROM_HERE, SyncChange::ACTION_ADD,
-            CreateLocalSyncData(it.key(), name, acknowledged, master_key)));
+    change_list.push_back(SyncChange(FROM_HERE, SyncChange::ACTION_ADD,
+                                     CreateSyncDataFromDictionaryEntry(it)));
   }
   result.set_error(sync_processor_->ProcessSyncChanges(FROM_HERE, change_list));
 
@@ -201,19 +219,9 @@ SyncDataList ManagedUserSyncService::GetAllSyncData(
   SyncDataList data;
   DictionaryPrefUpdate update(prefs_, prefs::kManagedUsers);
   DictionaryValue* dict = update.Get();
-  for (DictionaryValue::Iterator it(*dict); !it.IsAtEnd(); it.Advance()) {
-    const DictionaryValue* dict = NULL;
-    bool success = it.value().GetAsDictionary(&dict);
-    DCHECK(success);
-    std::string name;
-    dict->GetString(kName, &name);
-    std::string master_key;
-    dict->GetString(kMasterKey, &master_key);
-    bool acknowledged = false;
-    dict->GetBoolean(kAcknowledged, &acknowledged);
-    data.push_back(
-        CreateLocalSyncData(it.key(), name, acknowledged, master_key));
-  }
+  for (DictionaryValue::Iterator it(*dict); !it.IsAtEnd(); it.Advance())
+    data.push_back(CreateSyncDataFromDictionaryEntry(it));
+
   return data;
 }
 
@@ -252,6 +260,7 @@ SyncError ManagedUserSyncService::ProcessSyncChanges(
         value->SetString(kName, managed_user.name());
         value->SetBoolean(kAcknowledged, managed_user.acknowledged());
         value->SetString(kMasterKey, managed_user.master_key());
+        value->SetString(kAvatar, managed_user.avatar());
         dict->SetWithoutPathExpansion(managed_user.id(), value);
         break;
       }