ChromeOS: implement per-user time zone preferences.

ChromeOS: implement per-user time zone preferences.

Historically ChromeOS had only one global time zone preference. But now time
settings become complicate and we need to support per-user time zone
preferences.

TBR=phajdan.jr@chromium.org

BUG=622555, 699472
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2849823003
Cr-Commit-Position: refs/heads/master@{#493673}
Committed: https://chromium.googlesource.com/chromium/src/+/2cb0d75387b3fbf61e1d37a7e9b902a87842b363

[Alexander Alekseev, 2017-04-29 02:38:33]

Description was changed from

==========
ChromeOS: implement per-user time zone preferences.

Historically ChromeOS had only one global time zone preference. But now time
settings become complicate and we need to support per-user time zone
preferences.

BUG=622555,699472
==========

to

==========
ChromeOS: implement per-user time zone preferences.

Historically ChromeOS had only one global time zone preference. But now time
settings become complicate and we need to support per-user time zone
preferences.

BUG=622555,699472
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Alexander Alekseev, 2017-04-29 02:39:50]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 02:40:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 02:54:35]

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

[commit-bot: I haz the power, 2017-04-29 02:54:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2017-05-01 05:09:10]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 05:09:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 05:53:00]

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

[commit-bot: I haz the power, 2017-05-01 05:53:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2017-05-01 07:59:31]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 07:59:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 09:10:38]

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

[commit-bot: I haz the power, 2017-05-01 09:10:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2017-05-03 07:04:44]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 07:04:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 08:18:38]

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

[commit-bot: I haz the power, 2017-05-03 08:18:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2017-05-11 00:19:05]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 00:20:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 00:35:04]

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

[commit-bot: I haz the power, 2017-05-11 00:35:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexander Alekseev, 2017-05-11 00:41:41]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 00:43:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 00:55:59]

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

[commit-bot: I haz the power, 2017-05-11 00:56:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2017-05-11 01:18:58]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 01:20:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 03:48:19]

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

[commit-bot: I haz the power, 2017-05-11 03:48:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2017-05-12 03:02:28]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 03:02:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 04:02:28]

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

[commit-bot: I haz the power, 2017-05-12 04:02:29]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexander Alekseev, 2017-05-12 05:28:32]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 05:28:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 06:12:35]

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

[commit-bot: I haz the power, 2017-05-12 06:12:36]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexander Alekseev, 2017-05-12 21:28:08]

alemate@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Alexander Alekseev, 2017-05-12 21:28:09]

Steven, please review.

(I am still working on policy test of CrSettingsDateTimePageTest, but all the
other code is ready).

[Alexander Alekseev, 2017-05-13 01:07:08]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-13 01:07:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-13 01:46:14]

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

[commit-bot: I haz the power, 2017-05-13 01:46:15]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[stevenjb, 2017-05-15 17:08:01]

stevenjb@chromium.org changed reviewers:
+ michaelpg@chromium.org

[stevenjb, 2017-05-15 17:08:02]

+michaelpg@ who is more familiar with these settings than I am.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:212: timezone);
This should only be necessary inside the else, and only if
(!customization_timezone.empty()), correct?

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:213:
chromeos::CrosSettings::Get()->SetString(chromeos::kSystemTimezone, timezone);
What if timezone is empty, should we still be setting this? Also, could you
document why we are setting the CrosSettings pref here?

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/wizard_controller.cc:1432:
CrosSettings::Get()->SetString(kSystemTimezone, timezone->timeZoneId);
Setting both of these in multiple places is a little confusing, maybe have a
helper function somewhere instead, maybe in timezone_util?

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/preferences.cc:351: : "");
|current_timezone_id_valid| isn't actually necessary, |current_timezone_id| will
be unset and therefore empty if GetString fails.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/timezone_util.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:199: }
nit: blank line

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:201: const bool
is_resolve_managed = local_state->IsManagedPreference(
I'm confused about what 'resolve' refers to here and below.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:246: // Actual system timezone
change will happen automatically.
Could you elaborate on 'automatically'? Where does that happen?

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:272: const bool user_is_primary
= user_manager->GetPrimaryUser() == user;
Move this to where it is used (better yet, just remove the local since it's only
used once)

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/timezone_util.h (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.h:40: bool
IfTimezonePrefsmanaged(const std::string& pref_name);
IsTimezonePrefsManaged

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:55: };
const char* const primary_user_prefs[] = {
...
};
const size_t primary_user_prefs_len = arraysize(primary_user_prefs);

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:698: return false;
Since this appears to be the only place we use |primary_user_prefs|, I think
it's probably more clear just to compare |pref_name| to each of the 4 pref names
here instead.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.html:55:
</settings-dropdown-menu>
Having multiple dropdowns with opposite 'hidden' values is pretty confusing. I
think this would be best as a single dropdown menu with
'pref="[[getTimeZonePref_(prefs.settings.timezone,
prefs.settings.resolve_timezone_by_geolocation.value)]]"'

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:70: // UI actually shows real
device timezone, but allows to change only user
s/to change only user timezone/only allows changing the user timezone/

[Alexander Alekseev, 2017-05-15 21:27:11]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 21:28:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 22:14:53]

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

[commit-bot: I haz the power, 2017-05-15 22:14:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[michaelpg, 2017-05-15 23:01:50]

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:194: if
(g_browser_process && g_browser_process->local_state()) {
this might be a silly question, but when does OOBE not have a browser process
and a LocalState?

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:212: timezone);
On 2017/05/15 17:08:02, stevenjb wrote:
> This should only be necessary inside the else, and only if
> (!customization_timezone.empty()), correct?

Seems that way, and then we wouldn't need to read in the pref value before
setting it.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/timezone_util.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:216: return true;
nit: use fall-through for trivial cases

[michaelpg, 2017-05-15 23:01:50]

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:194: if
(g_browser_process && g_browser_process->local_state()) {
this might be a silly question, but when does OOBE not have a browser process
and a LocalState?

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:212: timezone);
On 2017/05/15 17:08:02, stevenjb wrote:
> This should only be necessary inside the else, and only if
> (!customization_timezone.empty()), correct?

Seems that way, and then we wouldn't need to read in the pref value before
setting it.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/timezone_util.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:216: return true;
nit: use fall-through for trivial cases

[Alexander Alekseev, 2017-05-16 01:11:56]

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:194: if
(g_browser_process && g_browser_process->local_state()) {
On 2017/05/15 23:01:50, michaelpg wrote:
> this might be a silly question, but when does OOBE not have a browser process
> and a LocalState?

In unit_tests, I suppose. We have similar check in ShowLoginWizard().
I'll try to remove this check and see what I get.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:212: timezone);
On 2017/05/15 17:08:02, stevenjb wrote:
> This should only be necessary inside the else, and only if
> (!customization_timezone.empty()), correct?

Done.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:212: timezone);
On 2017/05/15 23:01:50, michaelpg wrote:
> On 2017/05/15 17:08:02, stevenjb wrote:
> > This should only be necessary inside the else, and only if
> > (!customization_timezone.empty()), correct?
> 
> Seems that way, and then we wouldn't need to read in the pref value before
> setting it.

We still need to set system timezone value here.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:213:
chromeos::CrosSettings::Get()->SetString(chromeos::kSystemTimezone, timezone);
On 2017/05/15 17:08:02, stevenjb wrote:
> What if timezone is empty, should we still be setting this? Also, could you
> document why we are setting the CrosSettings pref here?
> 

You are right, we should probably use system::TimezoneSettings here.
Added check for timezone correctness.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/wizard_controller.cc:1432:
CrosSettings::Get()->SetString(kSystemTimezone, timezone->timeZoneId);
On 2017/05/15 17:08:02, stevenjb wrote:
> Setting both of these in multiple places is a little confusing, maybe have a
> helper function somewhere instead, maybe in timezone_util?

Done.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/preferences.cc:351: : "");
On 2017/05/15 17:08:02, stevenjb wrote:
> |current_timezone_id_valid| isn't actually necessary, |current_timezone_id|
will
> be unset and therefore empty if GetString fails.
> 

