[Doodle] Pull time_to_live out of DoodleConfig

[Doodle] Pull time_to_live out of DoodleConfig

It didn't belong there, since by definition it can't be kept around.
Also clients aren't interested in it, since the DoodleService will
handle expiration.

As a positive side effect, this allows us to implement
DoodleConfig::operator==, which makes some things much nicer.

This is a follow-up to https://codereview.chromium.org/2726883002/

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

[Marc Treib, 2017-03-02 11:39:06]

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

[commit-bot: I haz the power, 2017-03-02 11:39:13]

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-02 11:42:51]

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

[Marc Treib, 2017-03-02 11:42:51]

Here's that follow-up I promised. PTAL!

https://codereview.chromium.org/2725143003/diff/1/components/doodle/doodle_fe...
File components/doodle/doodle_fetcher.h (right):

https://codereview.chromium.org/2725143003/diff/1/components/doodle/doodle_fe...
components/doodle/doodle_fetcher.h:31: const base::Optional<DoodleConfig>&
doodle_config)>;
As an alternative, we could wrap ttl and config into a new "DoodleResponse", but
that seems more trouble than it's worth. WDYT?

[commit-bot: I haz the power, 2017-03-02 12:21:48]

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

[commit-bot: I haz the power, 2017-03-02 12:21:48]

Dry run: This issue passed the CQ dry run.

[fhorschig, 2017-03-02 13:18:08]

lgtm w/ nits

https://codereview.chromium.org/2725143003/diff/1/components/doodle/doodle_fe...
File components/doodle/doodle_fetcher.h (right):

https://codereview.chromium.org/2725143003/diff/1/components/doodle/doodle_fe...
components/doodle/doodle_fetcher.h:31: const base::Optional<DoodleConfig>&
doodle_config)>;
On 2017/03/02 11:42:51, Marc Treib wrote:
> As an alternative, we could wrap ttl and config into a new "DoodleResponse",
but
> that seems more trouble than it's worth. WDYT?

Doesn't sound too bad but I think for three parameters,
this solution is good.

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

https://codereview.chromium.org/2725143003/diff/1/components/doodle/doodle_fe...
components/doodle/doodle_fetcher_impl.cc:167: ParseBaseInformation(ddljson,
&doodle, time_to_live);
Is there a good reason to pull this up?
If ParseImage fails, all of this work was unnecessary.

https://codereview.chromium.org/2725143003/diff/1/components/doodle/doodle_se...
File components/doodle/doodle_service.cc (right):

https://codereview.chromium.org/2725143003/diff/1/components/doodle/doodle_se...
components/doodle/doodle_service.cc:47: for (auto& observer : observers_) {
Is non-const auto& something we are allowed to use?

I can not find any decision related to that. 
Codesearch says 50:50 auto* vs auto& ...

So, do as you like, I would prefer a pointer type.
(But I am aware that an observer notification is 
almost impossible to be const.)

[Marc Treib, 2017-03-02 14:30:08]

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

https://codereview.chromium.org/2725143003/diff/1/components/doodle/doodle_fe...
components/doodle/doodle_fetcher_impl.cc:167: ParseBaseInformation(ddljson,
&doodle, time_to_live);
On 2017/03/02 13:18:08, fhorschig wrote:
> Is there a good reason to pull this up?
> If ParseImage fails, all of this work was unnecessary.

Not really, it just seemed a bit weird to me that the base information is parsed
last. I moved it down again.

https://codereview.chromium.org/2725143003/diff/1/components/doodle/doodle_se...
File components/doodle/doodle_service.cc (right):

https://codereview.chromium.org/2725143003/diff/1/components/doodle/doodle_se...
components/doodle/doodle_service.cc:47: for (auto& observer : observers_) {
On 2017/03/02 13:18:08, fhorschig wrote:
> Is non-const auto& something we are allowed to use?

Yes (for locally-scoped stuff like this).
This is actually the default usage of ObserverList:
https://cs.chromium.org/chromium/src/base/observer_list.h?rcl=d83555d59cc6dba...

> I can not find any decision related to that. 
> Codesearch says 50:50 auto* vs auto& ...
> 
> So, do as you like, I would prefer a pointer type.
> (But I am aware that an observer notification is 
> almost impossible to be const.)

It's not a pointer, the auto& resolves to Observer&.

[Marc Treib, 2017-03-02 14:30:33]

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

[commit-bot: I haz the power, 2017-03-02 14:30:45]

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-02 14:30:53]

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

[Marc Treib, 2017-03-02 14:30:53]

+mastiz: Once again, committer's approval please? :)

[commit-bot: I haz the power, 2017-03-02 15:05:36]

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

[commit-bot: I haz the power, 2017-03-02 15:05:37]

Dry run: This issue passed the CQ dry run.

[mastiz, 2017-03-02 19:55:42]

LGTM with optional comments.

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
File components/doodle/doodle_fetcher_impl.h (right):

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
components/doodle/doodle_fetcher_impl.h:67: base::TimeDelta* time_to_live)
const;
Optional: wouldn't it be cleaner to return a Optional<pair<DoodleConfig,
TimeDelta>> and possibly rename to ParseDoodleConfigAndTimeToLive?

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
components/doodle/doodle_fetcher_impl.h:79: const base::Optional<DoodleConfig>&
config);
Ditto: perhaps a typedef or struct could also help.

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

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
components/doodle/doodle_fetcher_impl_unittest.cc:110:
DoodleFetcherImpl::FinishedCallback CreateResponseSavingCallback(
Optional: this function can likely be replaced with a MockCallback together with
testing::SaveArgs.

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

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
components/doodle/doodle_service_unittest.cc:67: base::TimeDelta some_time()
const { return base::TimeDelta::FromHours(1); }
Use a constant instead, kSomeTime?

[fhorschig, 2017-03-02 20:18:56]

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

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
components/doodle/doodle_service_unittest.cc:67: base::TimeDelta some_time()
const { return base::TimeDelta::FromHours(1); }
On 2017/03/02 19:55:42, mastiz wrote:
> Use a constant instead, kSomeTime?

Has const base::TimeDelta and exit-time destructor?
Otherwise, you could make it an implementation detail
named |AnHour()| ... I just looked at your follow-up
and you might save some work here or there.

[Marc Treib, 2017-03-03 09:35:07]

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

[commit-bot: I haz the power, 2017-03-03 09:35:14]

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-03 09:35:26]

