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

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

- Implement requests to change the tooltip text of the tray Night Light
  button to "Night Light: On/Off".
- Fix changing the schedule type to "sunset to sunrise" triggering a new
  geoposition request every time.
- Add OWNERS.

BUG=736856, 705816
Review-Url: https://codereview.chromium.org/2966873002
Cr-Commit-Position: refs/heads/master@{#484406}
Committed: https://chromium.googlesource.com/chromium/src/+/e955e728e2a1ed9225ab13d2751b07eff7d291b4

[afakhry, 2017-06-30 18:52:32]

afakhry@chromium.org changed reviewers:
+ jamescook@chromium.org

[afakhry, 2017-06-30 18:52:32]

Hello James, I hope I caught you before you're OOO! :)

Please take a look at this CL. Thank you!

[afakhry, 2017-06-30 19:05:54]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-30 19:06:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2966873002/diff/1/chrome/browser/chromeos/nig...
File chrome/browser/chromeos/night_light/night_light_client.cc (right):

https://codereview.chromium.org/2966873002/diff/1/chrome/browser/chromeos/nig...
chrome/browser/chromeos/night_light/night_light_client.cc:117: // No need to
request a new position if we already have a valid one from a
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.

https://codereview.chromium.org/2966873002/diff/1/chrome/browser/chromeos/nig...
chrome/browser/chromeos/night_light/night_light_client.cc:129: }
Is it possible to write a test for this behavior?

https://codereview.chromium.org/2966873002/diff/1/chrome/browser/chromeos/nig...
File chrome/browser/chromeos/night_light/night_light_client.h (right):

https://codereview.chromium.org/2966873002/diff/1/chrome/browser/chromeos/nig...
chrome/browser/chromeos/night_light/night_light_client.h:49: void
SendCurrentGeoposition();
nit: docs (send to who?)

https://codereview.chromium.org/2966873002/diff/1/chrome/browser/chromeos/nig...
chrome/browser/chromeos/night_light/night_light_client.h:69: base::Time
last_successful_request_;
optional: maybe |last_successful_geo_request_|?

[commit-bot: I haz the power, 2017-06-30 19:53:46]

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

[commit-bot: I haz the power, 2017-06-30 19:53:47]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2966873002/diff/1/chrome/browser/chromeos/nig...
File chrome/browser/chromeos/night_light/night_light_client.cc (right):

https://codereview.chromium.org/2966873002/diff/1/chrome/browser/chromeos/nig...
chrome/browser/chromeos/night_light/night_light_client.cc:117: // No need to
request a new position if we already have a valid one from a
On 2017/06/30 19:12:26, James Cook wrote:
> 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.

Makes sense. Done.

https://codereview.chromium.org/2966873002/diff/1/chrome/browser/chromeos/nig...
chrome/browser/chromeos/night_light/night_light_client.cc:129: }
On 2017/06/30 19:12:26, James Cook wrote:
> Is it possible to write a test for this behavior?

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

Done.

https://codereview.chromium.org/2966873002/diff/1/chrome/browser/chromeos/nig...
File chrome/browser/chromeos/night_light/night_light_client.h (right):

https://codereview.chromium.org/2966873002/diff/1/chrome/browser/chromeos/nig...
chrome/browser/chromeos/night_light/night_light_client.h:49: void
SendCurrentGeoposition();
On 2017/06/30 19:12:26, James Cook wrote:
> nit: docs (send to who?)

Done.

https://codereview.chromium.org/2966873002/diff/1/chrome/browser/chromeos/nig...
chrome/browser/chromeos/night_light/night_light_client.h:69: base::Time
last_successful_request_;
On 2017/06/30 19:12:26, James Cook wrote:
> optional: maybe |last_successful_geo_request_|?

Done. Actually named it |last_successful_geo_request_time_|.

[James Cook, 2017-06-30 22:50:07]

LGTM with nits. Thanks for adding tests! Time is tough.

https://codereview.chromium.org/2966873002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/night_light/night_light_client_unittest.cc (right):

https://codereview.chromium.org/2966873002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/night_light/night_light_client_unittest.cc:130:
TEST_F(NightLightClientTest, TestInavlidPositions) {
nit: invalid

https://codereview.chromium.org/2966873002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/night_light/night_light_client_unittest.cc:193: // To
avoid flakiness, we get the absolute value of difference between the
This makes me a little nervous. As you mention, testing time is hard. I suggest
either:

* Injecting a Clock and a TickClock into the NightLightClient constructor and
using it to build the timer, which will allow you to use test clocks to exactly
control time, or 
* Putting this bit in a separate test so if it flakes it can be disabled
independently from the rest of the code, or
* Just deleting the part of the test after line 188

[afakhry, 2017-06-30 23:41:31]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-30 23:41:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2017-06-30 23:41:46]

