[Doodle] Replace the expiry_date in DoodleConfig by time_to_live

[Doodle] Replace the expiry_date in DoodleConfig by time_to_live

The service will require both Time (for persisting) and TimeTicks (for
scheduling), so better to just pass the TimeDelta.
It also makes the fetcher nicer (no need for injectable clock).

In a later follow-up, I'll move the ttl out of DoodleConfig (and
instead pass it as a separate param to DoodleFetcher::FinishedCallback).

BUG=690467
Review-Url: https://codereview.chromium.org/2726883002
Cr-Commit-Position: refs/heads/master@{#454254}
Committed: https://chromium.googlesource.com/chromium/src/+/d83555d59cc6dba31282c95cd402ebbcdc42d016

[Marc Treib, 2017-03-01 14:44:18]

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

[commit-bot: I haz the power, 2017-03-01 14:44:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marc Treib, 2017-03-01 14:44:29]

treib@chromium.org changed reviewers:
+ fhorschig@chromium.org

[Marc Treib, 2017-03-01 14:44:29]

PTAL!

[commit-bot: I haz the power, 2017-03-01 15:17:10]

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

[commit-bot: I haz the power, 2017-03-01 15:17:12]

Dry run: This issue passed the CQ dry run.

[fhorschig, 2017-03-01 15:40:53]

TTL seems much more reasonable. 
Especially because we can move the expiry logic out.

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_fe...
File components/doodle/doodle_fetcher_impl.cc (right):

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_fe...
components/doodle/doodle_fetcher_impl.cc:71: DoodleConfig::DoodleConfig() :
doodle_type(DoodleType::UNKNOWN) {}
Do we want the default TTL to be 0?
Alternatives would be +/- TimeDelta::Max();

-TimeDelta::Max() is something I would like
because you could see that the config isn't
valid.

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_fe...
components/doodle/doodle_fetcher_impl.cc:209: // TODO(treib,fhorschig): Should
this logic live in the service instead?
In my opinion absolutely. The clamping was mainly a requirement
for having an expiry time as we didn't want to pass the raw TTL.

If there is no opposing view, you can drop the question mark or
implement it in the service. 
(TODO would be fine with me so we can split the CLs.)

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_fe...
File components/doodle/doodle_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_fe...
components/doodle/doodle_fetcher_impl_unittest.cc:245:
TEST_F(DoodleFetcherImplTest, DoodleExpiresNowWithNegativeTTL) {
Can we change the name to remove the "Now"?
e.g. ShouldExpireImmediatelyWithNegativeTTL

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_fe...
components/doodle/doodle_fetcher_impl_unittest.cc:262:
TEST_F(DoodleFetcherImplTest, DoodleExpiresNowWithoutValidTTL) {
ShouldExpireImmediatelyWithoutValidTTL?

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_ty...
File components/doodle/doodle_types.cc (right):

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_ty...
components/doodle/doodle_types.cc:21: // place.
What do you mean by "shouldn't be here in the first place"?
In this function or in the Config at all?

Maybe it's more important to just say that the TTL is not 
sufficient for distinguishing 2 Doodles.

[Marc Treib, 2017-03-02 09:28:03]

Thanks!

TTL is reasonable from the fetcher's POV, but unfortunately it's completely out
of place in DoodleConfig. But I'll fix that soon!

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_fe...
File components/doodle/doodle_fetcher_impl.cc (right):

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_fe...
components/doodle/doodle_fetcher_impl.cc:71: DoodleConfig::DoodleConfig() :
doodle_type(DoodleType::UNKNOWN) {}
On 2017/03/01 15:40:52, fhorschig wrote:
> Do we want the default TTL to be 0?
> Alternatives would be +/- TimeDelta::Max();
> 
> -TimeDelta::Max() is something I would like
> because you could see that the config isn't
> valid.

Hm. Max() I don't like, because then we'd have a config that never expires.
-Max() is more reasonable, but it also seems a bit magic, in the sense that it's
not clear why this exact value would correspond to "invalid". It doesn't seem to
be a common convention.

TBH, "expires right now" seems like a reasonable default for the uninitialized
case to me. Kinda similar to how we set the image sizes to 0 rather than
INT_MIN. So unless you insist, I'd keep it as is.

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_fe...
components/doodle/doodle_fetcher_impl.cc:209: // TODO(treib,fhorschig): Should
this logic live in the service instead?
On 2017/03/01 15:40:52, fhorschig wrote:
> In my opinion absolutely. The clamping was mainly a requirement
> for having an expiry time as we didn't want to pass the raw TTL.
> 
> If there is no opposing view, you can drop the question mark or
> implement it in the service. 
> (TODO would be fine with me so we can split the CLs.)

Reformulated without a question :)

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_fe...
File components/doodle/doodle_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_fe...
components/doodle/doodle_fetcher_impl_unittest.cc:245:
TEST_F(DoodleFetcherImplTest, DoodleExpiresNowWithNegativeTTL) {
On 2017/03/01 15:40:52, fhorschig wrote:
> Can we change the name to remove the "Now"?
> e.g. ShouldExpireImmediatelyWithNegativeTTL

Renamed to DoodleExpiresImmediatelyWithNegativeTTL.

(All the other tests use DoesTheThing rather than ShouldDoTheThing. We can
rename them if you want, I honestly don't care one way or the other, but we
ShouldDoThatInASeparateCL :D)

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_fe...
components/doodle/doodle_fetcher_impl_unittest.cc:262:
TEST_F(DoodleFetcherImplTest, DoodleExpiresNowWithoutValidTTL) {
On 2017/03/01 15:40:52, fhorschig wrote:
> ShouldExpireImmediatelyWithoutValidTTL?

DoodleExpiresImmediatelyWithoutValidTTL

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_ty...
File components/doodle/doodle_types.cc (right):

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_ty...
components/doodle/doodle_types.cc:21: // place.
On 2017/03/01 15:40:52, fhorschig wrote:
> What do you mean by "shouldn't be here in the first place"?
> In this function or in the Config at all?

In the config. Made that clear in the comment.

> Maybe it's more important to just say that the TTL is not 
> sufficient for distinguishing 2 Doodles.

Actually the other way around is more important: Two Doodles with different TTL
(but everything else the same) must still be considered equivalent.
Anyway, I'll send a followup Real Soon Now that'll remove the field from the
config.

[fhorschig, 2017-03-02 10:35:13]

lgtm

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_fe...
File components/doodle/doodle_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/2726883002/diff/1/components/doodle/doodle_fe...
components/doodle/doodle_fetcher_impl_unittest.cc:245:
TEST_F(DoodleFetcherImplTest, DoodleExpiresNowWithNegativeTTL) {
On 2017/03/02 09:28:02, Marc Treib wrote:
> On 2017/03/01 15:40:52, fhorschig wrote:
> > Can we change the name to remove the "Now"?
> > e.g. ShouldExpireImmediatelyWithNegativeTTL
> 
> Renamed to DoodleExpiresImmediatelyWithNegativeTTL.
> 
> (All the other tests use DoesTheThing rather than ShouldDoTheThing. We can
> rename them if you want, I honestly don't care one way or the other, but we
> ShouldDoThatInASeparateCL :D)

I like that one and it's not pressing.

(Actually, we have two standards for different cases: 
1.III.A. ShouldBePreferedOverEverything
1.III.B. DoesEmphaziseActions)

[Marc Treib, 2017-03-02 10:41:22]

treib@chromium.org changed reviewers:
+ mastiz@chromium.org

[Marc Treib, 2017-03-02 10:41:22]

Thanks!

+mastiz: Can I get a committer stamp, please? :)

[Marc Treib, 2017-03-02 10:41:28]

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

[commit-bot: I haz the power, 2017-03-02 10:41:55]

Dry run: 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-03-02 11:15:43]

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

[commit-bot: I haz the power, 2017-03-02 11:15:44]

Dry run: This issue passed the CQ dry run.

[mastiz, 2017-03-02 13:50:14]

LGTM.

https://codereview.chromium.org/2726883002/diff/20001/components/doodle/doodl...
File components/doodle/doodle_types.cc (right):

https://codereview.chromium.org/2726883002/diff/20001/components/doodle/doodl...
components/doodle/doodle_types.cc:21: // the first place.
Nit: it's not obvious to me that time_to_live isn't constant over time.

[Marc Treib, 2017-03-02 13:53:04]

Thanks!

https://codereview.chromium.org/2726883002/diff/20001/components/doodle/doodl...
File components/doodle/doodle_types.cc (right):

https://codereview.chromium.org/2726883002/diff/20001/components/doodle/doodl...
components/doodle/doodle_types.cc:21: // the first place.
On 2017/03/02 13:50:13, mastiz wrote:
> Nit: it's not obvious to me that time_to_live isn't constant over time.

Well, it's a TimeDelta, so it's relative to "now". Anyway, the CL that removes
it from here is already out: https://codereview.chromium.org/2725143003/

[Marc Treib, 2017-03-02 13:53:12]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2017-03-02 13:53:30]

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-03-02 13:59:14]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488462792211190,
"parent_rev": "31f64f6b4a597af9c53e7e4c4c87298a14ff4ddd", "commit_rev":
"d83555d59cc6dba31282c95cd402ebbcdc42d016"}

[commit-bot: I haz the power, 2017-03-02 13:59:50]

Description was changed from

==========
[Doodle] Replace the expiry_date in DoodleConfig by time_to_live

The service will require both Time (for persisting) and TimeTicks (for
scheduling), so better to just pass the TimeDelta.
It also makes the fetcher nicer (no need for injectable clock).

In a later follow-up, I'll move the ttl out of DoodleConfig (and
instead pass it as a separate param to DoodleFetcher::FinishedCallback).

BUG=690467
==========

to

==========
[Doodle] Replace the expiry_date in DoodleConfig by time_to_live

The service will require both Time (for persisting) and TimeTicks (for
scheduling), so better to just pass the TimeDelta.
It also makes the fetcher nicer (no need for injectable clock).

In a later follow-up, I'll move the ttl out of DoodleConfig (and
instead pass it as a separate param to DoodleFetcher::FinishedCallback).

BUG=690467

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

[commit-bot: I haz the power, 2017-03-02 13:59:51]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d83555d59cc6dba31282c95cd402...