Avatar syncing for supervised users

Avatar syncing for supervised users

Syncs the avatar of the supervised user selected during its creation process but doesn't sync any further changes.

TBR=nkostylev@chromium.org

BUG=278083
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222019

[ibra, 2013-09-04 12:06:51]

Bernhard, can you please take a look?

Thanks!

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

Can you expand the CL comment to explain that we sync the avatar at profile
creation time, but not changes after that?

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_registration_utility.h (right):

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_registration_utility.h:34: explicit
ManagedUserRegistrationInfo(const string16& name, int avatar_index);
If you have more than one constructor argument, "explicit" isn't necessary
anymore.

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_sync_service.cc (right):

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service.cc:40: const char
kAvatar[] = "avatar";
Keep these alphabetized please.

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service.cc:41: const char
kAvatarPrefix[] = "chrome-avatar-index:";
How would this be used on Chrome OS?

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service.cc:119: std::string avatar
= base::StringPrintf("%s%d", kAvatarPrefix, avatar_index);
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]

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_registration_utility.h (right):

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_registration_utility.h:34: explicit
ManagedUserRegistrationInfo(const string16& name, int avatar_index);
On 2013/09/04 12:28:33, Bernhard Bauer wrote:
> If you have more than one constructor argument, "explicit" isn't necessary
> anymore.

Done.

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_sync_service.cc (right):

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service.cc:40: const char
kAvatar[] = "avatar";
On 2013/09/04 12:28:33, Bernhard Bauer wrote:
> Keep these alphabetized please.

Done.

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service.cc:41: const char
kAvatarPrefix[] = "chrome-avatar-index:";
On 2013/09/04 12:28:33, Bernhard Bauer wrote:
> How would this be used on Chrome OS?
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.

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service.cc:119: std::string avatar
= base::StringPrintf("%s%d", kAvatarPrefix, avatar_index);
On 2013/09/04 12:28:33, Bernhard Bauer wrote:
> Are you planning to extract the avatar index here too? If not, you could
> directly use "chrome-avatar-index:%d" as the format string.
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]

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_sync_service.cc (right):

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service.cc:41: const char
kAvatarPrefix[] = "chrome-avatar-index:";
On 2013/09/04 16:19:45, ibra wrote:
> On 2013/09/04 12:28:33, Bernhard Bauer wrote:
> > How would this be used on Chrome OS?
> 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

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?

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

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.

https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service.cc:119: std::string avatar
= base::StringPrintf("%s%d", kAvatarPrefix, avatar_index);
On 2013/09/04 16:19:45, ibra wrote:
> On 2013/09/04 12:28:33, Bernhard Bauer wrote:
> > Are you planning to extract the avatar index here too? If not, you could
> > directly use "chrome-avatar-index:%d" as the format string.
> 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.

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).

[ibra, 2013-09-05 12:49:27]

On 2013/09/04 16:33:10, Bernhard Bauer wrote:
>
https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
> File chrome/browser/managed_mode/managed_user_sync_service.cc (right):
> 
>
https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
> chrome/browser/managed_mode/managed_user_sync_service.cc:41: const char
> kAvatarPrefix[] = "chrome-avatar-index:";
> On 2013/09/04 16:19:45, ibra wrote:
> > On 2013/09/04 12:28:33, Bernhard Bauer wrote:
> > > How would this be used on Chrome OS?
> > 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
> 
> 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?
> 
> > or do you mean that we shall add this in this CL? but it won't be used until
> > they support profile picture sync.
> 
> 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.
> 
>
https://codereview.chromium.org/23653007/diff/3001/chrome/browser/managed_mod...
> chrome/browser/managed_mode/managed_user_sync_service.cc:119: std::string
avatar
> = base::StringPrintf("%s%d", kAvatarPrefix, avatar_index);
> On 2013/09/04 16:19:45, ibra wrote:
> > On 2013/09/04 12:28:33, Bernhard Bauer wrote:
> > > Are you planning to extract the avatar index here too? If not, you could
> > > directly use "chrome-avatar-index:%d" as the format string.
> > 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.
> 
> 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).

