Clarify settings UI in multi-profiles mode

Clarify settings UI in multi-profiles mode

Adds bubble buttons to indicate which settings are shared.

When signed in as a secondary user (i.e., added to a multi-profile session),
a banner should be display. The "Internet connection" header should show a
shared icon. Owner-controlled settings should show the owner icon.
"Require password to wake from sleep" is a special case.

This also fixes a bug where controlled settings on managed devices should say
they are controlled by policy, but instead say the (non-existent) owner controls
them.

For details, see PRD with additional links at bottom:
https://docs.google.com/a/google.com/document/d/1G09Mql1TtVFA4qhb7hlt4u2crBv6_fhrZMf40Yo--lc

BUG=326354
R=dbeam@chromium.org, stevenjb@chromium.org, oshima@chromium.org

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

[michaelpg, 2014-02-06 23:32:22]

Adding icons to shared/owner-controlled settings in multi-profiles mode!

[stevenjb, 2014-02-07 00:40:08]

https://codereview.chromium.org/152143009/diff/1/chrome/app/chromeos_strings....
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/152143009/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:2682: These settings belong to <ph
name="OWNER_EMAIL">$1<ex>john@google.com</ex></ph>.
It's odd that one of these is 'setting' singular and the other is plural. Either
the text should be consistent or the context better described (and name
modified).

https://codereview.chromium.org/152143009/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:2685: Another user has turned this on, so
passwords are required for all signed-in users.
"turned this on" sounds odd to me here, maybe double check the text with UX or a
PM.

https://codereview.chromium.org/152143009/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

https://codereview.chromium.org/152143009/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:50:
return UserManager::Get()->GetOwnerEmail() == username;
Double check with Stefan that this is the best way to do this. (It looks fine to
me, but I know there have been a lot of related changes recently, would be good
to not use a method that will be deprecated soon).

https://codereview.chromium.org/152143009/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:162:
!ProfileHelper::IsOwnerProfile(Profile::FromWebUI(web_ui()));
Should this be 'controlled_by_owner'? It looks like any pref controlled by the
owner is implicitly disabled, and this just checks whether it is controlled?

