ChromeOS: Implement periodic timezone refresh on geolocation data.

ChromeOS: Implement periodic timezone refresh on geolocation data.

This CL implements automatic timezone refresh on location update.

BUG=416494
TEST=manually tested

Committed: https://crrev.com/48255f3da019c9728fc22b7007e5ca84b90cd78f
Cr-Commit-Position: refs/heads/master@{#313943}

[Alexander Alekseev, 2015-01-05 00:58:15]

alemate@chromium.org changed reviewers:
+ bauerb@chromium.org, dpolukhin@chromium.org, isherman@chromium.org,
sky@chromium.org, stevenjb@chromium.org

[Alexander Alekseev, 2015-01-05 00:58:16]

sky@ :
chrome/browser/browser_process_platform_part_chromeos.*

isherman@ :
tools/metrics/*

stevenjb@ :
chrome/browser/ui/webui/options/browser_options_handler.*
chrome/browser/resources/options/browser_options*
chromeos/timezone/*

bauerb@ :
chrome/browser/supervised_user/supervised_user_service.cc

dpolukhin@ : All

[Bernhard Bauer, 2015-01-05 10:22:33]

I'm looking forward to this feature! :)

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/browser_p...
File chrome/browser/browser_process_platform_part_chromeos.cc (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/browser_p...
chrome/browser/browser_process_platform_part_chromeos.cc:31: class
DelayNetworkCallWithDelay {
Could you do this with a static method DelayNetworkCallWithDelay(base::TimeDelta
delta, const base::Closure& callback) and then base::Bind the first argument?

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:630: if (value == true) {
== true isn't necessary.

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.h (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.h:145: BooleanPrefMember
resolve_timezone_by_geolocation_;
If you only need to observe preference changes, a PrefMember isn't necessary.
Instead you can use a PrefChangeRegistrar (and probably for the other
PrefMembers here as well).

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/timezone_util.cc (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/timezone_util.cc:178: if
(!primary_user->is_profile_created()) {
No braces for one-line bodies.

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.html:308: <input
id="resolve-timezone-by-geolocation"
pref="settings.resolve_timezone_by_geolocation"
Break before pref to fit into 80 columns.

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:177: io_url_filter_,
base::Unretained(context), cx));
Is base::Unretained necessary for raw pointers after the first one now? And is
there a reason why this change is made in this CL?

https://codereview.chromium.org/834073002/diff/60001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/834073002/diff/60001/chromeos/chromeos.gyp#ne...
chromeos/chromeos.gyp:423: 'timezone/timezone_resolver.h',
Alphabetical ordering: "resolver" after "request".

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:107: // Total number of requst attempts.
Nit: "request"

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:173: if (!position.Valid()) {
No braces for one-line bodies.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:213: scoped_ptr<TZRequest>
self(resolver_->ReleaseRequest());
All this method does is call methods on the resolver. Could you move this
directly to the resolver?

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:241: DCHECK(power_monitor);
This DCHECK isn't really necessary; if |power_monitor| is in fact null, you will
crash on the next line anyway.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:255: if (request_.get() ||
refresh_timer_.IsRunning())
scoped_ptr has a bool overload, so you don't need the .get(). (Same in other
places below).

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:269: // This is initial request, which
should be served immediate.
Nit: "immediately"

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:279: power)
Wait... You divide by |power| here. That means you will return the maximum
interval when

  initial_interval * 2^(requests_count_ * power) > maximum_interval.

But below you calculate the interval as 

  initial_interval * ((2 << power) << requests_count_) = initial_interval *
2^(requests_count_ + power + 1).

Do you need to change the code here to log2(...) - power - 1?

A unit test might be good here :)

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:284: return std::min(interval,
maximum_interval);
Assuming you clamp at |maximum_interval| already above, clamping here isn't
necessary anymore; you could replace it with a DCHECK_LE(interval,
maximum_interval).

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:361: if (implementation_) {
This seems like an excellent use case for a scoped_ptr :)

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
File chromeos/timezone/timezone_resolver.h (right):

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.h:27: class TimeZoneResolverImpl;
This can be moved to the private section.

[stevenjb, 2015-01-05 17:34:22]

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:392: cr.isChromeOS) {
The entire 'Date and time section' is CrOS only, so enclose the above if and
this one in an if (cr.isChromeOS) {}

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:682:
!chromeos::system::HasSystemTimezonePolicy());
nit: use a temporary here to improve readability.

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.h (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.h:232: // wallpaper if
kSystemTimezonePolicy is not set.
s/wallpaper/time zone/

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:37: // Timezone refresh happens at least
each this interval.
'at least once each interval'

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:44: // = initial_interval * (16 <<
requests_count_)] in seconds.
Is this level of complexity really necessary? Could the code be simplified by
just using a multiplier, e.g. interval = initial_interval * kRefreshMultipler *
(2 ^ requests_count) ?

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:79: TimeZoneProvider* timezone_provider()
{ return &timezone_provider_; }
Can either of these be const?

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:192: base::Closure unused =
on_request_finished.Release();
nit: Add a comment for those who aren't familiar with ScopedClosureRunner (the
Release() prevents it from running, right?)

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:247: DCHECK(power_monitor);
power_monitor could be NULL on shutdown, so just return if NULL here.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:322: return request_.release();
This should return a scoped_ptr to ensure that ownership is not lost.

[Ilya Sherman, 2015-01-06 00:02:54]

actions.xml and histograms.xml lgtm

[Alexander Alekseev, 2015-01-15 18:59:03]

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/browser_p...
File chrome/browser/browser_process_platform_part_chromeos.cc (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/browser_p...
chrome/browser/browser_process_platform_part_chromeos.cc:31: class
DelayNetworkCallWithDelay {
On 2015/01/05 10:22:32, Bernhard Bauer wrote:
> Could you do this with a static method
DelayNetworkCallWithDelay(base::TimeDelta
> delta, const base::Closure& callback) and then base::Bind the first argument?

Done.

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:630: if (value == true) {
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> == true isn't necessary.

Done.

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.h (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.h:145: BooleanPrefMember
resolve_timezone_by_geolocation_;
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> If you only need to observe preference changes, a PrefMember isn't necessary.
> Instead you can use a PrefChangeRegistrar (and probably for the other
> PrefMembers here as well).

Done.

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/timezone_util.cc (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/timezone_util.cc:178: if
(!primary_user->is_profile_created()) {
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> No braces for one-line bodies.

Done.

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.html:308: <input
id="resolve-timezone-by-geolocation"
pref="settings.resolve_timezone_by_geolocation"
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> Break before pref to fit into 80 columns.

Done.

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:392: cr.isChromeOS) {
On 2015/01/05 17:34:21, stevenjb wrote:
> The entire 'Date and time section' is CrOS only, so enclose the above if and
> this one in an if (cr.isChromeOS) {}

Done.

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:177: io_url_filter_,
base::Unretained(context), cx));
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> Is base::Unretained necessary for raw pointers after the first one now? And is
> there a reason why this change is made in this CL?

This is actually a refcounted pointer, but here it was always bound as raw
pointer.

My previous patch set introduced some change to headers, so I had to add proper
include and force the pointer to be bound as Unretained to keep compatibility.

I see that now it is not required (I've changed something and error doesn't
happen any more). So I've removed this change, though it is a bug. Probably we
should fix it with a separate CL.

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:682:
!chromeos::system::HasSystemTimezonePolicy());
On 2015/01/05 17:34:21, stevenjb wrote:
> nit: use a temporary here to improve readability.

Done.

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.h (right):

https://codereview.chromium.org/834073002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.h:232: // wallpaper if
kSystemTimezonePolicy is not set.
On 2015/01/05 17:34:21, stevenjb wrote:
> s/wallpaper/time zone/

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/834073002/diff/60001/chromeos/chromeos.gyp#ne...
chromeos/chromeos.gyp:423: 'timezone/timezone_resolver.h',
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> Alphabetical ordering: "resolver" after "request".

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:37: // Timezone refresh happens at least
each this interval.
On 2015/01/05 17:34:21, stevenjb wrote:
> 'at least once each interval'

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:44: // = initial_interval * (16 <<
requests_count_)] in seconds.
On 2015/01/05 17:34:21, stevenjb wrote:
> Is this level of complexity really necessary? Could the code be simplified by
> just using a multiplier, e.g. interval = initial_interval * kRefreshMultipler
*
> (2 ^ requests_count) ?

(2 ^ requests_count) was my initial implementation, but this leads to too many
requests. I've simplified algorithm here a bit, so now it starts requests with
intervals:

3 seconds (initial)
48        (second request)
384       (third request)
3072      (4)
21600     (5+) - this is maximum value (6 hours), formula would give  24576
seconds here.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:79: TimeZoneProvider* timezone_provider()
{ return &timezone_provider_; }
On 2015/01/05 17:34:21, stevenjb wrote:
> Can either of these be const?

It would require |timezone_provider_| to be a scoped_ptr, not an object itself.
Do you think it would be better?

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:107: // Total number of requst attempts.
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> Nit: "request"

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:173: if (!position.Valid()) {
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> No braces for one-line bodies.

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:192: base::Closure unused =
on_request_finished.Release();
On 2015/01/05 17:34:21, stevenjb wrote:
> nit: Add a comment for those who aren't familiar with ScopedClosureRunner (the
> Release() prevents it from running, right?)

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:213: scoped_ptr<TZRequest>
self(resolver_->ReleaseRequest());
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> All this method does is call methods on the resolver. Could you move this
> directly to the resolver?

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:241: DCHECK(power_monitor);
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> This DCHECK isn't really necessary; if |power_monitor| is in fact null, you
will
> crash on the next line anyway.

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:247: DCHECK(power_monitor);
On 2015/01/05 17:34:22, stevenjb wrote:
> power_monitor could be NULL on shutdown, so just return if NULL here.

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:255: if (request_.get() ||
refresh_timer_.IsRunning())
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> scoped_ptr has a bool overload, so you don't need the .get(). (Same in other
> places below).

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:269: // This is initial request, which
should be served immediate.
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> Nit: "immediately"

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:279: power)
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> Wait... You divide by |power| here. That means you will return the maximum
> interval when
> 
>   initial_interval * 2^(requests_count_ * power) > maximum_interval.
> 
> But below you calculate the interval as 
> 
>   initial_interval * ((2 << power) << requests_count_) = initial_interval *
> 2^(requests_count_ + power + 1).
> 
> Do you need to change the code here to log2(...) - power - 1?
> 
> A unit test might be good here :)

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:284: return std::min(interval,
maximum_interval);
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> Assuming you clamp at |maximum_interval| already above, clamping here isn't
> necessary anymore; you could replace it with a DCHECK_LE(interval,
> maximum_interval).

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:322: return request_.release();
On 2015/01/05 17:34:22, stevenjb wrote:
> This should return a scoped_ptr to ensure that ownership is not lost.

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:361: if (implementation_) {
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> This seems like an excellent use case for a scoped_ptr :)

Done.

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
File chromeos/timezone/timezone_resolver.h (right):

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.h:27: class TimeZoneResolverImpl;
On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> This can be moved to the private section.

It would lead to compilation error:

timezone_resolver.cc:121:40: error: 'TimeZoneResolverImpl' is a private member
of 'chromeos::TimeZoneResolver'
  explicit TZRequest(TimeZoneResolver::TimeZoneResolverImpl* resolver)
                                       ^

[Bernhard Bauer, 2015-01-16 09:25:05]

https://codereview.chromium.org/834073002/diff/60001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/834073002/diff/60001/chromeos/chromeos.gyp#ne...
chromeos/chromeos.gyp:423: 'timezone/timezone_resolver.h',
On 2015/01/15 18:59:03, Alexander Alekseev wrote:
> On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> > Alphabetical ordering: "resolver" after "request".
> 
> Done.

The order of header vs. implementation file is fixed now, but "resolver" still
comes after "request" :)

https://codereview.chromium.org/834073002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.h (right):

https://codereview.chromium.org/834073002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.h:146: PrefChangeRegistrar
pref_change_registarar_;
|pref_change_registrar_|.

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:228: void TZRequest::RequestIsFinished()
{
You only call this via a closure now, right? In that case, you could directly
bind TimeZoneResolverImpl::RequestIsFinished.

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:334: scoped_ptr<TZRequest>
previous_request(request_.release());
Is it important now that the request is only destroyed at the end of the method?
If not, you could just reset() request.

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
File chromeos/timezone/timezone_resolver.h (right):

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.h:64: unsigned int CHROMEOS_EXPORT
Per the style guide, we usually use just plain int for numbers, or a fixed-size
type if that is important (like uint64_t).

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.h:65:
MaxRequestsCountForIntervalForTesting(const double interval_seconds);
Could you make these methods static class methods?

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
File chromeos/timezone/timezone_unittest.cc (right):

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
chromeos/timezone/timezone_unittest.cc:291: struct TimeZoneResolverTest : public
testing::Test {
You don't need a test fixture for very simple tests. Just use TEST() instead of
TEST_F() below.

[Bernhard Bauer, 2015-01-16 10:56:12]

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:44: // in seconds.
BTW, I just found out there's also net::BackoffEntry to implement exponential
backoff.

[Alexander Alekseev, 2015-01-16 20:42:45]

https://codereview.chromium.org/834073002/diff/60001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/834073002/diff/60001/chromeos/chromeos.gyp#ne...
chromeos/chromeos.gyp:423: 'timezone/timezone_resolver.h',
On 2015/01/16 09:25:05, Bernhard Bauer wrote:
> On 2015/01/15 18:59:03, Alexander Alekseev wrote:
> > On 2015/01/05 10:22:33, Bernhard Bauer wrote:
> > > Alphabetical ordering: "resolver" after "request".
> > 
> > Done.
> 
> The order of header vs. implementation file is fixed now, but "resolver" still
> comes after "request" :)

Done.

https://codereview.chromium.org/834073002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.h (right):

https://codereview.chromium.org/834073002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.h:146: PrefChangeRegistrar
pref_change_registarar_;
On 2015/01/16 09:25:05, Bernhard Bauer wrote:
> |pref_change_registrar_|.

Done.

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:44: // in seconds.
On 2015/01/16 10:56:11, Bernhard Bauer wrote:
> BTW, I just found out there's also net::BackoffEntry to implement exponential
> backoff.

It implements back-off strategy to optimize for network errors.
We do not have erors here, so it seems to be too complicated.

I would better stay with this implementation.

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:228: void TZRequest::RequestIsFinished()
{
On 2015/01/16 09:25:05, Bernhard Bauer wrote:
> You only call this via a closure now, right? In that case, you could directly
> bind TimeZoneResolverImpl::RequestIsFinished.

Done.

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:334: scoped_ptr<TZRequest>
previous_request(request_.release());
On 2015/01/16 09:25:05, Bernhard Bauer wrote:
> Is it important now that the request is only destroyed at the end of the
method?
> If not, you could just reset() request.

Done.

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
File chromeos/timezone/timezone_resolver.h (right):

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.h:64: unsigned int CHROMEOS_EXPORT
On 2015/01/16 09:25:05, Bernhard Bauer wrote:
> Per the style guide, we usually use just plain int for numbers, or a
fixed-size
> type if that is important (like uint64_t).

Done.

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.h:65:
MaxRequestsCountForIntervalForTesting(const double interval_seconds);
On 2015/01/16 09:25:05, Bernhard Bauer wrote:
> Could you make these methods static class methods?

Done.

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
File chromeos/timezone/timezone_unittest.cc (right):

https://codereview.chromium.org/834073002/diff/80001/chromeos/timezone/timezo...
chromeos/timezone/timezone_unittest.cc:291: struct TimeZoneResolverTest : public
testing::Test {
On 2015/01/16 09:25:05, Bernhard Bauer wrote:
> You don't need a test fixture for very simple tests. Just use TEST() instead
of
> TEST_F() below.

Done.

[Bernhard Bauer, 2015-01-19 09:55:54]

LGTM, just two suggestions:

https://codereview.chromium.org/834073002/diff/100001/chromeos/timezone/timez...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/834073002/diff/100001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:68: public
base::SupportsWeakPtr<TimeZoneResolver::TimeZoneResolverImpl> {
Can you use a base::WeakPtrFactory<> as a member instead? It's considered
somewhat bad style to hand out weak pointers to other classes (as opposed to
just using them internally, e.g. for callbacks). Same for TZRequest.

https://codereview.chromium.org/834073002/diff/100001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:184: resolver_->AsWeakPtr()));
You can use |resolver_| unretained here. If it's not null now, it won't be null
by the time the closure runs either.

[Alexander Alekseev, 2015-01-20 14:41:45]

sky@, stejenjb@, dpolukhin@ please review:


sky@ :
chrome/browser/browser_process_platform_part_chromeos.*

stevenjb@ :
chrome/browser/ui/webui/options/browser_options_handler.*
chrome/browser/resources/options/browser_options*
chromeos/timezone/*

dpolukhin@ : All

https://codereview.chromium.org/834073002/diff/100001/chromeos/timezone/timez...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/834073002/diff/100001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:68: public
base::SupportsWeakPtr<TimeZoneResolver::TimeZoneResolverImpl> {
On 2015/01/19 09:55:54, Bernhard Bauer wrote:
> Can you use a base::WeakPtrFactory<> as a member instead? It's considered
> somewhat bad style to hand out weak pointers to other classes (as opposed to
> just using them internally, e.g. for callbacks). Same for TZRequest.

Done.

https://codereview.chromium.org/834073002/diff/100001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:184: resolver_->AsWeakPtr()));
On 2015/01/19 09:55:54, Bernhard Bauer wrote:
> You can use |resolver_| unretained here. If it's not null now, it won't be
null
> by the time the closure runs either.

Done.

[Dmitry Polukhin, 2015-01-20 15:32:29]

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1213:
LoginDisplayHostImpl* display_host_impl =
What happens with lifetime of the object? Even if it is OK, I still have
concerns about creating alias for memory allocated by new. I think it is better
to change display_host type to be LoginDisplayHostImpl.

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right):

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:382:
StartUserTimeZoneRefresh(profile);
I think in Guest mode we don't need to refresh timezone. As for Kiosk mode I
think we need to check with PMs.

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.h (right):

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.h:203: void
StartUserTimeZoneRefresh(Profile* profile);
Please rename to something more consistent. IMHO, it is confusing to call
function that "Start" that may stop something.

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:35: const int64 kInitialRefreshIntervalMS
= 3000;
Please add jitter and also consider not sending request just after restore after
browser crash.

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:175:
resolver_->geolocation_provider()->RequestGeolocation(
Please make sure that geolocation_provider does have proper backoff logic in
case of service errors.

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
File chromeos/timezone/timezone_resolver.h (right):

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.h:24: // This class implements periodic
timezone synchronization.
Please describe logic how periodic refresh happens. I think here is better place
when in .cc file.

[sky, 2015-01-20 16:47:30]

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.cc (right):

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:30: // We need
DelayNetworkCall() with different order of arguments to be able to
Is there a reason you can't change the signature in the chromeos side?

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.h (right):

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.h:12: #include
"chromeos/timezone/timezone_resolver.h"
forward declare.

[stevenjb, 2015-01-20 18:30:42]

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/834073002/diff/60001/chromeos/timezone/timezo...
chromeos/timezone/timezone_resolver.cc:79: TimeZoneProvider* timezone_provider()
{ return &timezone_provider_; }
On 2015/01/15 18:59:03, Alexander Alekseev wrote:
> On 2015/01/05 17:34:21, stevenjb wrote:
> > Can either of these be const?
> 
> It would require |timezone_provider_| to be a scoped_ptr, not an object
itself.
> Do you think it would be better?

Why would it need to be scoped to be const? If all accessors to
timezone_provider() only access const members, then this can be:
const TimeZoneProvider* timezone_provider() const { return &timezone_provider_;
}

If some of the accessors of this need access to non const methods of
TimeZoneProvider than this can't be const, in which case we might want to note
that in a comment.

Same with geolocation_provider().

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:38: const int64 kMaximalRefreshIntervalMS
= 6 * 3600 * 1000;  // 6 hours
Maximal -> Maximum

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:49:
base::TimeDelta::FromMilliseconds(kInitialRefreshIntervalMS);
Am I missing something or is this just being used to convert MS -> seconds, in
which case it would be simpler just to do:
const float initial_interval_seconds = kInitialRefreshIntervalMS * 1000.0f;

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:56:
base::TimeDelta::FromMilliseconds(kInitialRefreshIntervalMS);
Same comment here. Do we ever use kInitialRefreshIntervalMS as anything other
than a float? If not, we should just define the constant as a float in seconds.

[Alexander Alekseev, 2015-01-22 18:55:23]

I've swapped arguments of chromeos::DelayNetworkCall(), added jitter to delay
calculation and longer delay on browser restart.

PTAL.

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.cc (right):

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:30: // We need
DelayNetworkCall() with different order of arguments to be able to
On 2015/01/20 16:47:30, sky wrote:
> Is there a reason you can't change the signature in the chromeos side?

Probably no.
Done.

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.h (right):

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.h:12: #include
"chromeos/timezone/timezone_resolver.h"
On 2015/01/20 16:47:30, sky wrote:
> forward declare.

Done.

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1213:
LoginDisplayHostImpl* display_host_impl =
On 2015/01/20 15:32:28, Dmitry Polukhin wrote:
> What happens with lifetime of the object? Even if it is OK, I still have
> concerns about creating alias for memory allocated by new. I think it is
better
> to change display_host type to be LoginDisplayHostImpl.

Done.

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right):

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:382:
StartUserTimeZoneRefresh(profile);
On 2015/01/20 15:32:28, Dmitry Polukhin wrote:
> I think in Guest mode we don't need to refresh timezone. As for Kiosk mode I
> think we need to check with PMs.

Done.

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.h (right):

https://codereview.chromium.org/834073002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.h:203: void
StartUserTimeZoneRefresh(Profile* profile);
On 2015/01/20 15:32:28, Dmitry Polukhin wrote:
> Please rename to something more consistent. IMHO, it is confusing to call
> function that "Start" that may stop something.

Done.

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:35: const int64 kInitialRefreshIntervalMS
= 3000;
On 2015/01/20 15:32:28, Dmitry Polukhin wrote:
> Please add jitter and also consider not sending request just after restore
after
> browser crash.

Done.

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:38: const int64 kMaximalRefreshIntervalMS
= 6 * 3600 * 1000;  // 6 hours
On 2015/01/20 18:30:42, stevenjb wrote:
> Maximal -> Maximum

Done.

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:49:
base::TimeDelta::FromMilliseconds(kInitialRefreshIntervalMS);
On 2015/01/20 18:30:42, stevenjb wrote:
> Am I missing something or is this just being used to convert MS -> seconds, in
> which case it would be simpler just to do:
> const float initial_interval_seconds = kInitialRefreshIntervalMS * 1000.0f;

Done.

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:56:
base::TimeDelta::FromMilliseconds(kInitialRefreshIntervalMS);
On 2015/01/20 18:30:42, stevenjb wrote:
> Same comment here. Do we ever use kInitialRefreshIntervalMS as anything other
> than a float? If not, we should just define the constant as a float in
seconds.

Done.

https://codereview.chromium.org/834073002/diff/140001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:175:
resolver_->geolocation_provider()->RequestGeolocation(
On 2015/01/20 15:32:28, Dmitry Polukhin wrote:
> Please make sure that geolocation_provider does have proper backoff logic in
> case of service errors.

It has it. It makes (probably) several requests until given timeut comes to an
end.

[stevenjb, 2015-01-22 19:04:31]

https://codereview.chromium.org/834073002/diff/160001/chromeos/timezone/timez...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/834073002/diff/160001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:38: const double
kInitialRefreshIntervalSec = 3;
nit: 3.0

https://codereview.chromium.org/834073002/diff/160001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:41: const double
kMaximumRefreshIntervalSec = 6 * 3600;  // 6 hours
nit: 6.0

https://codereview.chromium.org/834073002/diff/160001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:60: const double
kRefreshTimeZoneMinimumDelayOnRestartSec = 600;  // 10 minutes
10 * 60.0

https://codereview.chromium.org/834073002/diff/160001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:67:
kRefreshIntervalRequestsCountMultiplier;
I still don't understand why a TimeDelta is being used here, this is just
converting seconds -> microseconds -> seconds. Why not just use
kInitialRefreshIntervalSec directly?

https://codereview.chromium.org/834073002/diff/160001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:75:
kRefreshIntervalRequestsCountMultiplier)));
same here.

[sky, 2015-01-22 20:43:20]

chrome/browser/browser_process_platform_part_chromeos.* LGTM

[Alexander Alekseev, 2015-01-27 19:02:00]

https://codereview.chromium.org/834073002/diff/160001/chromeos/timezone/timez...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/834073002/diff/160001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:38: const double
kInitialRefreshIntervalSec = 3;
On 2015/01/22 19:04:30, stevenjb wrote:
> nit: 3.0

Done.

https://codereview.chromium.org/834073002/diff/160001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:41: const double
kMaximumRefreshIntervalSec = 6 * 3600;  // 6 hours
On 2015/01/22 19:04:30, stevenjb wrote:
> nit: 6.0

Done.

https://codereview.chromium.org/834073002/diff/160001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:60: const double
kRefreshTimeZoneMinimumDelayOnRestartSec = 600;  // 10 minutes
On 2015/01/22 19:04:30, stevenjb wrote:
> 10 * 60.0

Done.

https://codereview.chromium.org/834073002/diff/160001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:67:
kRefreshIntervalRequestsCountMultiplier;
On 2015/01/22 19:04:30, stevenjb wrote:
> I still don't understand why a TimeDelta is being used here, this is just
> converting seconds -> microseconds -> seconds. Why not just use
> kInitialRefreshIntervalSec directly?
> 

Done.

https://codereview.chromium.org/834073002/diff/160001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.cc:75:
kRefreshIntervalRequestsCountMultiplier)));
On 2015/01/22 19:04:30, stevenjb wrote:
> same here.

Done.

[Dmitry Polukhin, 2015-01-28 12:10:10]

https://codereview.chromium.org/834073002/diff/180001/chromeos/timezone/timez...
File chromeos/timezone/timezone_resolver.h (right):

https://codereview.chromium.org/834073002/diff/180001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.h:65: net::URLRequestContextGetter* const
context_;
It looks like it should be scoped_refptr<net::URLRequestContextGetter>, here and
in other places where you store it (i.e. not only pass it).

https://codereview.chromium.org/834073002/diff/180001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.h:70: PrefService* local_state_;
Suggestion, I don't like having all these data members in 2 classes (interface +
impl). Can we always create impl and pass all params but initialize impl it only
if needed?

[Alexander Alekseev, 2015-01-28 17:57:55]

Dmitry, PTAL.

https://codereview.chromium.org/834073002/diff/180001/chromeos/timezone/timez...
File chromeos/timezone/timezone_resolver.h (right):

https://codereview.chromium.org/834073002/diff/180001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.h:65: net::URLRequestContextGetter* const
context_;
On 2015/01/28 12:10:10, Dmitry Polukhin wrote:
> It looks like it should be scoped_refptr<net::URLRequestContextGetter>, here
and
> in other places where you store it (i.e. not only pass it).

Done.

https://codereview.chromium.org/834073002/diff/180001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.h:70: PrefService* local_state_;
On 2015/01/28 12:10:10, Dmitry Polukhin wrote:
> Suggestion, I don't like having all these data members in 2 classes (interface
+
> impl). Can we always create impl and pass all params but initialize impl it
only
> if needed?

The idea was to use RAII, so that implementation would exist only of periodic
resolve is started.

I've removed data copying, and now implementation would get all the data from
TimeZoneResolver object.

[Dmitry Polukhin, 2015-01-29 08:57:41]

lgtm

[Alexander Alekseev, 2015-01-29 12:41:09]

stevenjb@, please review.

[stevenjb, 2015-01-29 18:29:21]

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

https://codereview.chromium.org/834073002/diff/220001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5968: If enabled, in chrome://settings new
option becomes available, which allows to automatically update timezone by
current geolocation.
If enabled, in chrome://settings a new option becomes available which allows
automatically updating the timezone by the current geolocation.

https://codereview.chromium.org/834073002/diff/220001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.h (right):

https://codereview.chromium.org/834073002/diff/220001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.h:90:
chromeos::TimeZoneResolver* timezone_resolver();
This is not a trival accessor, it should be named GetTimezoneResolver().

https://codereview.chromium.org/834073002/diff/220001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/834073002/diff/220001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1623: /**
Brief description of function. Be sure to mention where this is called from in
Chrome since otherwise the |managed| parameter seems unnecessary.

https://codereview.chromium.org/834073002/diff/220001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1624: * @param {boolean}
managed
Describe parameter

https://codereview.chromium.org/834073002/diff/220001/chromeos/timezone/timez...
File chromeos/timezone/timezone_resolver.h (right):

https://codereview.chromium.org/834073002/diff/220001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.h:29: ApplyTimeZoneCallback;
using ApplyTimeZoneCallback = base::Callback<void(const TimeZoneResponseData*)>;

https://codereview.chromium.org/834073002/diff/220001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.h:33: typedef base::Callback<void(const
base::Closure&)> DelayNetworkCallClosure;
ditto

https://codereview.chromium.org/834073002/diff/220001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.h:37: // It is used to limit request rate on
browser restart.
Combine lines

[Alexander Alekseev, 2015-01-29 19:51:08]

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

https://codereview.chromium.org/834073002/diff/220001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5968: If enabled, in chrome://settings new
option becomes available, which allows to automatically update timezone by
current geolocation.
On 2015/01/29 18:29:20, stevenjb wrote:
> If enabled, in chrome://settings a new option becomes available which allows
> automatically updating the timezone by the current geolocation.

Done.

https://codereview.chromium.org/834073002/diff/220001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.h (right):

https://codereview.chromium.org/834073002/diff/220001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.h:90:
chromeos::TimeZoneResolver* timezone_resolver();
On 2015/01/29 18:29:20, stevenjb wrote:
> This is not a trival accessor, it should be named GetTimezoneResolver().

Done.

https://codereview.chromium.org/834073002/diff/220001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/834073002/diff/220001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1623: /**
On 2015/01/29 18:29:20, stevenjb wrote:
> Brief description of function. Be sure to mention where this is called from in
> Chrome since otherwise the |managed| parameter seems unnecessary.

Done.

https://codereview.chromium.org/834073002/diff/220001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1624: * @param {boolean}
managed
On 2015/01/29 18:29:20, stevenjb wrote:
> Describe parameter

Done.

https://codereview.chromium.org/834073002/diff/220001/chromeos/timezone/timez...
File chromeos/timezone/timezone_resolver.h (right):

https://codereview.chromium.org/834073002/diff/220001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.h:29: ApplyTimeZoneCallback;
On 2015/01/29 18:29:20, stevenjb wrote:
> using ApplyTimeZoneCallback = base::Callback<void(const
TimeZoneResponseData*)>;

Done.

https://codereview.chromium.org/834073002/diff/220001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.h:33: typedef base::Callback<void(const
base::Closure&)> DelayNetworkCallClosure;
On 2015/01/29 18:29:20, stevenjb wrote:
> ditto

Done.

https://codereview.chromium.org/834073002/diff/220001/chromeos/timezone/timez...
chromeos/timezone/timezone_resolver.h:37: // It is used to limit request rate on
browser restart.
On 2015/01/29 18:29:20, stevenjb wrote:
> Combine lines

Done.

[stevenjb, 2015-01-29 20:23:27]

lgtm

https://codereview.chromium.org/834073002/diff/240001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/834073002/diff/240001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1627: * enterprise policy.
False otherwize.
nit: indent 'enterprise' by 4 spaces, i.e.
    *     enterprise ...

[Alexander Alekseev, 2015-01-30 13:53:45]

https://codereview.chromium.org/834073002/diff/240001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/834073002/diff/240001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1627: * enterprise policy.
False otherwize.
On 2015/01/29 20:23:27, stevenjb wrote:
> nit: indent 'enterprise' by 4 spaces, i.e.
>     *     enterprise ...

Done.

[Alexander Alekseev, 2015-01-30 16:27:20]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2015-01-30 16:27:38]

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

[commit-bot: I haz the power, 2015-01-30 16:37:20]

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

[commit-bot: I haz the power, 2015-01-30 16:37:22]

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

[Alexander Alekseev, 2015-01-30 16:56:11]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2015-01-30 16:56:55]

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

[commit-bot: I haz the power, 2015-01-30 18:12:16]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-01-30 18:13:34]

Patchset 16 (id:??) landed as
https://crrev.com/48255f3da019c9728fc22b7007e5ca84b90cd78f
Cr-Commit-Position: refs/heads/master@{#313943}