Display supervised users synced avatars in the import overlay

Display supervised users synced avatars in the import overlay

BUG=None

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

[ibra, 2013-09-09 12:34:59]

Hey Sergiu,

Please review.

Thanks!

[Sergiu, 2013-09-09 13:35:35]

lgtm

[ibra, 2013-09-09 13:44:22]

Hey Bernhard, please take a look. Thanks!

[Bernhard Bauer, 2013-09-09 14:13:14]

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

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service.cc:118: NOTREACHED();
From
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
: "You should not handle DCHECK() failures". If it indeed cannot ever happen
that someone passes in a string without the prefix, it's fine to assume just
that (documenting it with DCHECKs) without ever checking it in production code.
If it can happen, returning success or failure should be an explicit part of the
contract (see also my other comment in the test below).

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service.cc:161: chrome_avatar =
base::StringPrintf("%s%d", kChromeAvatarPrefix, avatar_index);
As an additional sanity check, you could extract this into a method, and unit
test that the result of this method parses correctly.

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_sync_service.h (right):

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service.h:39: static int
GetAvatarIndex(const std::string& avatar_str);
Please document the preconditions (what format should the string have?) and
return value.

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_sync_service_unittest.cc (right):

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service_unittest.cc:280:
EXPECT_DEATH(ManagedUserSyncService::GetAvatarIndex("wrong-prefix:5"), "");
Urgh. In general, I'm not a big fan of testing the behavior when you violate a
precondition, as that is supposed to be undefined behavior (for example, the
behavior differs between a build with DCHECKs enabled and one without). In
addition, EXPECT_DEATH tests in gtest are kind of a hack.

If you want to test whether this method can deal with invalid inputs, have it
return a boolean flag that says whether extracting the index was successful, and
return the index as an outparam. Then you can DCHECK the result in production
code, and ASSERT_TRUE or EXPECT_TRUE it in tests.

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service_unittest.cc:280:
EXPECT_DEATH(ManagedUserSyncService::GetAvatarIndex("wrong-prefix:5"), "");
Nit: Use std::string() instead of "" (it results in less machine code).

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/managed_user_import_handler.cc (right):

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_import_handler.cc:74:
value->GetString("chromeAvatar", &avatar_str);
Would it make sense to export these keys from ManagedUserSyncService?

[ibra, 2013-09-09 18:03:11]

PTAL, thanks!

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

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service.cc:118: NOTREACHED();
On 2013/09/09 14:13:14, Bernhard Bauer wrote:
> From
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
> : "You should not handle DCHECK() failures". If it indeed cannot ever happen
> that someone passes in a string without the prefix, it's fine to assume just
> that (documenting it with DCHECKs) without ever checking it in production
code.
> If it can happen, returning success or failure should be an explicit part of
the
> contract (see also my other comment in the test below). 

Done.

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service.cc:161: chrome_avatar =
base::StringPrintf("%s%d", kChromeAvatarPrefix, avatar_index);
On 2013/09/09 14:13:14, Bernhard Bauer wrote:
> As an additional sanity check, you could extract this into a method, and unit
> test that the result of this method parses correctly.

Done.

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_sync_service.h (right):

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service.h:39: static int
GetAvatarIndex(const std::string& avatar_str);
On 2013/09/09 14:13:14, Bernhard Bauer wrote:
> Please document the preconditions (what format should the string have?) and
> return value.

Done.

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_sync_service_unittest.cc (right):

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service_unittest.cc:280:
EXPECT_DEATH(ManagedUserSyncService::GetAvatarIndex("wrong-prefix:5"), "");
On 2013/09/09 14:13:14, Bernhard Bauer wrote:
> Urgh. In general, I'm not a big fan of testing the behavior when you violate a
> precondition, as that is supposed to be undefined behavior (for example, the
> behavior differs between a build with DCHECKs enabled and one without). In
> addition, EXPECT_DEATH tests in gtest are kind of a hack.
> 
> If you want to test whether this method can deal with invalid inputs, have it
> return a boolean flag that says whether extracting the index was successful,
and
> return the index as an outparam. Then you can DCHECK the result in production
> code, and ASSERT_TRUE or EXPECT_TRUE it in tests.

Done.

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_sync_service_unittest.cc:280:
EXPECT_DEATH(ManagedUserSyncService::GetAvatarIndex("wrong-prefix:5"), "");
On 2013/09/09 14:13:14, Bernhard Bauer wrote:
> Nit: Use std::string() instead of "" (it results in less machine code).

Done.

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/managed_user_import_handler.cc (right):

https://codereview.chromium.org/23523038/diff/1005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_import_handler.cc:74:
value->GetString("chromeAvatar", &avatar_str);
On 2013/09/09 14:13:14, Bernhard Bauer wrote:
> Would it make sense to export these keys from ManagedUserSyncService?

Done.

[Bernhard Bauer, 2013-09-09 19:16:08]

https://codereview.chromium.org/23523038/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_sync_service.h (right):

https://codereview.chromium.org/23523038/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_sync_service.h:53: static bool
GetAvatarIndex(const std::string& avatar_str, int* avatar_index);
What are you planning to do here for Chrome OS?

https://codereview.chromium.org/23523038/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_sync_service_unittest.cc (right):

https://codereview.chromium.org/23523038/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_sync_service_unittest.cc:272: int
avatar;
Initialize the variable maybe?

https://codereview.chromium.org/23523038/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_sync_service_unittest.cc:277:
EXPECT_TRUE(ManagedUserSyncService::GetAvatarIndex(avatar_str, &avatar));
You could also check the resulting string.

[ibra, 2013-09-10 11:10:44]

https://codereview.chromium.org/23523038/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_sync_service.h (right):

https://codereview.chromium.org/23523038/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_sync_service.h:53: static bool
GetAvatarIndex(const std::string& avatar_str, int* avatar_index);
On 2013/09/09 19:16:08, Bernhard Bauer wrote:
> What are you planning to do here for Chrome OS?

Switching on the prefix since we just expect a different prefix for Chrome OS.

One minor thing also is that we should not return -1 because it is used on
Chrome OS.

https://codereview.chromium.org/23523038/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_sync_service_unittest.cc (right):

https://codereview.chromium.org/23523038/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_sync_service_unittest.cc:272: int
avatar;
On 2013/09/09 19:16:08, Bernhard Bauer wrote:
> Initialize the variable maybe?

Done.

https://codereview.chromium.org/23523038/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_sync_service_unittest.cc:277:
EXPECT_TRUE(ManagedUserSyncService::GetAvatarIndex(avatar_str, &avatar));
On 2013/09/09 19:16:08, Bernhard Bauer wrote:
> You could also check the resulting string.

Done.

[Bernhard Bauer, 2013-09-10 11:34:11]

lgtm

[commit-bot: I haz the power, 2013-09-10 11:34:49]

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

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

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

[commit-bot: I haz the power, 2013-09-11 09:36:45]

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

[commit-bot: I haz the power, 2013-09-11 12:31:40]

Change committed as 222513