Bernhard, PTAL. Thanks!

[Bernhard Bauer, 2013-09-05 13:15:32]

https://codereview.chromium.org/23653007/diff/19001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_registration_utility.h (right):

https://codereview.chromium.org/23653007/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_registration_utility.h:34: explicit
ManagedUserRegistrationInfo(const string16& name);
You should probably still initialize |avatar_index| to *some* value.

https://codereview.chromium.org/23653007/diff/19001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_sync_service.cc (right):

https://codereview.chromium.org/23653007/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_sync_service.cc:130: // users avatar
syncing on chrome OS.
Nit: "Chrome" with an uppercase "C".

https://codereview.chromium.org/23653007/diff/19001/sync/protocol/managed_use...
File sync/protocol/managed_user_specifics.proto (right):

https://codereview.chromium.org/23653007/diff/19001/sync/protocol/managed_use...
sync/protocol/managed_user_specifics.proto:28: // A string representing the
index of the supervised user avatar on chrome
Nit: period at the end of the sentence (also below).

[ibra, 2013-09-05 13:28:38]

https://codereview.chromium.org/23653007/diff/19001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_registration_utility.h (right):

https://codereview.chromium.org/23653007/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_registration_utility.h:34: explicit
ManagedUserRegistrationInfo(const string16& name);
On 2013/09/05 13:15:33, Bernhard Bauer wrote:
> You should probably still initialize |avatar_index| to *some* value.

Wouldn't that make more sense once Chrome OS also supports avatar syncing, right
now we would have to send in a dummy value, WDYT? If you agree I can add a TODO
instead.

https://codereview.chromium.org/23653007/diff/19001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_sync_service.cc (right):

https://codereview.chromium.org/23653007/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_sync_service.cc:130: // users avatar
syncing on chrome OS.
On 2013/09/05 13:15:33, Bernhard Bauer wrote:
> Nit: "Chrome" with an uppercase "C".

Done.

https://codereview.chromium.org/23653007/diff/19001/sync/protocol/managed_use...
File sync/protocol/managed_user_specifics.proto (right):

https://codereview.chromium.org/23653007/diff/19001/sync/protocol/managed_use...
sync/protocol/managed_user_specifics.proto:28: // A string representing the
index of the supervised user avatar on chrome
On 2013/09/05 13:15:33, Bernhard Bauer wrote:
> Nit: period at the end of the sentence (also below).

Done.

[Bernhard Bauer, 2013-09-05 14:09:47]

https://codereview.chromium.org/23653007/diff/19001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_registration_utility.h (right):

https://codereview.chromium.org/23653007/diff/19001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_registration_utility.h:34: explicit
ManagedUserRegistrationInfo(const string16& name);
On 2013/09/05 13:28:38, ibra wrote:
> On 2013/09/05 13:15:33, Bernhard Bauer wrote:
> > You should probably still initialize |avatar_index| to *some* value.
> 
> Wouldn't that make more sense once Chrome OS also supports avatar syncing,
right
> now we would have to send in a dummy value, WDYT? If you agree I can add a
TODO
> instead.

Well, right now you get an undefined value if the caller does not set one (like
on Chrome OS), which is what I would like to avoid.

You could use 0 or a dummy value; I would prefer a dummy value because you can
DCHECK in ManagedUserSyncService for its presence if you don't use the avatar
index (i.e. on Chrome OS), or for its absence if you do use it.

https://codereview.chromium.org/23653007/diff/22001/sync/protocol/managed_use...
File sync/protocol/managed_user_specifics.proto (right):

https://codereview.chromium.org/23653007/diff/22001/sync/protocol/managed_use...
sync/protocol/managed_user_specifics.proto:32: // A string representing the
index of the supervised user avatar on chromeos.
Nit: "Chrome OS" (sorry, I overloooked this).

[ibra, 2013-09-05 15:14:31]

PTAL, thanks.

https://codereview.chromium.org/23653007/diff/22001/sync/protocol/managed_use...
File sync/protocol/managed_user_specifics.proto (right):

