Browser tests for multiprofile settings.

Browser tests for multiprofile settings.

Check the preferences thaht can be controlled by other users.
Also look for the secondary user banner, and controlled-by
indicators in headings.

BUG=359578
R=nkostylev@chromium.org

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

[Nikita (slow), 2014-06-16 16:25:16]

looking great, some nits

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

https://codereview.chromium.org/322293002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc:96: //
Add the stub settings provider, moving the device settings provider
nit: insert empty line.

https://codereview.chromium.org/322293002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc:123:
CheckSharedSections(contents, !is_owner);
nit: !is_owner: I assume this only works when owner is primary however this
restriction will be removed in M38.

https://codereview.chromium.org/322293002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc:275:
CheckOptionsUI(manager->FindUser(kTestUsers[0]), true);
Can you rely on UserManager to define which user is an owner?

This would require setting kDeviceOwner
https://code.google.com/p/chromium/codesearch#search/&q=kDeviceOwner%20file:t...

https://codereview.chromium.org/322293002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc:317:
CheckBooleanPreference(contents1, prefs::kEnableAutoScreenLock, false, "",
nit: std::string() here and below

https://codereview.chromium.org/322293002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc:317:
CheckBooleanPreference(contents1, prefs::kEnableAutoScreenLock, false, "",
It would be great to have something like

bool disabled = false;
bool expected_value = false;

std::string empty_controlled;

{
CheckBooleanPreference(
    contents1, prefs::kEnableAutoScreenLock, disabled, empty_controlled,
expected_value);
}

[Nikita (slow), 2014-06-16 16:25:50]

sorry for review delay, 4 days holidays.

[michaelpg, 2014-06-16 18:31:17]

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

https://codereview.chromium.org/322293002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc:96: //
Add the stub settings provider, moving the device settings provider
On 2014/06/16 16:25:16, Nikita Kostylev wrote:
> nit: insert empty line.

Done.

https://codereview.chromium.org/322293002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc:123:
CheckSharedSections(contents, !is_owner);
On 2014/06/16 16:25:16, Nikita Kostylev wrote:
> nit: !is_owner: I assume this only works when owner is primary however this
> restriction will be removed in M38.

Added is_primary and a TODO.

https://codereview.chromium.org/322293002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc:275:
CheckOptionsUI(manager->FindUser(kTestUsers[0]), true);
On 2014/06/16 16:25:16, Nikita Kostylev wrote:
> Can you rely on UserManager to define which user is an owner?
> 
> This would require setting kDeviceOwner
>
https://code.google.com/p/chromium/codesearch#search/&q=kDeviceOwner%20file:t...

I set kDeviceOwner already to make the tests work. I could use
manager->GetOwnerEmail() but I don't see the point.

I do like the idea of renaming my constants though, especially for
when we have more test cases once the primary user doesn't have to
be the owner.

https://codereview.chromium.org/322293002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc:317:
CheckBooleanPreference(contents1, prefs::kEnableAutoScreenLock, false, "",
On 2014/06/16 16:25:16, Nikita Kostylev wrote:
> nit: std::string() here and below

Done.

https://codereview.chromium.org/322293002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc:317:
CheckBooleanPreference(contents1, prefs::kEnableAutoScreenLock, false, "",
On 2014/06/16 16:25:16, Nikita Kostylev wrote:
> It would be great to have something like
> 
> bool disabled = false;
> bool expected_value = false;
> 
> std::string empty_controlled;
> 
> {
> CheckBooleanPreference(
>     contents1, prefs::kEnableAutoScreenLock, disabled, empty_controlled,
> expected_value);
> }

Done.

[Nikita (slow), 2014-06-17 09:29:59]

lgtm

https://codereview.chromium.org/322293002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc
(right):

https://codereview.chromium.org/322293002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc:321: bool
expected_value = false;
nit: How about always setting expected_value inside {} scope even if it has a
correct value from previous test case.

This way you won't have to update test cases in case you'll change previous ones
and this way expected value is seen in the context.

[michaelpg, 2014-06-17 17:15:23]

Thanks

https://codereview.chromium.org/322293002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc
(right):

https://codereview.chromium.org/322293002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc:321: bool
expected_value = false;
On 2014/06/17 09:29:59, Nikita Kostylev wrote:
> nit: How about always setting expected_value inside {} scope even if it has a
> correct value from previous test case.
> 
> This way you won't have to update test cases in case you'll change previous
ones
> and this way expected value is seen in the context.

Done.

[michaelpg, 2014-06-17 17:15:27]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-06-17 17:16:28]

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

[commit-bot: I haz the power, 2014-06-17 21:26:05]

Change committed as 277868