https://codereview.chromium.org/2966873002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/night_light/night_light_client_unittest.cc (right):

https://codereview.chromium.org/2966873002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/night_light/night_light_client_unittest.cc:130:
TEST_F(NightLightClientTest, TestInavlidPositions) {
On 2017/06/30 22:50:06, James Cook wrote:
> nit: invalid

Done.

https://codereview.chromium.org/2966873002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/night_light/night_light_client_unittest.cc:193: // To
avoid flakiness, we get the absolute value of difference between the
On 2017/06/30 22:50:06, James Cook wrote:
> This makes me a little nervous. As you mention, testing time is hard. I
suggest
> either:
> 
> * Injecting a Clock and a TickClock into the NightLightClient constructor and
> using it to build the timer, which will allow you to use test clocks to
exactly
> control time, or 
> * Putting this bit in a separate test so if it flakes it can be disabled
> independently from the rest of the code, or
> * Just deleting the part of the test after line 188
> 

I opted for the first suggestion. Please take a look.

[afakhry, 2017-06-30 23:46:47]

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

[afakhry, 2017-06-30 23:46:47]

+stevenjb@ for c/b/chromeos

[stevenjb, 2017-07-01 00:02:36]

https://codereview.chromium.org/2966873002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/night_light/night_light_client.cc (right):

https://codereview.chromium.org/2966873002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/night_light/night_light_client.cc:80:
kNextRequestDelayAfterSuccess - now));
nit: With the extra complexity here, invert the if and early exit for the
simpler case to reduce indentation of this more complicated case.

https://codereview.chromium.org/2966873002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/night_light/night_light_client.h (right):

https://codereview.chromium.org/2966873002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/night_light/night_light_client.h:32: explicit
NightLightClient(net::URLRequestContextGetter* url_context_getter);
Avoid multiple constructors for high level classes like this.

Since it looks like this is for testing, prefer a SetClocksForTest() method
instead of a separate constructor.

[James Cook, 2017-07-01 00:06:57]

LGTM with steven's comments addressed

https://codereview.chromium.org/2966873002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/night_light/night_light_client.h (right):

https://codereview.chromium.org/2966873002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/night_light/night_light_client.h:32: explicit
NightLightClient(net::URLRequestContextGetter* url_context_getter);
On 2017/07/01 00:02:35, stevenjb wrote:
> Avoid multiple constructors for high level classes like this.
> 
> Since it looks like this is for testing, prefer a SetClocksForTest() method
> instead of a separate constructor. 

Or make the parameters mandatory and use a SystemClock where you allocate the
object.

[afakhry, 2017-07-01 00:26:15]

https://codereview.chromium.org/2966873002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/night_light/night_light_client.cc (right):

https://codereview.chromium.org/2966873002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/night_light/night_light_client.cc:80:
kNextRequestDelayAfterSuccess - now));
On 2017/07/01 00:02:35, stevenjb wrote:
> nit: With the extra complexity here, invert the if and early exit for the
> simpler case to reduce indentation of this more complicated case.

Done.

https://codereview.chromium.org/2966873002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/night_light/night_light_client.h (right):

https://codereview.chromium.org/2966873002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/night_light/night_light_client.h:32: explicit
NightLightClient(net::URLRequestContextGetter* url_context_getter);
On 2017/07/01 00:02:35, stevenjb wrote:
> Avoid multiple constructors for high level classes like this.
> 
> Since it looks like this is for testing, prefer a SetClocksForTest() method
> instead of a separate constructor. 

The problem is base::Timer, for which we cannot change its TickClock once
constructed:
https://cs.chromium.org/chromium/src/base/timer/timer.h?q=base::Timer+package...,
so we need to have this in the constructor.

I removed the explicit constructor, and added a comment that passing nullptr
will result in using the default system clocks, which is the behavior of all the
Timers (ex:
https://cs.chromium.org/chromium/src/base/timer/timer.h?q=base::Timer+package...).

[stevenjb, 2017-07-01 00:31:26]

lgtm with a couple of suggestions

https://codereview.chromium.org/2966873002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/night_light/night_light_client.h (right):

https://codereview.chromium.org/2966873002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/night_light/night_light_client.h:32: explicit
NightLightClient(net::URLRequestContextGetter* url_context_getter);
On 2017/07/01 00:26:15, afakhry wrote:
> On 2017/07/01 00:02:35, stevenjb wrote:
> > Avoid multiple constructors for high level classes like this.
> > 
> > Since it looks like this is for testing, prefer a SetClocksForTest() method
> > instead of a separate constructor. 
> 
> The problem is base::Timer, for which we cannot change its TickClock once
> constructed:
>
https://cs.chromium.org/chromium/src/base/timer/timer.h?q=base::Timer+package...,
> so we need to have this in the constructor.
> 
> I removed the explicit constructor, and added a comment that passing nullptr
> will result in using the default system clocks, which is the behavior of all
the
> Timers (ex:
>
https://cs.chromium.org/chromium/src/base/timer/timer.h?q=base::Timer+package...).
> 
Could you use a unique_ptr<> for timer_?

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

https://codereview.chromium.org/2966873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:996:
g_browser_process->system_request_context(), nullptr, nullptr);
Comment these, e.g. nullptr /* no TickClock override */

[afakhry, 2017-07-01 00:31:37]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-01 00:31:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2017-07-01 00:57:30]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-01 00:57:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2017-07-01 00:57:45]