Done.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/timezone_util.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:199: }
On 2017/05/15 17:08:02, stevenjb wrote:
> nit: blank line

Done.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:201: const bool
is_resolve_managed = local_state->IsManagedPreference(
On 2017/05/15 17:08:02, stevenjb wrote:
> I'm confused about what 'resolve' refers to here and below.

This was lame abbreviation of "resolve timezone automatically using your
location". But here it is probably simpler to remove it completely.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:216: return true;
On 2017/05/15 23:01:50, michaelpg wrote:
> nit: use fall-through for trivial cases

Done.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:246: // Actual system timezone
change will happen automatically.
On 2017/05/15 17:08:02, stevenjb wrote:
> Could you elaborate on 'automatically'? Where does that happen?
> 

Done.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:272: const bool user_is_primary
= user_manager->GetPrimaryUser() == user;
On 2017/05/15 17:08:02, stevenjb wrote:
> Move this to where it is used (better yet, just remove the local since it's
only
> used once)

Done.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/timezone_util.h (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.h:40: bool
IfTimezonePrefsmanaged(const std::string& pref_name);
On 2017/05/15 17:08:02, stevenjb wrote:
> IsTimezonePrefsManaged

Done.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:55: };
On 2017/05/15 17:08:02, stevenjb wrote:
> const char* const primary_user_prefs[] = {
> ...
> };
> const size_t primary_user_prefs_len = arraysize(primary_user_prefs);

Done.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:698: return false;
On 2017/05/15 17:08:02, stevenjb wrote:
> Since this appears to be the only place we use |primary_user_prefs|, I think
> it's probably more clear just to compare |pref_name| to each of the 4 pref
names
> here instead.

Done.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.html:55:
</settings-dropdown-menu>
On 2017/05/15 17:08:02, stevenjb wrote:
> Having multiple dropdowns with opposite 'hidden' values is pretty confusing. I
> think this would be best as a single dropdown menu with
> 'pref="[[getTimeZonePref_(prefs.settings.timezone,
> prefs.settings.resolve_timezone_by_geolocation.value)]]"'

as you see, preferences are two-way bound here. I don't know how to emulate this
using preference selector method.

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/2849823003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:70: // UI actually shows real
device timezone, but allows to change only user
On 2017/05/15 17:08:02, stevenjb wrote:
> s/to change only user timezone/only allows changing the user timezone/

Done.

[Alexander Alekseev, 2017-05-16 01:12:09]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 01:12:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 02:09:14]

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

[commit-bot: I haz the power, 2017-05-16 02:09:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stevenjb, 2017-05-16 17:16:57]

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/timezone_util.cc (right):

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:204: return false;
nit: {}

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:216: // fall-through
nit: 'fall-through' comment really isn't necessary and actually makes this a
little less clear imho.

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:247: // primary.
Better, but I'm still unclear where/how "preferences" will automatically do the
change. ("preferences" is a very large system).

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:274:
SetSigninScreenTimezone(value, false /* set_system_timezone */);
Is this the only place we call SetSigninScreenTimezone with
'set_system_timezone' = false?

If so, maybe just call
g_browser_process->local_state()->SetString(prefs::kSigninScreenTimezone, value)
directly here and remove the parameter from the helper function.

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:288: if (set_system_timezone) {
nit: if (!set_system_timezone) return;

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:696: return true;
{}

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.html:55:
</settings-dropdown-menu>
Good point. You could create a fake pref and map that to the correct pref, but
that is probably more hassle then it is worth.

Instead, I would suggest using dom-if instead of hidden, it will make the fact
that we only show one or the other dropdown a bit more clear.

[Alexander Alekseev, 2017-05-18 04:47:15]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 04:48:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 05:04:54]

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

[commit-bot: I haz the power, 2017-05-18 05:04:56]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Alexander Alekseev, 2017-05-18 05:59:03]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 05:59:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 06:10:57]

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

[commit-bot: I haz the power, 2017-05-18 06:10:58]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Alexander Alekseev, 2017-05-18 13:24:38]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 13:25:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 15:48:35]

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

[commit-bot: I haz the power, 2017-05-18 15:48:36]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2017-05-19 11:25:29]

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/timezone_util.cc (right):

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:204: return false;
On 2017/05/16 17:16:57, stevenjb wrote:
> nit: {}

Done.

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:216: // fall-through
On 2017/05/16 17:16:56, stevenjb wrote:
> nit: 'fall-through' comment really isn't necessary and actually makes this a
> little less clear imho.

Done.

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:247: // primary.
On 2017/05/16 17:16:57, stevenjb wrote:
> Better, but I'm still unclear where/how "preferences" will automatically do
the
> change. ("preferences" is a very large system).

From the point of view of English language, "pref" is an abbreviation for
"preferences".

But in Chromium, we have Chrome "Prefs" (which are "Pref[A-Z].*") and Chrome OS
"Preferences", which are different ;)

Added more clarification here.

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:274:
SetSigninScreenTimezone(value, false /* set_system_timezone */);
On 2017/05/16 17:16:57, stevenjb wrote:
> Is this the only place we call SetSigninScreenTimezone with
> 'set_system_timezone' = false?
> 
> If so, maybe just call
> g_browser_process->local_state()->SetString(prefs::kSigninScreenTimezone,
value)
> directly here and remove the parameter from the helper function.

Done.

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:288: if (set_system_timezone) {
On 2017/05/16 17:16:56, stevenjb wrote:
> nit: if (!set_system_timezone) return;

This line has been removed.

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:696: return true;
On 2017/05/16 17:16:57, stevenjb wrote:
> {}

Done.

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):

https://codereview.chromium.org/2849823003/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.html:55:
</settings-dropdown-menu>
On 2017/05/16 17:16:57, stevenjb wrote:
> Good point. You could create a fake pref and map that to the correct pref, but
> that is probably more hassle then it is worth.
> 
> Instead, I would suggest using dom-if instead of hidden, it will make the fact
> that we only show one or the other dropdown a bit more clear.

Done.

[Alexander Alekseev, 2017-05-19 11:25:42]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 11:26:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexander Alekseev, 2017-05-19 11:30:17]

alemate@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Alexander Alekseev, 2017-05-19 11:30:18]

+Pawel