https://codereview.chromium.org/23653007/diff/22001/sync/protocol/managed_use...
sync/protocol/managed_user_specifics.proto:32: // A string representing the
index of the supervised user avatar on chromeos.
On 2013/09/05 14:09:47, Bernhard Bauer wrote:
> Nit: "Chrome OS" (sorry, I overloooked this).

Done.

[Bernhard Bauer, 2013-09-05 15:35:24]

https://codereview.chromium.org/23653007/diff/42001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_sync_service.cc (right):

https://codereview.chromium.org/23653007/diff/42001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_sync_service.cc:126:
DCHECK_EQ(avatar_index, -111);
Where is this number coming from?

[ibra, 2013-09-05 15:44:50]

https://codereview.chromium.org/23653007/diff/42001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_sync_service.cc (right):

https://codereview.chromium.org/23653007/diff/42001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_sync_service.cc:126:
DCHECK_EQ(avatar_index, -111);
On 2013/09/05 15:35:24, Bernhard Bauer wrote:
> Where is this number coming from?

Are you talking about the value? or about what?

This is just a dummy value

[Bernhard Bauer, 2013-09-05 15:51:11]

https://codereview.chromium.org/23653007/diff/42001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_sync_service.cc (right):

https://codereview.chromium.org/23653007/diff/42001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_sync_service.cc:126:
DCHECK_EQ(avatar_index, -111);
On 2013/09/05 15:44:50, ibra wrote:
> On 2013/09/05 15:35:24, Bernhard Bauer wrote:
> > Where is this number coming from?
> 
> Are you talking about the value? or about what?
> 
> This is just a dummy value

Yes, you're setting the value in locally_managed_user_creation_controller.cc,
then check it here. A casual reader of the code wouldn't know where this value
comes from. Can you at least add a reference to it in a comment (and a TODO to
remove this once it is implemented on Chrome OS)?

[ibra, 2013-09-05 16:25:02]

https://codereview.chromium.org/23653007/diff/42001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_sync_service.cc (right):

https://codereview.chromium.org/23653007/diff/42001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_sync_service.cc:126:
DCHECK_EQ(avatar_index, -111);
On 2013/09/05 15:51:12, Bernhard Bauer wrote:
> On 2013/09/05 15:44:50, ibra wrote:
> > On 2013/09/05 15:35:24, Bernhard Bauer wrote:
> > > Where is this number coming from?
> > 
> > Are you talking about the value? or about what?
> > 
> > This is just a dummy value
> 
> Yes, you're setting the value in locally_managed_user_creation_controller.cc,
> then check it here. A casual reader of the code wouldn't know where this value
> comes from. Can you at least add a reference to it in a comment (and a TODO to
> remove this once it is implemented on Chrome OS)?

Done.

[Bernhard Bauer, 2013-09-05 16:26:38]

lgtm

[ibra, 2013-09-05 16:37:48]

Hi Andrew,

Can I have an OWNERS review for:

sync/protocol/ ?

Thanks!

[Andrew T Wilson (Slow), 2013-09-06 14:57:09]

sync/protocol lgtm

