MD Settings: Date and Time page, part 1/3

MD Settings: Date and Time page, part 1/3

Modernizes the UI a bit and adds a DateTimeHandler for policy stuff
(notifications TBD) and to provide the time zone list (TBD).

BUG=546835, 460542
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/614d31daee2f61b0180df403a8ad43f20b9f6dd7
Cr-Commit-Position: refs/heads/master@{#423768}

[michaelpg, 2016-10-06 03:40:11]

Description was changed from

==========
MD Settings: Date and Time page, part 1/3

Modernizes the UI a bit and adds a DateTimeHandler for policy stuff
(notifications TBD) and to provide the time zone list (TBD).

BUG=546835,460542
R=dpapad@chromium.org
==========

to

==========
MD Settings: Date and Time page, part 1/3

Modernizes the UI a bit and adds a DateTimeHandler for policy stuff
(notifications TBD) and to provide the time zone list (TBD).

BUG=546835,460542
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-10-06 03:50:09]

dpapad, have some cycles for some Date & Time reviews?

https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(left):

https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/date_time_page/date_time_page.html:11:
$i18n{timeZone}
didn't delete from settings_strings.grdp because it's going back in next CL.

[dpapad, 2016-10-06 17:31:02]

On 2016/10/06 at 03:50:09, michaelpg wrote:
> dpapad, have some cycles for some Date & Time reviews?

I'll get to this around lunch time (before or shortly after).

> 
>
https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/date_time_page/date_time_page.html
(left):
> 
>
https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/date_time_page/date_time_page.html:11:
$i18n{timeZone}
> didn't delete from settings_strings.grdp because it's going back in next CL.

[dpapad, 2016-10-06 18:01:14]

LGTM, except for the 
chrome/browser/extensions/api/settings_private/prefs_util.cc change, which I am
not familiar with. Is it adding a new pref? Or is it just making prefs_util.cc
use an existing pref?

https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):

https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/date_time_page/date_time_page.html:10: <!--
TODO(michaelpg): Time zone dropdown menu. -->
Just FYI, when you get to this, let's use <select class="md-select">. There are
plenty examples in the codebase of native selects styled as MD now.

https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/date_time_page/date_time_page.html:15:
<paper-checkbox class="settings-box continuation"
Is there a preference of hiding this checkbox with hidden VS wrapping it in a
<template is="dom-if" if="[[hideTimeZoneDetection_]]" ? Perhaps the latter is
slightly cheaper?

https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/date_time_handler.cc (right):

https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/date_time_handler.cc:65:
html_source->AddBoolean("hideTimeZoneDetection", true);
How about always setting this boolean, so that you don't have to call both
valueExists() and getBoolean() in JS?

html_source->AddBoolean(
    "hideTimeZoneDetection",
    base::CommandLine::ForCurrentProcess()->HasSwitch(
        chromeos::switches::kDisableTimeZoneTrackingOption));

[michaelpg, 2016-10-06 18:22:50]

Thanks for the review. I've opened https://crbug.com/653625 to track removing
the time zone geolocation flag, by the way.

On 2016/10/06 18:01:14, dpapad wrote:
> LGTM, except for the 
> chrome/browser/extensions/api/settings_private/prefs_util.cc change, which I
am
> not familiar with. Is it adding a new pref? Or is it just making prefs_util.cc
> use an existing pref?

This "whitelist" is the list of prefs settingsPrivate returns via getAllPrefs(),
which also makes it the list of prefs that <settings-prefs> stores in |prefs|.
(Any pref can be fetched or set via getPref/setPref, however.)