phajdan.jr@: Please review chrome/test/*

The changes in chrome/test/ come from the fact that on ChoromeOS you need
CroSettings instance to create profile.

[commit-bot: I haz the power, 2017-05-19 12:31:03]

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

[commit-bot: I haz the power, 2017-05-19 12:31:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Alexander Alekseev, 2017-05-19 14:22:46]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 14:23:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 14:26:27]

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

[commit-bot: I haz the power, 2017-05-19 14:26:29]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexander Alekseev, 2017-05-19 14:42:50]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 14:43:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 14:56:25]

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

[commit-bot: I haz the power, 2017-05-19 14:56:27]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Alexander Alekseev, 2017-05-20 03:11:30]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-20 03:12:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-20 03:15:36]

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

[commit-bot: I haz the power, 2017-05-20 03:15:37]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexander Alekseev, 2017-05-24 14:53:59]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 14:54:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 15:07:26]

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

[commit-bot: I haz the power, 2017-05-24 15:07:27]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Alexander Alekseev, 2017-05-24 17:42:35]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 17:43:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 17:56:23]

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

[commit-bot: I haz the power, 2017-05-24 17:56:24]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Alexander Alekseev, 2017-05-24 19:25:23]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 19:26:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexander Alekseev, 2017-05-24 19:27:29]

Friendly ping.

[stevenjb, 2017-05-24 20:08:36]

On 2017/05/24 19:27:29, Alexander Alekseev wrote:
> Friendly ping.

Sorry, I missed the update message.

The latest patch set is pretty huge, can this be broken up some?

[stevenjb, 2017-05-24 20:08:52]

https://codereview.chromium.org/2849823003/diff/480001/chrome/browser/browsin...
File
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
(left):

https://codereview.chromium.org/2849823003/diff/480001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1386:
chromeos::ScopedTestCrosSettings test_cros_settings;
Are these changes related? Can they be separated?

[commit-bot: I haz the power, 2017-05-25 00:02:55]

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

[commit-bot: I haz the power, 2017-05-25 00:02:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Alexander Alekseev, 2017-05-25 00:30:05]

https://codereview.chromium.org/2849823003/diff/480001/chrome/browser/browsin...
File
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
(left):

https://codereview.chromium.org/2849823003/diff/480001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1386:
chromeos::ScopedTestCrosSettings test_cros_settings;
On 2017/05/24 20:08:52, stevenjb wrote:
> Are these changes related? Can they be separated?

ChromeOS preferences are now tied to CrosSettings (to make user timezone 
default to current timezone to make transition correct), and to
components/user_manager .

This means that TestingProfile now needs CrosSettings and UserManager (for
GetProfileByUser() ) to run.

So it was reasonable to initialize them all in TestingProfile. Than we need to
remove duplicate initialization from the tests that had it previously.

It also turned out, that TestingProfile is used also in browser_tests and
interactive_ui_tests. So it now initializes only objects that has not been
already initialized.

UserManager did not support running in unit_tests (this is now fixed in this
CL).


Then, there was an incorrect object initialization sequence in one test, and a
bug in chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc .


We can move tests update to a separate CL, but it won't be obvious why it is
done.

[stevenjb, 2017-05-25 00:37:23]

On 2017/05/25 00:30:05, Alexander Alekseev wrote:
>
https://codereview.chromium.org/2849823003/diff/480001/chrome/browser/browsin...
> File
> chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
> (left):
> 
>
https://codereview.chromium.org/2849823003/diff/480001/chrome/browser/browsin...
>
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1386:
> chromeos::ScopedTestCrosSettings test_cros_settings;
> On 2017/05/24 20:08:52, stevenjb wrote:
> > Are these changes related? Can they be separated?
> 
> ChromeOS preferences are now tied to CrosSettings (to make user timezone 
> default to current timezone to make transition correct), and to
> components/user_manager .
> 
> This means that TestingProfile now needs CrosSettings and UserManager (for
> GetProfileByUser() ) to run.
> 
> So it was reasonable to initialize them all in TestingProfile. Than we need to
> remove duplicate initialization from the tests that had it previously.
> 
> It also turned out, that TestingProfile is used also in browser_tests and
> interactive_ui_tests. So it now initializes only objects that has not been
> already initialized.
> 
> UserManager did not support running in unit_tests (this is now fixed in this
> CL).
> 
> 
> Then, there was an incorrect object initialization sequence in one test, and a
> bug in chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc .
> 
> 
> We can move tests update to a separate CL, but it won't be obvious why it is
> done.

I think that explaining why the test changes are being done in the CL will be
sufficient and it will allow us to land and test the test re-factoring
independent of the feature change, which is generally a good idea.

[Alexander Alekseev, 2017-05-25 00:45:24]

On 2017/05/25 00:30:05, Alexander Alekseev wrote:
>
https://codereview.chromium.org/2849823003/diff/480001/chrome/browser/browsin...
> File
> chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
> (left):
> 
>
https://codereview.chromium.org/2849823003/diff/480001/chrome/browser/browsin...
>
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1386:
> chromeos::ScopedTestCrosSettings test_cros_settings;
> On 2017/05/24 20:08:52, stevenjb wrote:
> > Are these changes related? Can they be separated?
> 
> ChromeOS preferences are now tied to CrosSettings (to make user timezone 
> default to current timezone to make transition correct), and to
> components/user_manager .
> 
> This means that TestingProfile now needs CrosSettings and UserManager (for
> GetProfileByUser() ) to run.
> 
> So it was reasonable to initialize them all in TestingProfile. Than we need to
> remove duplicate initialization from the tests that had it previously.
> 
> It also turned out, that TestingProfile is used also in browser_tests and
> interactive_ui_tests. So it now initializes only objects that has not been
> already initialized.
> 
> UserManager did not support running in unit_tests (this is now fixed in this
> CL).
> 
> 
> Then, there was an incorrect object initialization sequence in one test, and a
> bug in chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc .
> 
> 
> We can move tests update to a separate CL, but it won't be obvious why it is
> done.

I mechanically separated test files to a different Cl:
https://codereview.chromium.org/2907493002
Let's see if it builds.

[Alexander Alekseev, 2017-05-25 15:23:51]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 15:24:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 16:31:16]

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

[commit-bot: I haz the power, 2017-05-25 16:31:17]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2017-05-25 20:55:35]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 20:56:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stevenjb, 2017-05-25 22:13:06]

Thanks for separating out the test changes, this makes this much less
challenging to review.

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/info_private_api.cc (right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/info_private_api.cc:335: } else {
no else after return
Also, ideally the PerUserTimezoneEnabled() check might be better handled in a
helper function instead of scattered across the code like this, e.g. in
timezone_util.

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/network_screen.cc (right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen.cc:141:
chromeos::system::SetSystemAndSigninScreenTimezone(timezone_id);
Could we make this do the test and call CrosSettings::Get()->SetString if
system::PerUserTimezoneEnabled() is false instead?

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/timezone_util.h (right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.h:40: bool
IsTimezonePrefsManaged(const std::string& pref_name);
s/Prefs/Pref/

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:694: return true;
In the interest of readability, can we do this check first outside the
PerUserTimezoneEnabled() since it appears to apply to both cases?

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.html:55:
<settings-toggle-button
indent

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:115: }
This seems like another case where it would be better to just call a helper
method in timezone_util.

https://codereview.chromium.org/2849823003/diff/520001/chromeos/settings/cros...
File chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/2849823003/diff/520001/chromeos/settings/cros...
chromeos/settings/cros_settings_names.cc:249: const char
kPerUserTimezoneEnabled[] = "cros.flags.per_user_timezone_enabled";
Why are we mapping a flag to a pref? In particular I find the HTML/JS code that
uses prefs.cros.flags.per_user_timezone_enabled.value more complicated than it
probably needs to be. Unless this can change dynamically, we should just set the
value in loadTimeData like we do for other flags, e.g.:
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/chromeo...

[commit-bot: I haz the power, 2017-05-25 22:49:01]

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

[commit-bot: I haz the power, 2017-05-25 22:49:02]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2017-05-26 02:33:11]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 02:33:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 04:44:35]

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

[commit-bot: I haz the power, 2017-05-26 04:44:37]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2017-05-29 21:10:24]

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/info_private_api.cc (right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/info_private_api.cc:335: } else {
On 2017/05/25 22:13:06, stevenjb wrote:
> no else after return
> Also, ideally the PerUserTimezoneEnabled() check might be better handled in a
> helper function instead of scattered across the code like this, e.g. in
> timezone_util.

Done.

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/network_screen.cc (right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen.cc:141:
chromeos::system::SetSystemAndSigninScreenTimezone(timezone_id);
On 2017/05/25 22:13:06, stevenjb wrote:
> Could we make this do the test and call CrosSettings::Get()->SetString if
> system::PerUserTimezoneEnabled() is false instead?

SetSystemAndSigninScreenTimezone() is setting timezone directly (via
system::TimezoneSettings). CrosSettings interface is designed to be used for UI
API and does additional checks.

If I make this change, CrosSettings API will also be used in the other places,
where it could be wrong. On the contrary, replacing CrosSettings with
system::TimezoneSettings call will not exactly mimic the old behavior here.

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/timezone_util.h (right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.h:40: bool
IsTimezonePrefsManaged(const std::string& pref_name);
On 2017/05/25 22:13:06, stevenjb wrote:
> s/Prefs/Pref/

Done.

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:694: return true;
On 2017/05/25 22:13:06, stevenjb wrote:
> In the interest of readability, can we do this check first outside the
> PerUserTimezoneEnabled() since it appears to apply to both cases?
> 

Done.

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.html:55:
<settings-toggle-button
On 2017/05/25 22:13:06, stevenjb wrote:
> indent

Done.

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:115: }
On 2017/05/25 22:13:06, stevenjb wrote:
> This seems like another case where it would be better to just call a helper
> method in timezone_util.

Done.

https://codereview.chromium.org/2849823003/diff/520001/chromeos/settings/cros...
File chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/2849823003/diff/520001/chromeos/settings/cros...
chromeos/settings/cros_settings_names.cc:249: const char
kPerUserTimezoneEnabled[] = "cros.flags.per_user_timezone_enabled";
On 2017/05/25 22:13:06, stevenjb wrote:
> Why are we mapping a flag to a pref? In particular I find the HTML/JS code
that
> uses prefs.cros.flags.per_user_timezone_enabled.value more complicated than it
> probably needs to be. Unless this can change dynamically, we should just set
the
> value in loadTimeData like we do for other flags, e.g.:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/chromeo...

Done.

[Alexander Alekseev, 2017-05-29 21:11:37]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 21:12:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-29 21:53:38]

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

[commit-bot: I haz the power, 2017-05-29 21:53:39]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[stevenjb, 2017-05-30 16:47:18]

This is close, thanks. michaelpg@ can you take a quick look now, at least at the
WebUI and associated handlers?

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/network_screen.cc (right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen.cc:141:
chromeos::system::SetSystemAndSigninScreenTimezone(timezone_id);
On 2017/05/29 21:10:23, Alexander Alekseev wrote:
> On 2017/05/25 22:13:06, stevenjb wrote:
> > Could we make this do the test and call CrosSettings::Get()->SetString if
> > system::PerUserTimezoneEnabled() is false instead?
> 
> SetSystemAndSigninScreenTimezone() is setting timezone directly (via
> system::TimezoneSettings). CrosSettings interface is designed to be used for
UI
> API and does additional checks.
> 
> If I make this change, CrosSettings API will also be used in the other places,
> where it could be wrong. On the contrary, replacing CrosSettings with
> system::TimezoneSettings call will not exactly mimic the old behavior here.

I was suggesting:

SetSystemAndSigninScreenTimezone(const std::string& timezone) {
  if (!PerUserTimezoneEnabled()) {
    ... execute lines 143-149 from this file and return
  }
}

And remove the if() in line 136. timezone_util already accesses CrosSettings so
it wouldn't add any new dependencies there and it would simplify this UI code.
It doesn't look like that would impact any other calls to
SetSystemAndSigninScreenTimezone(), but perhaps I missed a call in which case I
guess this is fine as-is.

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:115: }
On 2017/05/29 21:10:23, Alexander Alekseev wrote:
> On 2017/05/25 22:13:06, stevenjb wrote:
> > This seems like another case where it would be better to just call a helper
> > method in timezone_util.
> 
> Done.

Again, this is logic I would prefer to see in timzone_util.cc, i.e. just call
system::SetUserTimezone() (or a wrapper function) and have that do the
PerUserTimezoneEnabled() check.

PerUserTimezoneEnabled() is really a model concept. It doesn't affect the UI so
the UI code should be as unaware of it as possible. 

Again, I won't insist, but having the PerUserTimzoneEnabled() tests scattered
across the UI code instead of mostly in timezone_util.cc is going to make the
change to remove the test that much harder when the time comes.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/info_private_api.cc (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/info_private_api.cc:225: } else {
no else after return

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/info_private_api.cc:232: }
On 2017/05/29 21:10:23, Alexander Alekseev wrote:
> On 2017/05/25 22:13:06, stevenjb wrote:
> > no else after return
> > Also, ideally the PerUserTimezoneEnabled() check might be better handled in
a
> > helper function instead of scattered across the code like this, e.g. in
> > timezone_util.
> 
> Done.
I actually think this code would be better off in timezone_util.cc where it can
potentially be shared with other similar code, but I won't insist.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:58: // controlled
or owner controlled.
This comment is duplicated below, remove it here.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:691: } else {
No else after return

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.js (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:111: ],
This is much cleaner, thanks!

[michaelpg, 2017-05-30 22:21:24]

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:479: // status.
This makes sense, but can you run it by Tom? Is this CL the right place to
change this UI?

I could also go with the opposite: if the primary user has a user policy but the
secondary user does not, the secondary user's settings are overridden because
the primary user's state is being applied (e.g., signed in with @google.com as
primary and @gmail.com as secondary). Is it better to give the user the root
reason (some policy somewhere) or the most immediate reason (go look at Primary
user's settings if you want to know more)?

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:710: // it should
appear as disabled, but not owned.
Isn't this idea (and code) already built into IsPrivilegedCrosSetting? It looks
like the line 718 would be triggered anyway for kSystemTimezone since the check
at line 714 would be false.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.html:71:
prefs.settings.resolve_timezone_by_geolocation.value)]]" restamp>
nit: break after a continuation line before the next attribute, "restamp"

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.html:80:
prefs.settings.resolve_timezone_by_geolocation.value)]]" restamp>
ditto

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.js (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:92: * Boolean
flag indicatined whether per-user timezone option is enabled.
"indicating"

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:201: if
(this.perUserTimezoneEnabled_) {
optional: use the "if" described in the comment.

if (this.timeZoneAutoDetect_) {
  if (this.perUserTimezoneEnabled_) ...

^ the reasoning is clearer that way IMO

Or, just use a ternary for the pref name depending on the value of
perUserTimezoneEnabled_?

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:116:
SetUserTimezone(web_ui()->GetWebContents()->GetBrowserContext(),
Please test this UI on the sign-in screen as well as Settings. What profile is
used on the sign-in screen in SetUserTimezone?

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

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:68: //
All Cros Settings are controlled.
All *other* Cros Settings?

https://codereview.chromium.org/2849823003/diff/600001/chrome/test/data/webui...
File chrome/test/data/webui/settings/date_time_page_tests.js (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/test/data/webui...
chrome/test/data/webui/settings/date_time_page_tests.js:17:
per_user_timezone_enabled: {
what is this?

https://codereview.chromium.org/2849823003/diff/600001/chrome/test/data/webui...
chrome/test/data/webui/settings/date_time_page_tests.js:50: 'USER_POLICY';
chrome.settingsPrivate.ControlledBy.USER_POLICY, etc.

[Alexander Alekseev, 2017-05-31 01:28:45]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 01:29:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 02:49:26]

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

[commit-bot: I haz the power, 2017-05-31 02:49:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2017-06-01 19:31:43]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 19:32:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexander Alekseev, 2017-07-06 00:38:08]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 00:38:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 00:56:05]

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

[commit-bot: I haz the power, 2017-07-06 00:56:07]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Alexander Alekseev, 2017-07-06 03:26:47]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 03:26:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 04:38:10]

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

[commit-bot: I haz the power, 2017-07-06 04:38:12]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2017-07-06 06:29:35]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[Alexander Alekseev, 2017-07-06 06:29:41]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 06:29:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexander Alekseev, 2017-07-06 06:30:29]

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/network_screen.cc (right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen.cc:141:
chromeos::system::SetSystemAndSigninScreenTimezone(timezone_id);
On 2017/05/30 16:47:18, stevenjb wrote:
> On 2017/05/29 21:10:23, Alexander Alekseev wrote:
> > On 2017/05/25 22:13:06, stevenjb wrote:
> > > Could we make this do the test and call CrosSettings::Get()->SetString if
> > > system::PerUserTimezoneEnabled() is false instead?
> > 
> > SetSystemAndSigninScreenTimezone() is setting timezone directly (via
> > system::TimezoneSettings). CrosSettings interface is designed to be used for
> UI
> > API and does additional checks.
> > 
> > If I make this change, CrosSettings API will also be used in the other
places,
> > where it could be wrong. On the contrary, replacing CrosSettings with
> > system::TimezoneSettings call will not exactly mimic the old behavior here.
> 
> I was suggesting:
> 
> SetSystemAndSigninScreenTimezone(const std::string& timezone) {
>   if (!PerUserTimezoneEnabled()) {
>     ... execute lines 143-149 from this file and return
>   }
> }
> 
> And remove the if() in line 136. timezone_util already accesses CrosSettings
so
> it wouldn't add any new dependencies there and it would simplify this UI code.
> It doesn't look like that would impact any other calls to
> SetSystemAndSigninScreenTimezone(), but perhaps I missed a call in which case
I
> guess this is fine as-is.

You're righ. Even more, SetSystemAndSigninScreenTimezone() seems to do exactly
the same as lines 143-148 now, so we can simply drop PerUserTimezoneEnabled()
check here.

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/2849823003/diff/520001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:115: }
On 2017/05/30 16:47:18, stevenjb wrote:
> On 2017/05/29 21:10:23, Alexander Alekseev wrote:
> > On 2017/05/25 22:13:06, stevenjb wrote:
> > > This seems like another case where it would be better to just call a
helper
> > > method in timezone_util.
> > 
> > Done.
> 
> Again, this is logic I would prefer to see in timzone_util.cc, i.e. just call
> system::SetUserTimezone() (or a wrapper function) and have that do the
> PerUserTimezoneEnabled() check.
> 
> PerUserTimezoneEnabled() is really a model concept. It doesn't affect the UI
so
> the UI code should be as unaware of it as possible. 
> 
> Again, I won't insist, but having the PerUserTimzoneEnabled() tests scattered
> across the UI code instead of mostly in timezone_util.cc is going to make the
> change to remove the test that much harder when the time comes.

Done.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/info_private_api.cc (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/info_private_api.cc:225: } else {
On 2017/05/30 16:47:18, stevenjb wrote:
> no else after return

Done.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/info_private_api.cc:232: }
On 2017/05/30 16:47:18, stevenjb wrote:
> On 2017/05/29 21:10:23, Alexander Alekseev wrote:
> > On 2017/05/25 22:13:06, stevenjb wrote:
> > > no else after return
> > > Also, ideally the PerUserTimezoneEnabled() check might be better handled
in
> a
> > > helper function instead of scattered across the code like this, e.g. in
> > > timezone_util.
> > 
> > Done.
> I actually think this code would be better off in timezone_util.cc where it
can
> potentially be shared with other similar code, but I won't insist.

This is very UI-specific. I'd leave it here.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:58: // controlled
or owner controlled.
On 2017/05/30 16:47:18, stevenjb wrote:
> This comment is duplicated below, remove it here.

This comment is correct only if per-user time zone is not enabled.
It would be more correct to put the second comment inside else{}block, but I
haven't seen anything like that in chromium code ;)

The idea behind the current "if(!chromeos::system::PerUserTimezoneEnabled())"
checks implementation is to make it more clear that it is backward compatible.
So I am trying to keep old code in-place and wrap it with checks.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:479: // status.
On 2017/05/30 22:21:23, michaelpg wrote:
> This makes sense, but can you run it by Tom? Is this CL the right place to
> change this UI?
> 
> I could also go with the opposite: if the primary user has a user policy but
the
> secondary user does not, the secondary user's settings are overridden because
> the primary user's state is being applied (e.g., signed in with @google.com as
> primary and @gmail.com as secondary). Is it better to give the user the root
> reason (some policy somewhere) or the most immediate reason (go look at
Primary
> user's settings if you want to know more)?

This is a good point, but if current user doesn't have policy for this value, it
won't be enterprise managed (thus this check will be false).
So this doesn't affect user policies.

I did this for system preferences, which are device-policy managed.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:691: } else {
On 2017/05/30 16:47:18, stevenjb wrote:
> No else after return

Done.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:710: // it should
appear as disabled, but not owned.
On 2017/05/30 22:21:23, michaelpg wrote:
> Isn't this idea (and code) already built into IsPrivilegedCrosSetting? It
looks
> like the line 718 would be triggered anyway for kSystemTimezone since the
check
> at line 714 would be false.

IsPrivilegedCrosSetting() depends on whether per-user time zone is enabled.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.html:71:
prefs.settings.resolve_timezone_by_geolocation.value)]]" restamp>
On 2017/05/30 22:21:23, michaelpg wrote:
> nit: break after a continuation line before the next attribute, "restamp"

Done.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.html:80:
prefs.settings.resolve_timezone_by_geolocation.value)]]" restamp>
On 2017/05/30 22:21:23, michaelpg wrote:
> ditto

Done.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.js (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:92: * Boolean
flag indicatined whether per-user timezone option is enabled.
On 2017/05/30 22:21:23, michaelpg wrote:
> "indicating"

Done.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:111: ],
On 2017/05/30 16:47:18, stevenjb wrote:
> This is much cleaner, thanks!

Done.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:201: if
(this.perUserTimezoneEnabled_) {
On 2017/05/30 22:21:23, michaelpg wrote:
> optional: use the "if" described in the comment.
> 
> if (this.timeZoneAutoDetect_) {
>   if (this.perUserTimezoneEnabled_) ...
> 
> ^ the reasoning is clearer that way IMO
> 
> Or, just use a ternary for the pref name depending on the value of
> perUserTimezoneEnabled_?

Done.

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/set_time_ui.cc (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/set_time_ui.cc:116:
SetUserTimezone(web_ui()->GetWebContents()->GetBrowserContext(),
On 2017/05/30 22:21:23, michaelpg wrote:
> Please test this UI on the sign-in screen as well as Settings. What profile is
> used on the sign-in screen in SetUserTimezone?

SigninProfile.

I updated this code (in timezone_util now) so that it will work for the signin
screen.

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

https://codereview.chromium.org/2849823003/diff/600001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:68: //
All Cros Settings are controlled.
On 2017/05/30 22:21:23, michaelpg wrote:
> All *other* Cros Settings?

Done.

(The idea was to leave a comment that won't change after removing of
PerUserTimezoneEnabled() check above.)

https://codereview.chromium.org/2849823003/diff/600001/chrome/test/data/webui...
File chrome/test/data/webui/settings/date_time_page_tests.js (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/test/data/webui...
chrome/test/data/webui/settings/date_time_page_tests.js:17:
per_user_timezone_enabled: {
On 2017/05/30 22:21:23, michaelpg wrote:
> what is this?

This flag is generated in
chrome/browser/chromeos/settings/system_settings_provider.cc

(I don't understand why this file is not in this patchset, but it is included in
the last one.)

Also I don't know how to run all the tests here for both values of this flag,
but at least I tested that it works manually. ;)

https://codereview.chromium.org/2849823003/diff/600001/chrome/test/data/webui...
chrome/test/data/webui/settings/date_time_page_tests.js:50: 'USER_POLICY';
On 2017/05/30 22:21:24, michaelpg wrote:
> chrome.settingsPrivate.ControlledBy.USER_POLICY, etc.

Done.

[commit-bot: I haz the power, 2017-07-06 07:43:13]

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

[commit-bot: I haz the power, 2017-07-06 07:43:15]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-07-10 19:10:53]

Close.

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/chromeo...
File chrome/browser/chromeos/BUILD.gn (right):

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/chromeo...
chrome/browser/chromeos/BUILD.gn:1863:
"settings/scoped_cros_settings_test_helper.h",
Why are we removing the .cc file here and not the .h file?

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/info_private_api.cc (right):

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/info_private_api.cc:335: } else {
No else

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/timezone_util.cc (right):

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:327: }
Is it expected that this might sometimes do nothing? If so, add a comment (and
maybe update the header comment), if not, add NOTREACHED().

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:58: // controlled
or owner controlled.
I still think this is confusing. This comment is almost identical to the one
below. It is perfectly clear that the comment below applies to settings other
than kSystemTimezone. Please remove this one.

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:60: // All Cros
settings are considered privileged and are either policy
You can remove the 'All' to make this a bit more clear for both cases.

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:715: }
The above is confusing to read and duplicates logic. I believe the following
would be equivalent and more clear:

policy::BrowserPolicyConnectorChromeOS* connector =
  g_browser_process->platform_part()->browser_policy_connector_chromeos();
if (connector->IsEnterpriseManaged()) {
  if (IsPrivilegedCrosSetting(pref_name))
    return true;
  if (chromeos::system::PerUserTimezoneEnabled() &&
      (pref_name == prefs::kUserTimezone ||
       pref_name == prefs::kResolveTimezoneByGeolocation)) {
    return chromeos::system::IsTimezonePrefsManaged(pref_name);
  }
}

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.html:59:
</settings-toggle-button>
Indent

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.js (right):

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:226:
(prefResolveValue == true);
nit: () not needed for ==

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:238: if
(!this.getPref('cros.flags.per_user_timezone_enabled').value) {
invert and early exit

[michaelpg, 2017-07-10 19:29:37]

https://codereview.chromium.org/2849823003/diff/600001/chrome/test/data/webui...
File chrome/test/data/webui/settings/date_time_page_tests.js (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/test/data/webui...
chrome/test/data/webui/settings/date_time_page_tests.js:17:
per_user_timezone_enabled: {
On 2017/07/06 06:30:29, Alexander Alekseev wrote:
> On 2017/05/30 22:21:23, michaelpg wrote:
> > what is this?
> 
> This flag is generated in
> chrome/browser/chromeos/settings/system_settings_provider.cc
> 
> (I don't understand why this file is not in this patchset, but it is included
in
> the last one.)
> 
> Also I don't know how to run all the tests here for both values of this flag,
> but at least I tested that it works manually. ;)

that's not a bad idea though. add a TODO here to run the tests once for each
value, and I'll follow up later with you and dpapad about it later.

[michaelpg, 2017-07-10 19:29:37]

https://codereview.chromium.org/2849823003/diff/600001/chrome/test/data/webui...
File chrome/test/data/webui/settings/date_time_page_tests.js (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/test/data/webui...
chrome/test/data/webui/settings/date_time_page_tests.js:17:
per_user_timezone_enabled: {
On 2017/07/06 06:30:29, Alexander Alekseev wrote:
> On 2017/05/30 22:21:23, michaelpg wrote:
> > what is this?
> 
> This flag is generated in
> chrome/browser/chromeos/settings/system_settings_provider.cc
> 
> (I don't understand why this file is not in this patchset, but it is included
in
> the last one.)
> 
> Also I don't know how to run all the tests here for both values of this flag,
> but at least I tested that it works manually. ;)

that's not a bad idea though. add a TODO here to run the tests once for each
value, and I'll follow up later with you and dpapad about it later.

[Alexander Alekseev, 2017-07-14 03:32:47]

https://codereview.chromium.org/2849823003/diff/600001/chrome/test/data/webui...
File chrome/test/data/webui/settings/date_time_page_tests.js (right):

https://codereview.chromium.org/2849823003/diff/600001/chrome/test/data/webui...
chrome/test/data/webui/settings/date_time_page_tests.js:17:
per_user_timezone_enabled: {
On 2017/07/10 19:29:37, michaelpg wrote:
> On 2017/07/06 06:30:29, Alexander Alekseev wrote:
> > On 2017/05/30 22:21:23, michaelpg wrote:
> > > what is this?
> > 
> > This flag is generated in
> > chrome/browser/chromeos/settings/system_settings_provider.cc
> > 
> > (I don't understand why this file is not in this patchset, but it is
included
> in
> > the last one.)
> > 
> > Also I don't know how to run all the tests here for both values of this
flag,
> > but at least I tested that it works manually. ;)
> 
> that's not a bad idea though. add a TODO here to run the tests once for each
> value, and I'll follow up later with you and dpapad about it later.

Done.

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/chromeo...
File chrome/browser/chromeos/BUILD.gn (right):

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/chromeo...
chrome/browser/chromeos/BUILD.gn:1863:
"settings/scoped_cros_settings_test_helper.h",
On 2017/07/10 19:10:52, stevenjb wrote:
> Why are we removing the .cc file here and not the .h file?

Done.

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/info_private_api.cc (right):

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/info_private_api.cc:335: } else {
On 2017/07/10 19:10:52, stevenjb wrote:
> No else

Done.

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/timezone_util.cc (right):

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:327: }
On 2017/07/10 19:10:52, stevenjb wrote:
> Is it expected that this might sometimes do nothing? If so, add a comment (and
> maybe update the header comment), if not, add NOTREACHED().

Done.

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:58: // controlled
or owner controlled.
On 2017/07/10 19:10:52, stevenjb wrote:
> I still think this is confusing. This comment is almost identical to the one
> below. It is perfectly clear that the comment below applies to settings other
> than kSystemTimezone. Please remove this one.

Done.

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:60: // All Cros
settings are considered privileged and are either policy
On 2017/07/10 19:10:52, stevenjb wrote:
> You can remove the 'All' to make this a bit more clear for both cases.

Done.

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:715: }
On 2017/07/10 19:10:52, stevenjb wrote:
> The above is confusing to read and duplicates logic. I believe the following
> would be equivalent and more clear:
> 
> policy::BrowserPolicyConnectorChromeOS* connector =
>   g_browser_process->platform_part()->browser_policy_connector_chromeos();
> if (connector->IsEnterpriseManaged()) {
>   if (IsPrivilegedCrosSetting(pref_name))
>     return true;
>   if (chromeos::system::PerUserTimezoneEnabled() &&
>       (pref_name == prefs::kUserTimezone ||
>        pref_name == prefs::kResolveTimezoneByGeolocation)) {
>     return chromeos::system::IsTimezonePrefsManaged(pref_name);
>   }
> }

Done.

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.html:59:
</settings-toggle-button>
On 2017/07/10 19:10:52, stevenjb wrote:
> Indent

Done.

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.js (right):

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:226:
(prefResolveValue == true);
On 2017/07/10 19:10:53, stevenjb wrote:
> nit: () not needed for ==

Done.

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:238: if
(!this.getPref('cros.flags.per_user_timezone_enabled').value) {
On 2017/07/10 19:10:53, stevenjb wrote:
> invert and early exit

Done.

[stevenjb, 2017-07-17 16:57:11]

lgtm!

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:695: if
(connector->IsEnterpriseManaged()) {
Much more clear now, thanks!

nit: Now it looks like we can do:

if (!connector->IsEnterpriseManaged())
  return false;

[michaelpg, 2017-07-17 18:17:05]

One non-nit, and some closure stuff.

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.js (right):

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:193:
(this.getPref('cros.flags.per_user_timezone_enabled').value ?
nit: maybe split this out into a separate variable for readability

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:219: * @param
prefUserTimezone Object pref.settings.timezone
@param {?Pref} prefUserTimezone pref.settings.timezone

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:220: * @param
prefResolveValue Boolean
@param {boolean} prefResolveValue

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:226:
prefResolveValue == true;
nit: no need for the equality test here, prefResolveValue is a bool

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:230: *
Observer of this.prefs.cros.flags.per_user_timezone_enabled.value .
nit: remove space before period

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:243: //
before switching to polymer 2.0 .
ditto

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:244:
this['_addComplexObserverEffect'].call(
This looks really scary. Can we just add this as an observer normally and do
this logic in the callback?

[Alexander Alekseev, 2017-07-20 00:30:51]

alemate@chromium.org changed reviewers:
+ isherman@chromium.org

[Alexander Alekseev, 2017-07-20 00:30:54]

+isherman

Please review tools/metrics/histograms/histograms.xml .

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:695: if
(connector->IsEnterpriseManaged()) {
On 2017/07/17 16:57:11, stevenjb wrote:
> Much more clear now, thanks!
> 
> nit: Now it looks like we can do:
> 
> if (!connector->IsEnterpriseManaged())
>   return false;

Done.

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
File chrome/browser/resources/settings/date_time_page/date_time_page.js (right):

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:193:
(this.getPref('cros.flags.per_user_timezone_enabled').value ?
On 2017/07/17 18:17:05, michaelpg wrote:
> nit: maybe split this out into a separate variable for readability

Done.

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:219: * @param
prefUserTimezone Object pref.settings.timezone
On 2017/07/17 18:17:05, michaelpg wrote:
> @param {?Pref} prefUserTimezone pref.settings.timezone

Done.

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:220: * @param
prefResolveValue Boolean
On 2017/07/17 18:17:05, michaelpg wrote:
> @param {boolean} prefResolveValue

Done.

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:226:
prefResolveValue == true;
On 2017/07/17 18:17:04, michaelpg wrote:
> nit: no need for the equality test here, prefResolveValue is a bool

Done.

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:230: *
Observer of this.prefs.cros.flags.per_user_timezone_enabled.value .
On 2017/07/17 18:17:05, michaelpg wrote:
> nit: remove space before period

Done.

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:243: //
before switching to polymer 2.0 .
On 2017/07/17 18:17:05, michaelpg wrote:
> ditto

Done.

https://codereview.chromium.org/2849823003/diff/740001/chrome/browser/resourc...
chrome/browser/resources/settings/date_time_page/date_time_page.js:244:
this['_addComplexObserverEffect'].call(
On 2017/07/17 18:17:05, michaelpg wrote:
> This looks really scary. Can we just add this as an observer normally and do
> this logic in the callback?

Done.

[Ilya Sherman, 2017-07-20 00:56:43]

metrics xml files lgtm

[Alexander Alekseev, 2017-07-21 06:10:03]

Michael, Pawel, friendly ping.

[michaelpg, 2017-07-24 06:57:40]

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

https://codereview.chromium.org/2849823003/diff/760001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:52:
const char* kReadOnlySettings[] = {
the name and comment don't match up here...

https://codereview.chromium.org/2849823003/diff/760001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:221:
dict->Set("value", base::MakeUnique<base::Value>(*pref->GetValue()));
Why does PerUserTimezoneEnabled() imply that whatever pref "pref_name" refers to
isn't a boolean?

https://codereview.chromium.org/2849823003/diff/760001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2849823003/diff/760001/chrome/common/pref_nam...
chrome/common/pref_names.cc:2028: // This setting controlls initial device
timezone that is used before user
controls

https://codereview.chromium.org/2849823003/diff/760001/chromeos/chromeos_swit...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/2849823003/diff/760001/chromeos/chromeos_swit...
chromeos/chromeos_switches.cc:498: // Disables per-user timezone
nit: ending period

https://codereview.chromium.org/2849823003/diff/760001/chromeos/settings/cros...
File chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/2849823003/diff/760001/chromeos/settings/cros...
chromeos/settings/cros_settings_names.cc:250: }  // namespace chromeos
nit: space before brace, like before

[Alexander Alekseev, 2017-07-29 06:10:16]

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

https://codereview.chromium.org/2849823003/diff/760001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:52:
const char* kReadOnlySettings[] = {
On 2017/07/24 06:57:39, michaelpg wrote:
> the name and comment don't match up here...

Done.

https://codereview.chromium.org/2849823003/diff/760001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:221:
dict->Set("value", base::MakeUnique<base::Value>(*pref->GetValue()));
On 2017/07/24 06:57:39, michaelpg wrote:
> Why does PerUserTimezoneEnabled() imply that whatever pref "pref_name" refers
to
> isn't a boolean?

Why? base::MakeUnique<base::Value>(*pref->GetValue()) will make a copy of value
and keep its type.

Set(std::string, base::Value*) will use the same value keeping type.
So now value can be of any value type, including boolean.

https://codereview.chromium.org/2849823003/diff/760001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2849823003/diff/760001/chrome/common/pref_nam...
chrome/common/pref_names.cc:2028: // This setting controlls initial device
timezone that is used before user
On 2017/07/24 06:57:40, michaelpg wrote:
> controls

Done.

https://codereview.chromium.org/2849823003/diff/760001/chromeos/chromeos_swit...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/2849823003/diff/760001/chromeos/chromeos_swit...
chromeos/chromeos_switches.cc:498: // Disables per-user timezone
On 2017/07/24 06:57:40, michaelpg wrote:
> nit: ending period

Done.

https://codereview.chromium.org/2849823003/diff/760001/chromeos/settings/cros...
File chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/2849823003/diff/760001/chromeos/settings/cros...
chromeos/settings/cros_settings_names.cc:250: }  // namespace chromeos
On 2017/07/24 06:57:40, michaelpg wrote:
> nit: space before brace, like before

Done.

[Alexander Alekseev, 2017-08-01 02:01:34]

On 2017/07/29 06:10:16, Alexander Alekseev wrote:
>
https://codereview.chromium.org/2849823003/diff/760001/chrome/browser/ui/webu...
> File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
> (right):
> 
>
https://codereview.chromium.org/2849823003/diff/760001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:52:
> const char* kReadOnlySettings[] = {
> On 2017/07/24 06:57:39, michaelpg wrote:
> > the name and comment don't match up here...
> 
> Done.
> 
>
https://codereview.chromium.org/2849823003/diff/760001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:221:
> dict->Set("value", base::MakeUnique<base::Value>(*pref->GetValue()));
> On 2017/07/24 06:57:39, michaelpg wrote:
> > Why does PerUserTimezoneEnabled() imply that whatever pref "pref_name"
refers
> to
> > isn't a boolean?
> 
> Why? base::MakeUnique<base::Value>(*pref->GetValue()) will make a copy of
value
> and keep its type.
> 
> Set(std::string, base::Value*) will use the same value keeping type.
> So now value can be of any value type, including boolean.
> 
>
https://codereview.chromium.org/2849823003/diff/760001/chrome/common/pref_nam...
> File chrome/common/pref_names.cc (right):
> 
>
https://codereview.chromium.org/2849823003/diff/760001/chrome/common/pref_nam...
> chrome/common/pref_names.cc:2028: // This setting controlls initial device
> timezone that is used before user
> On 2017/07/24 06:57:40, michaelpg wrote:
> > controls
> 
> Done.
> 
>
https://codereview.chromium.org/2849823003/diff/760001/chromeos/chromeos_swit...
> File chromeos/chromeos_switches.cc (right):
> 
>
https://codereview.chromium.org/2849823003/diff/760001/chromeos/chromeos_swit...
> chromeos/chromeos_switches.cc:498: // Disables per-user timezone
> On 2017/07/24 06:57:40, michaelpg wrote:
> > nit: ending period
> 
> Done.
> 
>
https://codereview.chromium.org/2849823003/diff/760001/chromeos/settings/cros...
> File chromeos/settings/cros_settings_names.cc (right):
> 
>
https://codereview.chromium.org/2849823003/diff/760001/chromeos/settings/cros...
> chromeos/settings/cros_settings_names.cc:250: }  // namespace chromeos
> On 2017/07/24 06:57:40, michaelpg wrote:
> > nit: space before brace, like before
> 
> Done.

Michael, friendly ping.

[michaelpg, 2017-08-02 19:31:28]

lgtm

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

https://codereview.chromium.org/2849823003/diff/760001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:221:
dict->Set("value", base::MakeUnique<base::Value>(*pref->GetValue()));
On 2017/07/29 06:10:16, Alexander Alekseev wrote:
> On 2017/07/24 06:57:39, michaelpg wrote:
> > Why does PerUserTimezoneEnabled() imply that whatever pref "pref_name"
refers
> to
> > isn't a boolean?
> 
> Why? base::MakeUnique<base::Value>(*pref->GetValue()) will make a copy of
value
> and keep its type.
> 
> Set(std::string, base::Value*) will use the same value keeping type.
> So now value can be of any value type, including boolean.

You're right, I think I was confused.

[Alexander Alekseev, 2017-08-03 00:33:16]

Pawel, please review chrome/test/*

[Alexander Alekseev, 2017-08-03 09:18:02]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-03 09:18:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-03 09:57:05]

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

[commit-bot: I haz the power, 2017-08-03 09:57:06]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Alexander Alekseev, 2017-08-03 11:05:19]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-03 11:05:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexander Alekseev, 2017-08-03 11:15:02]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-03 11:15:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-03 12:22:49]

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

[commit-bot: I haz the power, 2017-08-03 12:22:51]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2017-08-07 20:42:12]

Description was changed from

==========
ChromeOS: implement per-user time zone preferences.

Historically ChromeOS had only one global time zone preference. But now time
settings become complicate and we need to support per-user time zone
preferences.

BUG=622555,699472
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
ChromeOS: implement per-user time zone preferences.

Historically ChromeOS had only one global time zone preference. But now time
settings become complicate and we need to support per-user time zone
preferences.

TBR=phajdan.jr@chromium.org

BUG=622555,699472
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Alexander Alekseev, 2017-08-07 20:42:42]

TBR Pawel for test.

[Alexander Alekseev, 2017-08-07 20:43:00]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2017-08-07 20:43:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, isherman@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2849823003/#ps820001
(title: "Fixed closure compilation.")

[commit-bot: I haz the power, 2017-08-07 20:43:14]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-07 20:47:11]

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

[commit-bot: I haz the power, 2017-08-07 20:47:14]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[Alexander Alekseev, 2017-08-11 04:07:17]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2017-08-11 04:07:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, isherman@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2849823003/#ps840001
(title: "Rebased.")

[commit-bot: I haz the power, 2017-08-11 04:07:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-11 05:22:19]

CQ is committing da patch.

Bot data: {"patchset_id": 840001, "attempt_start_ts": 1502424437247560,
"parent_rev": "e6cf2c3cd8720693a2b07744ccf9d42cda14f423", "commit_rev":
"2cb0d75387b3fbf61e1d37a7e9b902a87842b363"}

[commit-bot: I haz the power, 2017-08-11 05:23:00]

Description was changed from

==========
ChromeOS: implement per-user time zone preferences.

Historically ChromeOS had only one global time zone preference. But now time
settings become complicate and we need to support per-user time zone
preferences.

TBR=phajdan.jr@chromium.org

BUG=622555,699472
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
ChromeOS: implement per-user time zone preferences.

Historically ChromeOS had only one global time zone preference. But now time
settings become complicate and we need to support per-user time zone
preferences.

TBR=phajdan.jr@chromium.org

BUG=622555,699472
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2849823003
Cr-Commit-Position: refs/heads/master@{#493673}
Committed:
https://chromium.googlesource.com/chromium/src/+/2cb0d75387b3fbf61e1d37a7e9b9...
==========

[commit-bot: I haz the power, 2017-08-11 05:23:03]

Committed patchset #43 (id:840001) as
https://chromium.googlesource.com/chromium/src/+/2cb0d75387b3fbf61e1d37a7e9b9...

[Lei Zhang, 2017-08-31 08:15:59]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2017-08-31 08:16:02]

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/timezone_util.cc (right):

https://codereview.chromium.org/2849823003/diff/720001/chrome/browser/chromeo...
chrome/browser/chromeos/system/timezone_util.cc:327: }
On 2017/07/14 03:32:46, Alexander Alekseev wrote:
> On 2017/07/10 19:10:52, stevenjb wrote:
> > Is it expected that this might sometimes do nothing? If so, add a comment
(and
> > maybe update the header comment), if not, add NOTREACHED().
> 
> Done.

What landed is as follows:

Profile* primary_profile = ProfileManager::GetPrimaryUserProfile();
if (primary_profile && profile->IsSameProfile(primary_profile)) {
  profile->GetPrefs()->SetString(prefs::kUserTimezone, timezone_id);
}
// Time zone UI should be blocked for non-primary users.
NOTREACHED();

Isn't that missing a return in the if block? Otherwise, isn't it really each to
reach the NOTREACHED()? I'm also concerned this is not being covered by any
tests.

[Simon Que, 2017-09-07 22:15:53]

This breaks at least one kiosk app in M62. Do not merge this to M61.

http://crbug.com/759404