[commit-bot: I haz the power, 2013-09-06 16:58:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@chromium.org/23653007/51001

[commit-bot: I haz the power, 2013-09-06 16:58:36]

Failed to apply patch for
chrome/browser/managed_mode/managed_user_sync_service.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/managed_mode/managed_user_sync_service.cc
  Hunk #1 FAILED at 5.
  Hunk #2 succeeded at 35 (offset 1 line).
  Hunk #3 succeeded at 60 (offset 1 line).
  Hunk #4 succeeded at 114 (offset 1 line).
  Hunk #5 succeeded at 146 (offset 1 line).
  Hunk #6 succeeded at 215 (offset 18 lines).
  Hunk #7 succeeded at 229 (offset 18 lines).
  Hunk #8 succeeded at 257 (offset 20 lines).
  Hunk #9 succeeded at 298 (offset 20 lines).
  1 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/managed_mode/managed_user_sync_service.cc.rej

Patch:       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..830eced83c9183dee041647eb0a66b1641669868
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"
@@ -34,16 +35,24 @@ using sync_pb::ManagedUserSpecifics;
 namespace {
 
 const char kAcknowledged[] = "acknowledged";
+const char kChromeAvatar[] = "chromeAvatar";
+const char kChromeOsAvatar[] = "chromeOsAvatar";
 const char kName[] = "name";
 const char kMasterKey[] = "masterKey";
 
 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& chrome_avatar,
+                             const std::string& chromeos_avatar) {
   ::sync_pb::EntitySpecifics specifics;
   specifics.mutable_managed_user()->set_id(id);
   specifics.mutable_managed_user()->set_name(name);
+  if (!chrome_avatar.empty())
+    specifics.mutable_managed_user()->set_chrome_avatar(chrome_avatar);
+  if (!chromeos_avatar.empty())
+    specifics.mutable_managed_user()->set_chromeos_avatar(chromeos_avatar);
   if (!master_key.empty())
     specifics.mutable_managed_user()->set_master_key(master_key);
   if (acknowledged)
@@ -51,6 +60,27 @@ 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 chrome_avatar;
+  dict->GetString(kChromeAvatar, &chrome_avatar);
+  std::string chromeos_avatar;
+  dict->GetString(kChromeOsAvatar, &chromeos_avatar);
+
+  return CreateLocalSyncData(it.key(), name, acknowledged, master_key,
+                             chrome_avatar, chromeos_avatar);
+}
+
 }  // namespace
 
 ManagedUserSyncService::ManagedUserSyncService(PrefService* prefs)
@@ -84,12 +114,27 @@ 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 chrome_avatar;
+#if defined(CHROME_OS)
+  // This is a dummy value that is passed when a supervised user is created on
+  // Chrome OS.
+  // TODO(ibraaaa): update this to use the correct avatar index
+  // once avatar syncing for supervised users is implemented on Chrome OS.
+  DCHECK_EQ(avatar_index, -111);
+#else
+  chrome_avatar = base::StringPrintf("chrome-avatar-index:%d", avatar_index);
+#endif
+  value->SetString(kChromeAvatar, chrome_avatar);
+  // TODO(ibraaaa): this should be updated to allow supervised
+  // users avatar syncing on Chrome OS.
+  value->SetString(kChromeOsAvatar, std::string());
   DCHECK(!dict->HasKey(id));
   dict->SetWithoutPathExpansion(id, value);
 
@@ -101,7 +146,8 @@ 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,
+                          chrome_avatar, std::string())));
   SyncError error =
       sync_processor_->ProcessSyncChanges(FROM_HERE, change_list);
   DCHECK(!error.IsSet()) << error.ToString();
@@ -152,6 +198,8 @@ SyncMergeResult
ManagedUserSyncService::MergeDataAndStartSyncing(
     value->SetString(kName, managed_user.name());
     value->SetBoolean(kAcknowledged, managed_user.acknowledged());
     value->SetString(kMasterKey, managed_user.master_key());
+    value->SetString(kChromeAvatar, managed_user.chrome_avatar());
+    value->SetString(kChromeOsAvatar, managed_user.chromeos_avatar());
     if (dict->HasKey(managed_user.id()))
       num_items_modified++;
     else
@@ -164,19 +212,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 +238,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 +279,8 @@ SyncError ManagedUserSyncService::ProcessSyncChanges(
         value->SetString(kName, managed_user.name());
         value->SetBoolean(kAcknowledged, managed_user.acknowledged());
         value->SetString(kMasterKey, managed_user.master_key());
+        value->SetString(kChromeAvatar, managed_user.chrome_avatar());
+        value->SetString(kChromeOsAvatar, managed_user.chromeos_avatar());
         dict->SetWithoutPathExpansion(managed_user.id(), value);
         break;
       }

[commit-bot: I haz the power, 2013-09-09 08:12:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@chromium.org/23653007/70001

[commit-bot: I haz the power, 2013-09-09 13:51:10]

Change committed as 222019