> 
>
https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/date_time_page/date_time_page.html
> (right):
> 
>
https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/date_time_page/date_time_page.html:10: <!--
> TODO(michaelpg): Time zone dropdown menu. -->
> Just FYI, when you get to this, let's use <select class="md-select">. There
are
> plenty examples in the codebase of native selects styled as MD now.
> 
>
https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/date_time_page/date_time_page.html:15:
> <paper-checkbox class="settings-box continuation"
> Is there a preference of hiding this checkbox with hidden VS wrapping it in a
> <template is="dom-if" if="[[hideTimeZoneDetection_]]" ? Perhaps the latter is
> slightly cheaper?
> 
>
https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/settings/chromeos/date_time_handler.cc (right):
> 
>
https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/settings/chromeos/date_time_handler.cc:65:
> html_source->AddBoolean("hideTimeZoneDetection", true);
> How about always setting this boolean, so that you don't have to call both
> valueExists() and getBoolean() in JS?
> 
> html_source->AddBoolean(
>     "hideTimeZoneDetection",
>     base::CommandLine::ForCurrentProcess()->HasSwitch(
>         chromeos::switches::kDisableTimeZoneTrackingOption));

https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):

https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/date_time_page/date_time_page.html:10: <!--
TODO(michaelpg): Time zone dropdown menu. -->
On 2016/10/06 18:01:14, dpapad wrote:
> Just FYI, when you get to this, let's use <select class="md-select">. There
are
> plenty examples in the codebase of native selects styled as MD now.

I assume this means it's also okay to use settings-dropdown-menu here (since it
wraps a <select class="md-select">)? I'd like to use its functionality here.

https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/date_time_page/date_time_page.html:15:
<paper-checkbox class="settings-box continuation"
On 2016/10/06 18:01:14, dpapad wrote:
> Is there a preference of hiding this checkbox with hidden VS wrapping it in a
> <template is="dom-if" if="[[hideTimeZoneDetection_]]" ? Perhaps the latter is
> slightly cheaper?

Yes, definitely: A <template is="dom-if"> introduces extra overhead. Not much,
but if the majority of cases will show the contents immediately, we shouldn't
use <template> unless it's reeeally big.

hideTimeZoneDetection_ is controlled by a --command-line-flag so it is
definitely not the common case, and I'm not worried about one unnecessary
paper-checkbox when people enable that flag.

https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/date_time_handler.cc (right):

https://codereview.chromium.org/2393703005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/date_time_handler.cc:65:
html_source->AddBoolean("hideTimeZoneDetection", true);
On 2016/10/06 18:01:14, dpapad wrote:
> How about always setting this boolean, so that you don't have to call both
> valueExists() and getBoolean() in JS?
> 
> html_source->AddBoolean(
>     "hideTimeZoneDetection",
>     base::CommandLine::ForCurrentProcess()->HasSwitch(
>         chromeos::switches::kDisableTimeZoneTrackingOption));

There's enough precedence in the codebase for "optional" loadTimeData bools. 
Since this is a flag, it's not considered normal operation... I know it's an
insignificant number of bytes, but would prefer to omit it.

[michaelpg, 2016-10-06 23:57:05]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-10-06 23:57:06]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2393703005/#ps80001
(title: "Move SetInteger to WebUI CL")

[commit-bot: I haz the power, 2016-10-06 23:57:32]

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, 2016-10-07 00:49:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-07 00:52:21]

Description was changed from

==========
MD Settings: Date and Time page, part 1/3

Modernizes the UI a bit and adds a DateTimeHandler for policy stuff
(notifications TBD) and to provide the time zone list (TBD).

BUG=546835,460542
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Date and Time page, part 1/3

Modernizes the UI a bit and adds a DateTimeHandler for policy stuff
(notifications TBD) and to provide the time zone list (TBD).

BUG=546835,460542
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/614d31daee2f61b0180df403a8ad43f20b9f6dd7
Cr-Commit-Position: refs/heads/master@{#423768}
==========

[commit-bot: I haz the power, 2016-10-07 00:52:22]

Patchset 5 (id:??) landed as
https://crrev.com/614d31daee2f61b0180df403a8ad43f20b9f6dd7
Cr-Commit-Position: refs/heads/master@{#423768}