https://codereview.chromium.org/2966873002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/night_light/night_light_client.h (right):

https://codereview.chromium.org/2966873002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/night_light/night_light_client.h:32: explicit
NightLightClient(net::URLRequestContextGetter* url_context_getter);
On 2017/07/01 00:31:26, stevenjb wrote:
> On 2017/07/01 00:26:15, afakhry wrote:
> > On 2017/07/01 00:02:35, stevenjb wrote:
> > > Avoid multiple constructors for high level classes like this.
> > > 
> > > Since it looks like this is for testing, prefer a SetClocksForTest()
method
> > > instead of a separate constructor. 
> > 
> > The problem is base::Timer, for which we cannot change its TickClock once
> > constructed:
> >
>
https://cs.chromium.org/chromium/src/base/timer/timer.h?q=base::Timer+package...,
> > so we need to have this in the constructor.
> > 
> > I removed the explicit constructor, and added a comment that passing nullptr
> > will result in using the default system clocks, which is the behavior of all
> the
> > Timers (ex:
> >
>
https://cs.chromium.org/chromium/src/base/timer/timer.h?q=base::Timer+package...).
> > 
> Could you use a unique_ptr<> for timer_?
> 

Done. So much for those tests! :(

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

https://codereview.chromium.org/2966873002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:996:
g_browser_process->system_request_context(), nullptr, nullptr);
On 2017/07/01 00:31:26, stevenjb wrote:
> Comment these, e.g. nullptr /* no TickClock override */

Not needed anymore.

[commit-bot: I haz the power, 2017-07-01 02:06:57]

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

[commit-bot: I haz the power, 2017-07-01 02:06:58]

Dry run: This issue passed the CQ dry run.

[afakhry, 2017-07-05 16:24:30]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2017-07-05 16:24:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2966873002/#ps80001
(title: "Steven's suggestions")

[commit-bot: I haz the power, 2017-07-05 16:24:44]

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, 2017-07-05 19:22:54]

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

[commit-bot: I haz the power, 2017-07-05 19:22:55]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[afakhry, 2017-07-05 20:31:21]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2017-07-05 20:31:42]

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, 2017-07-05 22:32:57]

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

[commit-bot: I haz the power, 2017-07-05 22:32:58]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[afakhry, 2017-07-06 00:07:59]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2017-07-06 00:08:49]

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, 2017-07-06 00:20:54]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499299679232570,
"parent_rev": "7fa8c45949cae56fa51370e432ae8d25627da0ba", "commit_rev":
"6451ea1dc52ae19ba0d97e33ef371f0f27921381"}

[commit-bot: I haz the power, 2017-07-06 00:21:27]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499299679232570,
"parent_rev": "e577029f6cd54e8650c702999d56b7af721c6198", "commit_rev":
"bf3cd54d4de215e74cdc24549378c2c7648fad72"}

[commit-bot: I haz the power, 2017-07-06 00:21:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499299679232570,
"parent_rev": "e369fbdecbe1d12a2665218c5c9e3ae1d2e563d9", "commit_rev":
"e955e728e2a1ed9225ab13d2751b07eff7d291b4"}

[commit-bot: I haz the power, 2017-07-06 00:21:56]

Description was changed from

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

- Implement requests to change the tooltip text of the tray Night Light
  button to "Night Light: On/Off".
- Fix changing the schedule type to "sunset to sunrise" triggering a new
  geoposition request every time.
- Add OWNERS.

BUG=736856,705816
==========

to

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

- Implement requests to change the tooltip text of the tray Night Light
  button to "Night Light: On/Off".
- Fix changing the schedule type to "sunset to sunrise" triggering a new
  geoposition request every time.
- Add OWNERS.

BUG=736856,705816

Review-Url: https://codereview.chromium.org/2966873002
Cr-Commit-Position: refs/heads/master@{#484406}
Committed:
https://chromium.googlesource.com/chromium/src/+/e955e728e2a1ed9225ab13d2751b...
==========

[commit-bot: I haz the power, 2017-07-06 00:21:58]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e955e728e2a1ed9225ab13d2751b...