Thanks! Some comments addressed ;)

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
File components/doodle/doodle_fetcher_impl.h (right):

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
components/doodle/doodle_fetcher_impl.h:67: base::TimeDelta* time_to_live)
const;
On 2017/03/02 19:55:42, mastiz wrote:
> Optional: wouldn't it be cleaner to return a Optional<pair<DoodleConfig,
> TimeDelta>> and possibly rename to ParseDoodleConfigAndTimeToLive? 

Renamed.

I don't like std::pair much - IMO it often makes things hard to read. A struct
would be an option, but TBH I don't think it's worth the hassle of
packing/unpacking and std::move'ing the DoodleConfig itself...

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
components/doodle/doodle_fetcher_impl.h:79: const base::Optional<DoodleConfig>&
config);
On 2017/03/02 19:55:42, mastiz wrote:
> Ditto: perhaps a typedef or struct could also help.

Same: Unless you feel strongly, I don't think it's worth it.

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

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
components/doodle/doodle_fetcher_impl_unittest.cc:110:
DoodleFetcherImpl::FinishedCallback CreateResponseSavingCallback(
On 2017/03/02 19:55:42, mastiz wrote:
> Optional: this function can likely be replaced with a MockCallback together
with
> testing::SaveArgs.

Added a TODO for now. I agree that this is probably an example where the mockery
stuff will make things cleaner.

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

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
components/doodle/doodle_service_unittest.cc:67: base::TimeDelta some_time()
const { return base::TimeDelta::FromHours(1); }
On 2017/03/02 20:18:56, fhorschig wrote:
> On 2017/03/02 19:55:42, mastiz wrote:
> > Use a constant instead, kSomeTime?
> 
> Has const base::TimeDelta and exit-time destructor?
> Otherwise, you could make it an implementation detail
> named |AnHour()| ... I just looked at your follow-up
> and you might save some work here or there.

...I don't follow?

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
components/doodle/doodle_service_unittest.cc:67: base::TimeDelta some_time()
const { return base::TimeDelta::FromHours(1); }
On 2017/03/02 19:55:42, mastiz wrote:
> Use a constant instead, kSomeTime?

Constants of non-trivial types are mostly disallowed, because we don't like
static initializers.

I've just inlined it below, since the helper really doesn't save much anyway.

[commit-bot: I haz the power, 2017-03-03 10:10:16]

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

[commit-bot: I haz the power, 2017-03-03 10:10:16]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-03-03 10:12:13]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2017-03-03 10:12:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
fhorschig@chromium.org, mastiz@chromium.org
Link to the patchset: https://codereview.chromium.org/2725143003/#ps40001
(title: "mastiz review")

[commit-bot: I haz the power, 2017-03-03 10:12:26]

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-03 10:16:26]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488535933434240,
"parent_rev": "6a8242fa680b56f7052e1233e9472bf00ed1e27b", "commit_rev":
"ebcd266f8e04c1ebebf3e0eca08f6683641cf613"}

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

Description was changed from

==========
[Doodle] Pull time_to_live out of DoodleConfig

It didn't belong there, since by definition it can't be kept around.
Also clients aren't interested in it, since the DoodleService will
handle expiration.

As a positive side effect, this allows us to implement
DoodleConfig::operator==, which makes some things much nicer.

This is a follow-up to https://codereview.chromium.org/2726883002/

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

to

==========
[Doodle] Pull time_to_live out of DoodleConfig

It didn't belong there, since by definition it can't be kept around.
Also clients aren't interested in it, since the DoodleService will
handle expiration.

As a positive side effect, this allows us to implement
DoodleConfig::operator==, which makes some things much nicer.

This is a follow-up to https://codereview.chromium.org/2726883002/

BUG=690467

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

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

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/ebcd266f8e04c1ebebf3e0eca08f...

[mastiz, 2017-03-03 11:17:44]

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
File components/doodle/doodle_fetcher_impl.h (right):

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
components/doodle/doodle_fetcher_impl.h:79: const base::Optional<DoodleConfig>&
config);
On 2017/03/03 09:35:26, Marc Treib wrote:
> On 2017/03/02 19:55:42, mastiz wrote:
> > Ditto: perhaps a typedef or struct could also help.
> 
> Same: Unless you feel strongly, I don't think it's worth it.

I was certainly not feeling strongly, so it's fine. What I dislike about the
current signature is that it's not clear that the function returns both or
neither.

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

https://codereview.chromium.org/2725143003/diff/20001/components/doodle/doodl...
components/doodle/doodle_service_unittest.cc:67: base::TimeDelta some_time()
const { return base::TimeDelta::FromHours(1); }
On 2017/03/03 09:35:26, Marc Treib wrote:
> On 2017/03/02 19:55:42, mastiz wrote:
> > Use a constant instead, kSomeTime?
> 
> Constants of non-trivial types are mostly disallowed, because we don't like
> static initializers.
> 
> I've just inlined it below, since the helper really doesn't save much anyway.

Right, the solution is to not make it static. Simply a public (or protected)
variable that is const or constexpr.