Avatar syncing for supervised users

chrome/browser/managed_mode/managed_user_sync_service.cc

 // Copyright 2013 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.
 
 #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"
 #include "chrome/common/pref_names.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_data.h"
 #include "sync/api/sync_error.h"
 #include "sync/api/sync_error_factory.h"
 #include "sync/api/sync_merge_result.h"
@@ skipping 11 matching lines @@
 using syncer::SyncError;
 using syncer::SyncErrorFactory;
 using syncer::SyncMergeResult;
 using sync_pb::ManagedUserSpecifics;
 
 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)
     specifics.mutable_managed_user()->set_acknowledged(true);
   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)
     : prefs_(prefs) {
   pref_change_registrar_.Init(prefs_);
   pref_change_registrar_.Add(
       prefs::kGoogleServicesLastUsername,
       base::Bind(&ManagedUserSyncService::OnLastSignedInUsernameChange,
                  base::Unretained(this)));
 }
@@ skipping 13 matching lines @@
   observers_.AddObserver(observer);
 }
 
 void ManagedUserSyncService::RemoveObserver(
     ManagedUserSyncServiceObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
 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);
 
   if (!sync_processor_)
     return;
 
   // If we're already syncing, create a new change and upload it.
   SyncChangeList change_list;
   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();
 }
 
 void ManagedUserSyncService::DeleteManagedUser(const std::string& id) {
   if (!sync_processor_)
     return;
 
   SyncChangeList change_list;
@@ skipping 30 matching lines @@
   int num_items_modified = 0;
   for (SyncDataList::const_iterator it = initial_sync_data.begin();
        it != initial_sync_data.end(); ++it) {
     DCHECK_EQ(MANAGED_USERS, it->GetDataType());
     const ManagedUserSpecifics& managed_user =
         it->GetSpecifics().managed_user();
     DictionaryValue* value = new DictionaryValue();
     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
       num_items_added++;
     dict->SetWithoutPathExpansion(managed_user.id(), value);
     seen_ids.insert(managed_user.id());
   }
 
   for (DictionaryValue::Iterator it(*dict); !it.IsAtEnd(); it.Advance()) {
     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));
 
   result.set_num_items_modified(num_items_modified);
   result.set_num_items_added(num_items_added);
   result.set_num_items_after_association(dict->size());
 
   return result;
 }
 
 void ManagedUserSyncService::StopSyncing(ModelType type) {
   DCHECK_EQ(MANAGED_USERS, type);
   // The observers may want to change the Sync data, so notify them before
   // resetting the |sync_processor_|.
   NotifyManagedUsersSyncingStopped();
   sync_processor_.reset();
   error_handler_.reset();
 }
 
 SyncDataList ManagedUserSyncService::GetAllSyncData(
     ModelType type) const {
   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;
 }
 
 SyncError ManagedUserSyncService::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const SyncChangeList& change_list) {
   SyncError error;
   DictionaryPrefUpdate update(prefs_, prefs::kManagedUsers);
   DictionaryValue* dict = update.Get();
   for (SyncChangeList::const_iterator it = change_list.begin();
@@ skipping 18 matching lines @@
 
         // If the managed user switched from unacknowledged to acknowledged,
         // we might need to continue with a registration.
         if (old_value && !old_value->HasKey(kAcknowledged))
           NotifyManagedUserAcknowledged(managed_user.id());
 
         DictionaryValue* value = new DictionaryValue;
         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;
       }
       case SyncChange::ACTION_DELETE: {
         DCHECK(dict->HasKey(managed_user.id())) << managed_user.id();
         dict->RemoveWithoutPathExpansion(managed_user.id(), NULL);
         break;
       }
       case SyncChange::ACTION_INVALID: {
         NOTREACHED();
@@ skipping 15 matching lines @@
 void ManagedUserSyncService::NotifyManagedUserAcknowledged(
     const std::string& managed_user_id) {
   FOR_EACH_OBSERVER(ManagedUserSyncServiceObserver, observers_,
                     OnManagedUserAcknowledged(managed_user_id));
 }
 
 void ManagedUserSyncService::NotifyManagedUsersSyncingStopped() {
   FOR_EACH_OBSERVER(ManagedUserSyncServiceObserver, observers_,
                     OnManagedUsersSyncingStopped());
 }