[Night Light] CL12: String changes and fix frequent requests.

chrome/browser/chromeos/night_light/night_light_client.cc

Index: chrome/browser/chromeos/night_light/night_light_client.cc
diff --git a/chrome/browser/chromeos/night_light/night_light_client.cc b/chrome/browser/chromeos/night_light/night_light_client.cc
index 0b8941ba207973398bb177a215c7ea42d0f3b938..fe89a1d50859ade345c5781d05125a3d5807eb7e 100644
--- a/chrome/browser/chromeos/night_light/night_light_client.cc
+++ b/chrome/browser/chromeos/night_light/night_light_client.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/chromeos/night_light/night_light_client.h"
 
+#include <algorithm>
+
 #include "ash/public/interfaces/constants.mojom.h"
 #include "content/public/common/service_manager_connection.h"
 #include "services/service_manager/public/cpp/connector.h"
@@ -90,9 +92,11 @@ void NightLightClient::OnGeoposition(const chromeos::Geoposition& position,
     return;
   }
 
-  night_light_controller_->SetCurrentGeoposition(
-      ash::mojom::SimpleGeoposition::New(position.latitude,
-                                         position.longitude));
+  last_successful_request_ = base::Time::Now();
+
+  latitude_ = position.latitude;
+  longitude_ = position.longitude;
+  SendCurrentGeoposition();
 
   // On success, reset the backoff delay to its minimum value, and schedule
   // another request.
@@ -100,11 +104,30 @@ void NightLightClient::OnGeoposition(const chromeos::Geoposition& position,
   ScheduleNextRequest(kNextRequestDelayAfterSuccess);
 }
 
+void NightLightClient::SendCurrentGeoposition() {
+  night_light_controller_->SetCurrentGeoposition(
+      ash::mojom::SimpleGeoposition::New(latitude_, longitude_));
+}
+
 void NightLightClient::ScheduleNextRequest(base::TimeDelta delay) {
   timer_.Start(FROM_HERE, delay, this, &NightLightClient::RequestGeoposition);
 }
 
 void NightLightClient::RequestGeoposition() {
+  // No need to request a new position if we already have a valid one from a

[James Cook, 2017/06/30 19:12:26]

nit: Either document how this can happen (from schedule type change) or put the
duplicate-suppression logic in OnScheduleTypeChanged() to make clear that's the
cause.

[afakhry, 2017/06/30 22:28:28]

Makes sense. Done.

+  // request less than kNextRequestDelayAfterSuccess ago.
+  base::Time now = base::Time::Now();
+  if ((now - last_successful_request_) < kNextRequestDelayAfterSuccess) {
+    SendCurrentGeoposition();
+
+    // Next request is either immediate or kNextRequestDelayAfterSuccess later
+    // than the last success time, whichever is greater.
+    ScheduleNextRequest(std::max(
+        base::TimeDelta::FromSeconds(0),
+        last_successful_request_ + kNextRequestDelayAfterSuccess - now));
+    return;
+  }

[James Cook, 2017/06/30 19:12:26]

Is it possible to write a test for this behavior?

[afakhry, 2017/06/30 22:28:28]

Now that we moved this code outside RequestGeoposition(), it's much easier now;
though dealing with time and timers is always not fun!

Done.

+
   provider_.RequestGeolocation(
       kGeolocationRequestTimeout, false /* send_wifi_access_points */,
       false /* send_cell_towers */,