ChromeOS: Implement periodic timezone refresh on geolocation data.

chromeos/timezone/timezone_resolver.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/timezone/timezone_resolver.h"
+
+#include <math.h>
+
+#include <algorithm>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/callback_helpers.h"
+#include "base/logging.h"
+#include "base/power_monitor/power_monitor.h"
+#include "base/power_monitor/power_observer.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
+#include "chromeos/geolocation/geoposition.h"
+#include "chromeos/geolocation/simple_geolocation_provider.h"
+#include "chromeos/timezone/timezone_provider.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "url/gurl.h"
+
+namespace chromeos {
+
+namespace {
+
+class TZRequest;
+
+// Total timezone resolving process timeout.
+const unsigned int kRefreshTimeZoneTimeoutSeconds = 60;
+
+// Initial delay (for the first request).
+const int64 kInitialRefreshIntervalMS = 3000;
+
+// Timezone refresh happens at least each this interval.

[stevenjb, 2015/01/05 17:34:21]

'at least once each interval'

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

Done.

+const int64 kMaximalRefreshIntervalMS = 6 * 3600 * 1000;  // 6 hours
+
+// Delay between refresh attempts depends on current number of requests and
+// this constant. If [kRefreshIntervalBasePower=3], (base is always 2),
+// then [base_multiplier = (2 << kRefreshIntervalBasePower) = 16], and
+// this gives [interval = initial_interval * (base_multiplier << requests_count)
+// = initial_interval * (16 << requests_count_)] in seconds.

[stevenjb, 2015/01/05 17:34:21]

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) ?

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

(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.

+const unsigned long kRefreshIntervalBasePower = 3;
+
+}  // anonymous namespace
+
+// This class periodically refreshes location and timezone.
+// It should be destroyed to stop refresh.
+class TimeZoneResolver::TimeZoneResolverImpl
+    : public base::PowerObserver,
+      public base::SupportsWeakPtr<TimeZoneResolver::TimeZoneResolverImpl> {
+ public:
+  TimeZoneResolverImpl(
+      net::URLRequestContextGetter* url_context_getter,
+      const GURL& url,
+      const TimeZoneResolver::ApplyTimeZoneCallback& apply_timezone,
+      const TimeZoneResolver::DelayNetworkCallClosure& delay_network_call);
+
+  ~TimeZoneResolverImpl() override;
+
+  // This is called once after the object is created.
+  void Start();
+
+  // PowerObserver implementation.
+  void OnResume() override;
+
+  // (Re)Starts timer.
+  void ScheduleRequest();
+
+  // Creates new TZRequest.
+  void CreateNewRequest();
+
+  // Called by TZRequest.
+  SimpleGeolocationProvider* geolocation_provider() {
+    return &geolocation_provider_;
+  }
+  TimeZoneProvider* timezone_provider() { return &timezone_provider_; }

[stevenjb, 2015/01/05 17:34:21]

Can either of these be const?

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

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

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

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().

+
+  // Update requests count and last request time.
+  void RecordAttempt();
+
+  TZRequest* ReleaseRequest();
+
+  void ApplyTimeZone(const TimeZoneResponseData* timezone);
+
+  TimeZoneResolver::DelayNetworkCallClosure delay_network_call() const {
+    return delay_network_call_;
+  }
+
+ private:
+  // Returns delay to next timezone update request
+  base::TimeDelta CalculateNextInterval();
+
+  net::URLRequestContextGetter* url_context_getter_;
+  const GURL url_;
+
+  const TimeZoneResolver::ApplyTimeZoneCallback apply_timezone_;
+  const TimeZoneResolver::DelayNetworkCallClosure delay_network_call_;
+
+  SimpleGeolocationProvider geolocation_provider_;
+  TimeZoneProvider timezone_provider_;
+
+  base::OneShotTimer<TimeZoneResolver::TimeZoneResolverImpl> refresh_timer_;
+
+  // Total number of requst attempts.

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

Nit: "request"

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

Done.

+  unsigned int requests_count_;
+
+  // This is not NULL when update is in progress.
+  scoped_ptr<TZRequest> request_;
+
+  DISALLOW_COPY_AND_ASSIGN(TimeZoneResolverImpl);
+};
+
+namespace {
+
+// This class implements a single timezone refresh attempt.
+class TZRequest : public base::SupportsWeakPtr<TZRequest> {
+ public:
+  explicit TZRequest(TimeZoneResolver::TimeZoneResolverImpl* resolver)
+      : resolver_(resolver) {}
+
+  ~TZRequest();
+
+  // Starts request after specified delay.
+  void Start();
+
+  // Called from SimpleGeolocationProvider when location is resolved.
+  void OnLocationResolved(const Geoposition& position,
+                          bool server_error,
+                          const base::TimeDelta elapsed);
+
+  // TimeZoneRequest::TimeZoneResponseCallback implementation.
+  void OnTimezoneResolved(scoped_ptr<TimeZoneResponseData> timezone,
+                          bool server_error);
+
+ private:
+  // Schedules new request and destroys current object.
+  void RequestIsFinished();
+
+  // This is called by network detector when network is available.
+  void StartRequestOnNetworkAvailable();
+
+  TimeZoneResolver::TimeZoneResolverImpl* const resolver_;
+
+  DISALLOW_COPY_AND_ASSIGN(TZRequest);
+};
+
+TZRequest::~TZRequest() {
+}
+
+void TZRequest::StartRequestOnNetworkAvailable() {
+  resolver_->RecordAttempt();
+  resolver_->geolocation_provider()->RequestGeolocation(
+      base::TimeDelta::FromSeconds(kRefreshTimeZoneTimeoutSeconds),
+      base::Bind(&TZRequest::OnLocationResolved, AsWeakPtr()));
+}
+
+void TZRequest::Start() {
+  // call to chromeos::DelayNetworkCall
+  resolver_->delay_network_call().Run(
+      base::Bind(&TZRequest::StartRequestOnNetworkAvailable, AsWeakPtr()));
+}
+
+void TZRequest::OnLocationResolved(const Geoposition& position,
+                                   bool server_error,
+                                   const base::TimeDelta elapsed) {
+  base::ScopedClosureRunner on_request_finished(
+      base::Bind(&TZRequest::RequestIsFinished, base::Unretained(this)));
+
+  // Ignore invalid position.
+  if (!position.Valid()) {

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

No braces for one-line bodies.

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

Done.

+    return;
+  }
+
+  const base::TimeDelta timeout =
+      base::TimeDelta::FromSeconds(kRefreshTimeZoneTimeoutSeconds);
+
+  if (elapsed >= timeout) {
+    VLOG(1) << "Refresh TimeZone: got location after timeout ("
+            << elapsed.InSecondsF() << " seconds elapsed). Ignored.";
+    return;
+  }
+
+  resolver_->timezone_provider()->RequestTimezone(
+      position,
+      false,  // sensor
+      timeout - elapsed,
+      base::Bind(&TZRequest::OnTimezoneResolved, AsWeakPtr()));
+
+  base::Closure unused = on_request_finished.Release();

[stevenjb, 2015/01/05 17:34:21]

nit: Add a comment for those who aren't familiar with ScopedClosureRunner (the
Release() prevents it from running, right?)

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

Done.

+}
+
+void TZRequest::OnTimezoneResolved(scoped_ptr<TimeZoneResponseData> timezone,
+                                   bool server_error) {
+  base::ScopedClosureRunner on_request_finished(
+      base::Bind(&TZRequest::RequestIsFinished, base::Unretained(this)));
+
+  DCHECK(timezone.get());
+  VLOG(1) << "Refreshed local timezone={" << timezone->ToStringForDebug()
+          << "}.";
+
+  if (timezone->status != TimeZoneResponseData::OK) {
+    VLOG(1) << "Refresh TimeZone: failed to resolve timezone.";
+    return;
+  }
+
+  resolver_->ApplyTimeZone(timezone.get());
+}
+
+void TZRequest::RequestIsFinished() {
+  scoped_ptr<TZRequest> self(resolver_->ReleaseRequest());

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

All this method does is call methods on the resolver. Could you move this
directly to the resolver?

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

Done.

+  resolver_->ScheduleRequest();
+  // This object is destroyed here.
+}
+
+}  // anonymous namespace
+
+// ------------------------------------------------------------------------
+// TimeZoneResolver::TimeZoneResolverImpl implementation.
+
+TimeZoneResolver::TimeZoneResolverImpl::TimeZoneResolverImpl(
+    net::URLRequestContextGetter* url_context_getter,
+    const GURL& url,
+    const TimeZoneResolver::ApplyTimeZoneCallback& apply_timezone,
+    const TimeZoneResolver::DelayNetworkCallClosure& delay_network_call)
+    : url_context_getter_(url_context_getter),
+      url_(url),
+      apply_timezone_(apply_timezone),
+      delay_network_call_(delay_network_call),
+      geolocation_provider_(
+          url_context_getter,
+          SimpleGeolocationProvider::DefaultGeolocationProviderURL()),
+      timezone_provider_(url_context_getter, DefaultTimezoneProviderURL()),
+      requests_count_(0) {
+  DCHECK(!apply_timezone.is_null());
+  DCHECK(!delay_network_call.is_null());
+
+  base::PowerMonitor* power_monitor = base::PowerMonitor::Get();
+  DCHECK(power_monitor);

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

This DCHECK isn't really necessary; if |power_monitor| is in fact null, you will
crash on the next line anyway.

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

Done.

+  power_monitor->AddObserver(this);
+}
+
+TimeZoneResolver::TimeZoneResolverImpl::~TimeZoneResolverImpl() {
+  base::PowerMonitor* power_monitor = base::PowerMonitor::Get();
+  DCHECK(power_monitor);

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

power_monitor could be NULL on shutdown, so just return if NULL here.

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

Done.

+  power_monitor->RemoveObserver(this);
+}
+
+void TimeZoneResolver::TimeZoneResolverImpl::Start() {
+  // Start() is usually called twice:
+  // - On device boot.
+  // - On user session start.
+  if (request_.get() || refresh_timer_.IsRunning())

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

scoped_ptr has a bool overload, so you don't need the .get(). (Same in other
places below).

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

