ChromeOS: Update browser option UI for automatic timezone detection.

ChromeOS: Update browser option UI for automatic timezone detection.

Update chrome://settings for "automatic timezone detection" option after UI
review.

BUG=461953
TEST=manual

Committed: https://crrev.com/787f34332bc57394f5fde94cabf1676ad801fdea
Cr-Commit-Position: refs/heads/master@{#318753}

[Alexander Alekseev, 2015-02-19 22:03:26]

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

[Alexander Alekseev, 2015-02-19 22:03:27]

1) This Cl moves checkbox "Set time zone automatically using your location"
closer to "Select your timezone" dropbox.

2) Removes "Date and time are set automatically." placeholder (refer to
jennschen@ and barbm@ for details).

3) Fixes setSystemTimezoneManaged() JS method : it should be callable from C++
now.

4) Properly disables "Select timezone" dropbox if timezone is resolved
automatically. 


Please review.

[stevenjb, 2015-02-19 22:24:20]

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

[stevenjb, 2015-02-19 22:24:21]

+michaelpg@ Who is more familiar with this bit of settings UI.

[michaelpg, 2015-02-19 22:38:36]

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:408:
$('timezone-value-select').disabled = loadTimeData.getBoolean(
Do we re-enable this dropdown if I uncheck the resolve-timezone-by-geolocation
input?

[Alexander Alekseev, 2015-02-19 22:41:55]

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:408:
$('timezone-value-select').disabled = loadTimeData.getBoolean(
On 2015/02/19 22:38:36, michaelpg wrote:
> Do we re-enable this dropdown if I uncheck the resolve-timezone-by-geolocation
> input?

Yes. See setSystemTimezoneManaged() code.

[michaelpg, 2015-02-24 06:38:06]

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:408:
$('timezone-value-select').disabled = loadTimeData.getBoolean(
On 2015/02/19 22:41:55, Alexander Alekseev wrote:
> On 2015/02/19 22:38:36, michaelpg wrote:
> > Do we re-enable this dropdown if I uncheck the
resolve-timezone-by-geolocation
> > input?
> 
> Yes. See setSystemTimezoneManaged() code.

Sorry, I'm confused. Assuming no enterprise policy, if
resolve-timezone-by-geolocation is initially checked, how does
$('timezone-value-select') get enabled if I uncheck
resolve-timezone-by-geolocation?

[Alexander Alekseev, 2015-02-24 12:18:31]

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:408:
$('timezone-value-select').disabled = loadTimeData.getBoolean(
On 2015/02/24 06:38:06, michaelpg wrote:
> On 2015/02/19 22:41:55, Alexander Alekseev wrote:
> > On 2015/02/19 22:38:36, michaelpg wrote:
> > > Do we re-enable this dropdown if I uncheck the
> resolve-timezone-by-geolocation
> > > input?
> > 
> > Yes. See setSystemTimezoneManaged() code.
> 
> Sorry, I'm confused. Assuming no enterprise policy, if
> resolve-timezone-by-geolocation is initially checked, how does
> $('timezone-value-select') get enabled if I uncheck
> resolve-timezone-by-geolocation?

setSystemTimezoneManaged() creates onclick handler:

$('resolve-timezone-by-geolocation').onclick = function(event) {                

    $('timezone-value-select').disabled = event.currentTarget.checked;          

};

[michaelpg, 2015-02-24 13:04:40]

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:401: if
($('set-time-button'))
by the way, this check is redundant now

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:408:
$('timezone-value-select').disabled = loadTimeData.getBoolean(
On 2015/02/24 12:18:30, Alexander Alekseev wrote:
> On 2015/02/24 06:38:06, michaelpg wrote:
> > On 2015/02/19 22:41:55, Alexander Alekseev wrote:
> > > On 2015/02/19 22:38:36, michaelpg wrote:
> > > > Do we re-enable this dropdown if I uncheck the
> > resolve-timezone-by-geolocation
> > > > input?
> > > 
> > > Yes. See setSystemTimezoneManaged() code.
> > 
> > Sorry, I'm confused. Assuming no enterprise policy, if
> > resolve-timezone-by-geolocation is initially checked, how does
> > $('timezone-value-select') get enabled if I uncheck
> > resolve-timezone-by-geolocation?
> 
> setSystemTimezoneManaged() creates onclick handler:
> 
> $('resolve-timezone-by-geolocation').onclick = function(event) {              
 
> 
>     $('timezone-value-select').disabled = event.currentTarget.checked;        
 
> 
> }; 

Ah, I see. It's kind of unintuitive that we rely on that function for this
functionality. I'd suggest moving that click handler logic to a separate
function which we can use both here and in setSystemTimezoneManaged_.

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:2179:
'setSystemTimezoneManaged',
so this is an existing bug, since the function is already being called by the
C++?

[Nikita (slow), 2015-02-25 14:58:47]

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

[Nikita (slow), 2015-02-25 14:58:48]

Per offline discussion please use different BUG

[Alexander Alekseev, 2015-02-25 20:39:42]

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:401: if
($('set-time-button'))
On 2015/02/24 13:04:39, michaelpg wrote:
> by the way, this check is redundant now

Why? It seems I haven't changed its behavior.

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:408:
$('timezone-value-select').disabled = loadTimeData.getBoolean(
On 2015/02/24 13:04:39, michaelpg wrote:
> On 2015/02/24 12:18:30, Alexander Alekseev wrote:
> > On 2015/02/24 06:38:06, michaelpg wrote:
> > > On 2015/02/19 22:41:55, Alexander Alekseev wrote:
> > > > On 2015/02/19 22:38:36, michaelpg wrote:
> > > > > Do we re-enable this dropdown if I uncheck the
> > > resolve-timezone-by-geolocation
> > > > > input?
> > > > 
> > > > Yes. See setSystemTimezoneManaged() code.
> > > 
> > > Sorry, I'm confused. Assuming no enterprise policy, if
> > > resolve-timezone-by-geolocation is initially checked, how does
> > > $('timezone-value-select') get enabled if I uncheck
> > > resolve-timezone-by-geolocation?
> > 
> > setSystemTimezoneManaged() creates onclick handler:
> > 
> > $('resolve-timezone-by-geolocation').onclick = function(event) {            
 
>  
> > 
> >     $('timezone-value-select').disabled = event.currentTarget.checked;      
 
>  
> > 
> > }; 
> 
> Ah, I see. It's kind of unintuitive that we rely on that function for this
> functionality. I'd suggest moving that click handler logic to a separate
> function which we can use both here and in setSystemTimezoneManaged_.


This would lead to duplicate call to this new function on initialization,
because enableTimeZoneTrackingOption actually means that system time zone is not
managed:

--- chrome/browser/ui/webui/options/browser_options_handler.cc ---

 678   values->SetBoolean("enableTimeZoneTrackingOption",
 679                      !have_disable_time_zone_tracking_option_switch &&
 680                          !chromeos::system::HasSystemTimezonePolicy());

------------------------------------------------------------------

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:2179:
'setSystemTimezoneManaged',
On 2015/02/24 13:04:39, michaelpg wrote:
> so this is an existing bug, since the function is already being called by the
> C++?

Yes.

[Alexander Alekseev, 2015-02-26 17:46:03]

Ping?

On 2015/02/25 20:39:42, Alexander Alekseev wrote:
>
https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
> File chrome/browser/resources/options/browser_options.js (right):
> 
>
https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
> chrome/browser/resources/options/browser_options.js:401: if
> ($('set-time-button'))
> On 2015/02/24 13:04:39, michaelpg wrote:
> > by the way, this check is redundant now
> 
> Why? It seems I haven't changed its behavior.
> 
>
https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
> chrome/browser/resources/options/browser_options.js:408:
> $('timezone-value-select').disabled = loadTimeData.getBoolean(
> On 2015/02/24 13:04:39, michaelpg wrote:
> > On 2015/02/24 12:18:30, Alexander Alekseev wrote:
> > > On 2015/02/24 06:38:06, michaelpg wrote:
> > > > On 2015/02/19 22:41:55, Alexander Alekseev wrote:
> > > > > On 2015/02/19 22:38:36, michaelpg wrote:
> > > > > > Do we re-enable this dropdown if I uncheck the
> > > > resolve-timezone-by-geolocation
> > > > > > input?
> > > > > 
> > > > > Yes. See setSystemTimezoneManaged() code.
> > > > 
> > > > Sorry, I'm confused. Assuming no enterprise policy, if
> > > > resolve-timezone-by-geolocation is initially checked, how does
> > > > $('timezone-value-select') get enabled if I uncheck
> > > > resolve-timezone-by-geolocation?
> > > 
> > > setSystemTimezoneManaged() creates onclick handler:
> > > 
> > > $('resolve-timezone-by-geolocation').onclick = function(event) {          
 
>  
> >  
> > > 
> > >     $('timezone-value-select').disabled = event.currentTarget.checked;    
 
>  
> >  
> > > 
> > > }; 
> > 
> > Ah, I see. It's kind of unintuitive that we rely on that function for this
> > functionality. I'd suggest moving that click handler logic to a separate
> > function which we can use both here and in setSystemTimezoneManaged_.
> 
> 
> This would lead to duplicate call to this new function on initialization,
> because enableTimeZoneTrackingOption actually means that system time zone is
not
> managed:
> 
> --- chrome/browser/ui/webui/options/browser_options_handler.cc ---
> 
>  678   values->SetBoolean("enableTimeZoneTrackingOption",
>  679                      !have_disable_time_zone_tracking_option_switch &&
>  680                          !chromeos::system::HasSystemTimezonePolicy());
> 
> ------------------------------------------------------------------
> 
>
https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
> chrome/browser/resources/options/browser_options.js:2179:
> 'setSystemTimezoneManaged',
> On 2015/02/24 13:04:39, michaelpg wrote:
> > so this is an existing bug, since the function is already being called by
the
> > C++?
> 
> Yes.

[Nikita (slow), 2015-02-27 11:40:10]

lgtm

[michaelpg, 2015-02-27 18:12:29]

lgtm

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/944593002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:401: if
($('set-time-button'))
On 2015/02/25 20:39:41, Alexander Alekseev wrote:
> On 2015/02/24 13:04:39, michaelpg wrote:
> > by the way, this check is redundant now
> 
> Why? It seems I haven't changed its behavior.

True, I think the outer if statement had been added in the lady change. The
button appears if and only if we're on chrome os, so now that we're checking
cr.isChromeOs, the button will always exist.

[stevenjb, 2015-02-27 18:50:01]

owner lgtm

[Alexander Alekseev, 2015-03-02 13:52:48]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2015-03-02 13:53:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/944593002/1

[commit-bot: I haz the power, 2015-03-02 17:21:59]

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

[commit-bot: I haz the power, 2015-03-02 17:22:00]

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

[Alexander Alekseev, 2015-03-02 18:40:20]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2015-03-02 18:41:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/944593002/1

[Alexander Alekseev, 2015-03-02 19:59:31]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2015-03-02 19:59:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, nkostylev@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/944593002/#ps20001 (title:
"Fixed SharedOptionsTest.")

[commit-bot: I haz the power, 2015-03-02 19:59:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/944593002/20001

[commit-bot: I haz the power, 2015-03-02 21:07:05]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-02 21:07:49]

Patchset 2 (id:??) landed as
https://crrev.com/787f34332bc57394f5fde94cabf1676ad801fdea
Cr-Commit-Position: refs/heads/master@{#318753}