https://codereview.chromium.org/152143009/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:224:
pref_name == prefs::kEnableAutoScreenLock) {
nit: Check pref_name first

https://codereview.chromium.org/152143009/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:236:
// Screen lock is enabled, but user's pref is disabled. Show the user's
user's pref is disabled, or set to 'false' or either?

https://codereview.chromium.org/152143009/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:261: 
one blank line

https://codereview.chromium.org/152143009/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:295:
// Set fake owner email for testing.
If we want to check this in we should move this logic to UserManager or a
wrapper that we use consistently (and should probably put it behind a flag like
--enable-stub-interactive so that it doesn't affect tests)

[oshima, 2014-02-07 00:55:12]

https://codereview.chromium.org/152143009/diff/1/chrome/app/theme/theme_resou...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/152143009/diff/1/chrome/app/theme/theme_resou...
chrome/app/theme/theme_resources.grd:156: <structure type="chrome_scaled_image"
name="IDR_CONTROLLED_SETTING_OWNER" file="common/controlled_setting_owner.png"
/>
if this is for chromeos only, the png file should be under cros directory.

https://codereview.chromium.org/152143009/diff/1/chrome/app/theme/theme_resou...
chrome/app/theme/theme_resources.grd:838: <structure type="chrome_scaled_image"
name="IDR_SECONDARY_USER_SETTINGS" file="common/secondary_user_settings.png" />
ditto

[michaelpg, 2014-02-07 02:04:26]

https://codereview.chromium.org/152143009/diff/1/chrome/app/chromeos_strings....
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/152143009/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:2682: These settings belong to <ph
name="OWNER_EMAIL">$1<ex>john@google.com</ex></ph>.
On 2014/02/07 00:40:09, stevenjb wrote:
> It's odd that one of these is 'setting' singular and the other is plural.
Either
> the text should be consistent or the context better described (and name
> modified).

good point. This string refers to a section of settings rather than an
individual setting so I'm renaming things to make that clear.

https://codereview.chromium.org/152143009/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:2685: Another user has turned this on, so
passwords are required for all signed-in users.
On 2014/02/07 00:40:09, stevenjb wrote:
> "turned this on" sounds odd to me here, maybe double check the text with UX or
a
> PM.

Yeah this string is still under discussion.

https://codereview.chromium.org/152143009/diff/1/chrome/app/theme/theme_resou...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/152143009/diff/1/chrome/app/theme/theme_resou...
chrome/app/theme/theme_resources.grd:156: <structure type="chrome_scaled_image"
name="IDR_CONTROLLED_SETTING_OWNER" file="common/controlled_setting_owner.png"
/>
On 2014/02/07 00:55:12, oshima wrote:
> if this is for chromeos only, the png file should be under cros directory.

Done (actually this one should be common, but controlled_setting_shared is cros)

https://codereview.chromium.org/152143009/diff/1/chrome/app/theme/theme_resou...
chrome/app/theme/theme_resources.grd:838: <structure type="chrome_scaled_image"
name="IDR_SECONDARY_USER_SETTINGS" file="common/secondary_user_settings.png" />
On 2014/02/07 00:55:12, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/152143009/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

https://codereview.chromium.org/152143009/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:50:
return UserManager::Get()->GetOwnerEmail() == username;
On 2014/02/07 00:40:09, stevenjb wrote:
> Double check with Stefan that this is the best way to do this. (It looks fine
to
> me, but I know there have been a lot of related changes recently, would be
good
> to not use a method that will be deprecated soon).

I will double-check, but I think this is solid. The point is to replace
IsCurrentUserOwner, which doesn't do what it sounds like it does and will be
removed soon.

https://codereview.chromium.org/152143009/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:162:
!ProfileHelper::IsOwnerProfile(Profile::FromWebUI(web_ui()));
On 2014/02/07 00:40:09, stevenjb wrote:
> Should this be 'controlled_by_owner'? It looks like any pref controlled by the
> owner is implicitly disabled, and this just checks whether it is controlled?

That's a better name. Changed

https://codereview.chromium.org/152143009/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:224:
pref_name == prefs::kEnableAutoScreenLock) {
On 2014/02/07 00:40:09, stevenjb wrote:
> nit: Check pref_name first

Done.

https://codereview.chromium.org/152143009/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:236:
// Screen lock is enabled, but user's pref is disabled. Show the user's
On 2014/02/07 00:40:09, stevenjb wrote:
> user's pref is disabled, or set to 'false' or either?

set to false.

https://codereview.chromium.org/152143009/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:261: 
On 2014/02/07 00:40:09, stevenjb wrote:
> one blank line

Done.

https://codereview.chromium.org/152143009/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:295:
// Set fake owner email for testing.
On 2014/02/07 00:40:09, stevenjb wrote:
> If we want to check this in we should move this logic to UserManager or a
> wrapper that we use consistently (and should probably put it behind a flag
like
> --enable-stub-interactive so that it doesn't affect tests)
> 

I'll check with Stefan and Dmitry. I'm only including this here so the string
looks nicer; I'm wary of breaking other functionality that would expect a real
owner to exist if the owner email is set.

[oshima, 2014-02-07 20:01:36]

c/a/theme lgtm

[michaelpg, 2014-02-10 21:16:22]

Fixed strings and a few minor cleanups.

https://codereview.chromium.org/152143009/diff/1/chrome/app/chromeos_strings....
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/152143009/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:2682: These settings belong to <ph
name="OWNER_EMAIL">$1<ex>john@google.com</ex></ph>.
Actually, this is controlled settings work for other versions of Chrome. The
convention is to make _SETTINGS_ plural, so I'm following that instead.

On 2014/02/07 02:04:27, Michael Giuffrida wrote:
> On 2014/02/07 00:40:09, stevenjb wrote:
> > It's odd that one of these is 'setting' singular and the other is plural.
> Either
> > the text should be consistent or the context better described (and name
> > modified).
> 
> good point. This string refers to a section of settings rather than an
> individual setting so I'm renaming things to make that clear.

https://codereview.chromium.org/152143009/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:2685: Another user has turned this on, so
passwords are required for all signed-in users.
String finalized.

On 2014/02/07 02:04:27, Michael Giuffrida wrote:
> On 2014/02/07 00:40:09, stevenjb wrote:
> > "turned this on" sounds odd to me here, maybe double check the text with UX
or
> a
> > PM.
> 
> Yeah this string is still under discussion.

https://codereview.chromium.org/152143009/diff/710001/chrome/app/chromeos_str...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/152143009/diff/710001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:2694: <message
name="IDS_OPTIONS_CONTROLLED_SETTINGS_SHARED" desc="Text displayed in the
controlled settings bubble when it is shown for a group of settings which belong
to the primary user but can be edited.">
Following the convention in generated_resources.grd for plural controlled
settings bubbles.

[stevenjb, 2014-02-11 21:22:44]

C++ lgtm

[michaelpg, 2014-02-12 01:16:58]

dbeam and/or estade, could you take a look at the settings UI changes? Let me
know if you have further questions about the changes.

erg: profile_impl.cc
dpolukhin: c/b/ui/webui/options/chromeos/c*_options_handler.*

Thanks!

[Dmitry Polukhin, 2014-02-12 18:58:44]

https://codereview.chromium.org/152143009/diff/800001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

https://codereview.chromium.org/152143009/diff/800001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:50:
return UserManager::Get()->GetOwnerEmail() == username;
Why you don't check IsCurrentUserOwner() anymore? IsCurrentUserOwner indicates
that owner keys present and singed settings can be changed.

https://codereview.chromium.org/152143009/diff/800001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:162:
!ProfileHelper::IsOwnerProfile(Profile::FromWebUI(web_ui()));
I think we also need check for owner key presence.

https://codereview.chromium.org/152143009/diff/800001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc
(right):

https://codereview.chromium.org/152143009/diff/800001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:397:
if (user_manager->GetUserByProfile(profile) ==
I would compare emails here instead of comparing pointer to User.

[Elliot Glaysher, 2014-02-12 20:02:38]

profiles owners stamp lgtm

[michaelpg, 2014-02-12 20:29:57]

https://codereview.chromium.org/152143009/diff/800001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

https://codereview.chromium.org/152143009/diff/800001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:50:
return UserManager::Get()->GetOwnerEmail() == username;
On 2014/02/12 18:58:45, Dmitry Polukhin wrote:
> Why you don't check IsCurrentUserOwner() anymore? IsCurrentUserOwner indicates
> that owner keys present and singed settings can be changed.

The existing check doesn't work in multiprofile world:

IsCurrentUserOwner is true if any signed-in user is an owner
GetLoggedInUser (deprecated) checks if the username is the active user

So the existing function returns true if the username is the active user but not
the owner, and it returns false if the username is the owner but not the active
user.

This function is used only to add " - Owner" to the owner in the list of users.
Checking for the key shouldn't be necessary.

https://codereview.chromium.org/152143009/diff/800001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:162:
!ProfileHelper::IsOwnerProfile(Profile::FromWebUI(web_ui()));
That will only check that the owner is logged in, not that the current user is
the owner. !IsOwnerProfile should be sufficient to disable settings
(IsOwnerProfile is false if we don't have an owner key). Are you saying owner
key presence is important from the UI's perspective in case IsOwnerProfile gets
broken somehow?

https://codereview.chromium.org/152143009/diff/800001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc
(right):

https://codereview.chromium.org/152143009/diff/800001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:397:
if (user_manager->GetUserByProfile(profile) ==
On 2014/02/12 18:58:45, Dmitry Polukhin wrote:
> I would compare emails here instead of comparing pointer to User.

Done.

[Dmitry Polukhin, 2014-02-12 21:06:30]

LGTM chrome/browser/ui/webui/options/chromeos/*

Please remove IsLoggedInOwner if is used only in one place and for UI element
only.

+ Nikita FYI

https://codereview.chromium.org/152143009/diff/800001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

https://codereview.chromium.org/152143009/diff/800001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:50:
return UserManager::Get()->GetOwnerEmail() == username;
On 2014/02/12 20:29:58, Michael Giuffrida wrote:
> This function is used only to add " - Owner" to the owner in the list 
of users.
> Checking for the key shouldn't be necessary.

I completely agree that this function need to be changed in multi-profile world.
My point is that we can't relay on email comparison if owner key is not present.
Owner email is stored in local state and potentially could be malicious modified
and we can 100% detect it only when owner key present. But perhaps for this UI
element it is not so important. I'm more worry if it function will be used
somewhere else for owner checking. So I would remove this function and make this
check in directly in CreateUserInfo.

https://codereview.chromium.org/152143009/diff/800001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:162:
!ProfileHelper::IsOwnerProfile(Profile::FromWebUI(web_ui()));
On 2014/02/12 20:29:58, Michael Giuffrida wrote:
> That will only check that the owner is logged in, not that the current user is
> the owner. !IsOwnerProfile should be sufficient to disable settings
> (IsOwnerProfile is false if we don't have an owner key). Are you saying owner
> key presence is important from the UI's perspective in case IsOwnerProfile
gets
> broken somehow? 

IsOwnerProfile only check that email looks like owner email, it doesn't check
key presence. I would allow settings change only if owner key present in the
session. In some sense it is not only UI because false own will be able to
change on login screen for a while until real owner logs in. But I think the
change should be done in ProfileHelper::IsOwnerProfile not here. Will talk to
Nikita about it.

[Dan Beam, 2014-02-12 21:56:31]

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/options/browser_options.html:11: <include
src="secondary_user_banner.html">
^ why are you including this file twice?

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
File chrome/browser/resources/options/controlled_setting.js (right):

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/options/controlled_setting.js:109:
defaultStrings['shared'] =
s/['shared']/.shared/

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/options/controlled_setting.js:124:
defaultStrings['owner'] =
revert

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/options/controlled_setting.js:127:
loadTimeData.getString('controlledSettingShared');
s/['shared']/.shared/

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
File chrome/browser/resources/options/language_options.js (right):

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:585:
chrome.send('uiLanguageChange', [languageCode]);
indent off

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
File chrome/browser/resources/uber/uber_shared.css (left):

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/uber/uber_shared.css:130: }
separate this out and send to kevers@

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:50:
return UserManager::Get()->GetOwnerEmail() == username;
case sensitivity?

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:234:
pref->GetValue()->GetAsBoolean(&screen_lock);
bool success = pref->GetValue()->GetAsBoolean(&screen_lock);
DCHECK(success);

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:264:
if (user_manager->GetUserByProfile(profile)->email() != primary_email) {
^ case sensitivity?

https://codereview.chromium.org/152143009/diff/1080001/ui/webui/resources/css...
File ui/webui/resources/css/bubble_button.css (right):

https://codereview.chromium.org/152143009/diff/1080001/ui/webui/resources/css...
ui/webui/resources/css/bubble_button.css:19: top: -9px;
screenshots for these change

[michaelpg, 2014-02-13 02:15:22]

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/options/browser_options.html:11: <include
src="secondary_user_banner.html">
On 2014/02/12 21:56:31, Dan Beam wrote:
> ^ why are you including this file twice?

Duplicated code is duplicated. sorry!

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
File chrome/browser/resources/options/controlled_setting.js (right):

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/options/controlled_setting.js:109:
defaultStrings['shared'] =
On 2014/02/12 21:56:31, Dan Beam wrote:
> s/['shared']/.shared/

Done.

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/options/controlled_setting.js:124:
defaultStrings['owner'] =
On 2014/02/12 21:56:31, Dan Beam wrote:
> revert

Done.

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/options/controlled_setting.js:127:
loadTimeData.getString('controlledSettingShared');
On 2014/02/12 21:56:31, Dan Beam wrote:
> s/['shared']/.shared/

Done.

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
File chrome/browser/resources/options/language_options.js (right):

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:585:
chrome.send('uiLanguageChange', [languageCode]);
On 2014/02/12 21:56:31, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
File chrome/browser/resources/uber/uber_shared.css (left):

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/resourc...
chrome/browser/resources/uber/uber_shared.css:130: }
On 2014/02/12 21:56:31, Dan Beam wrote:
> separate this out and send to kevers@

He approved this already - should I commit it separately?

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:50:
return UserManager::Get()->GetOwnerEmail() == username;
On 2014/02/12 21:56:31, Dan Beam wrote:
> case sensitivity?

This should already be the canonicalized email.

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:234:
pref->GetValue()->GetAsBoolean(&screen_lock);
On 2014/02/12 21:56:31, Dan Beam wrote:
> bool success = pref->GetValue()->GetAsBoolean(&screen_lock);
> DCHECK(success);

Done.

https://codereview.chromium.org/152143009/diff/1080001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:264:
if (user_manager->GetUserByProfile(profile)->email() != primary_email) {
On 2014/02/12 21:56:31, Dan Beam wrote:
> ^ case sensitivity?

User::email() already returns the canonicalized email.

https://codereview.chromium.org/152143009/diff/1080001/ui/webui/resources/css...
File ui/webui/resources/css/bubble_button.css (right):

https://codereview.chromium.org/152143009/diff/1080001/ui/webui/resources/css...
ui/webui/resources/css/bubble_button.css:19: top: -9px;
On 2014/02/12 21:56:31, Dan Beam wrote:
> screenshots for these change

Will upload tomorrow (working remotely today)

[michaelpg, 2014-02-13 19:36:09]

Screenshots are available at
https://drive.google.com/a/google.com/folderview?id=0B6HSBrih6pNUMUl0QmJUV2JP...

This shows the changes in the alignment of the bubble buttons (the icons).

https://codereview.chromium.org/152143009/diff/1080001/ui/webui/resources/css...
File ui/webui/resources/css/bubble_button.css (right):

https://codereview.chromium.org/152143009/diff/1080001/ui/webui/resources/css...
ui/webui/resources/css/bubble_button.css:19: top: -9px;
On 2014/02/13 02:15:23, Michael Giuffrida wrote:
> On 2014/02/12 21:56:31, Dan Beam wrote:
> > screenshots for these change
> 
> Will upload tomorrow (working remotely today)

https://drive.google.com/a/google.com/folderview?id=0B6HSBrih6pNUMUl0QmJUV2JP...

[Dan Beam, 2014-02-13 23:06:09]

lgtm

the network screenshot looks really funky, imo...

[michaelpg, 2014-02-13 23:14:54]

On 2014/02/13 23:06:09, Dan Beam wrote:
> lgtm
> 
> the network screenshot looks really funky, imo...

The "BubbleNetwork" shots are the same as what you see today in single-profile
mode. It's a little clearer to see the icon is associated with the network if
you hover over the network (the row becomes highlighted).

[michaelpg, 2014-02-13 23:27:03]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-02-13 23:32:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelpg@chromium.org/152143009/1290001

[michaelpg, 2014-02-13 23:35:46]

The CQ bit was unchecked by michaelpg@chromium.org

[michaelpg, 2014-02-13 23:58:33]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2014-02-13 23:58:39]

The CQ bit was unchecked by michaelpg@chromium.org

[michaelpg, 2014-02-14 01:18:35]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-02-14 01:19:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelpg@chromium.org/152143009/1700001

[commit-bot: I haz the power, 2014-02-14 02:06:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 02:06:11]

Retried try job too often on mac_rel for step(s) check_deps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[michaelpg, 2014-02-14 02:40:34]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-02-14 02:42:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelpg@chromium.org/152143009/1700001

[commit-bot: I haz the power, 2014-02-14 03:54:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 03:54:27]

Try jobs failed on following builders: linux_chromium_rel

[michaelpg, 2014-02-14 04:45:36]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2014-02-14 04:45:52]

The CQ bit was unchecked by michaelpg@chromium.org

[michaelpg, 2014-02-14 04:46:29]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-02-14 04:47:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelpg@chromium.org/152143009/2110001

[commit-bot: I haz the power, 2014-02-14 14:07:30]

Change committed as 251302