Done.

+    return;
+
+  ScheduleRequest();
+}
+
+// Returns delay to next timezone update request
+base::TimeDelta
+TimeZoneResolver::TimeZoneResolverImpl::CalculateNextInterval() {
+  const base::TimeDelta initial_interval =
+      base::TimeDelta::FromMilliseconds(kInitialRefreshIntervalMS);
+  const base::TimeDelta maximum_interval =
+      base::TimeDelta::FromMilliseconds(kMaximalRefreshIntervalMS);
+
+  // This is initial request, which should be served immediate.

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

Nit: "immediately"

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

Done.

+  if (requests_count_ == 0)
+    return initial_interval;
+
+  // See comment to kRefreshIntervalBasePower.
+  const unsigned long power = kRefreshIntervalBasePower;
+  const unsigned long base_multiplier = 2 << power;
+
+  if (requests_count_ >
+      ::log2(maximum_interval.InSecondsF() / initial_interval.InSecondsF()) /
+          power)

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

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 :)

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

Done.

+    return maximum_interval;
+
+  const base::TimeDelta interval(base::TimeDelta::FromSeconds(
+      initial_interval.InSecondsF() * (base_multiplier << requests_count_)));
+  return std::min(interval, maximum_interval);

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

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).

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

Done.

+}
+
+void TimeZoneResolver::TimeZoneResolverImpl::OnResume() {
+  requests_count_ = 0;
+  // Refresh timezone immediately.
+  request_.reset();
+  ScheduleRequest();
+}
+
+void TimeZoneResolver::TimeZoneResolverImpl::ScheduleRequest() {
+  if (request_.get())
+    return;
+
+  // base::OneShotTimer
+  base::TimeDelta interval = CalculateNextInterval();
+  refresh_timer_.Stop();
+  refresh_timer_.Start(
+      FROM_HERE, interval,
+      base::Bind(&TimeZoneResolver::TimeZoneResolverImpl::CreateNewRequest,
+                 AsWeakPtr()));
+}
+
+void TimeZoneResolver::TimeZoneResolverImpl::CreateNewRequest() {
+  if (request_.get())
+    return;
+
+  refresh_timer_.Stop();
+
+  request_.reset(new TZRequest(this));
+  request_->Start();
+}
+
+void TimeZoneResolver::TimeZoneResolverImpl::RecordAttempt() {
+  ++requests_count_;
+}
+
+TZRequest* TimeZoneResolver::TimeZoneResolverImpl::ReleaseRequest() {
+  return request_.release();

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

This should return a scoped_ptr to ensure that ownership is not lost.

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

Done.

+}
+
+void TimeZoneResolver::TimeZoneResolverImpl::ApplyTimeZone(
+    const TimeZoneResponseData* timezone) {
+  apply_timezone_.Run(timezone);
+}
+
+// ------------------------------------------------------------------------
+// TimeZoneResolver implementation
+
+TimeZoneResolver::TimeZoneResolver(
+    net::URLRequestContextGetter* context,
+    const GURL& url,
+    const ApplyTimeZoneCallback& apply_timezone,
+    const DelayNetworkCallClosure& delay_network_call)
+    : context_(context),
+      url_(url),
+      apply_timezone_(apply_timezone),
+      delay_network_call_(delay_network_call),
+      implementation_(nullptr) {
+  DCHECK(!apply_timezone.is_null());
+}
+
+TimeZoneResolver::~TimeZoneResolver() {
+  Stop();
+}
+
+void TimeZoneResolver::Start() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!implementation_) {
+    implementation_ = new TimeZoneResolverImpl(context_, url_, apply_timezone_,
+                                               delay_network_call_);
+    implementation_->Start();
+  }
+}
+
+void TimeZoneResolver::Stop() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (implementation_) {

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

This seems like an excellent use case for a scoped_ptr :)

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

Done.

+    delete implementation_;
+    implementation_ = nullptr;
+  }
+}
+
+}  // namespace chromeos