[NTP Snippets] Implement snippets expiry

[NTP Snippets] Implement snippets expiry

BUG=586134
Committed: https://crrev.com/3110b49ebf6342f75fefd539f50da36d8504c054
Cr-Commit-Position: refs/heads/master@{#378441}

[Marc Treib, 2016-02-29 17:44:51]

https://codereview.chromium.org/1743333002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java
(right):

https://codereview.chromium.org/1743333002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:108:
if (params[0].isEmpty()) return null;
Totally unrelated - this gets rid of a lot of useless log noise when the
thumbnail URL is missing.

[Marc Treib, 2016-02-29 17:45:17]

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

[Marc Treib, 2016-02-29 17:45:17]

PTAL!

[Bernhard Bauer, 2016-03-01 11:05:04]

https://codereview.chromium.org/1743333002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java
(right):

https://codereview.chromium.org/1743333002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:108:
if (params[0].isEmpty()) return null;
On 2016/02/29 17:44:51, Marc Treib wrote:
> Totally unrelated - this gets rid of a lot of useless log noise when the
> thumbnail URL is missing.

Acknowledged.

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:175: ScheduleExpiryTimer();
Do we also want to expire old items, if this is called at startup?

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:206: for (const auto& snippet :
snippets_) {
You could go all-out here as well, and use std::min_element :)

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:218: snippets_.erase(
This only erases a single element, but remove_if can potentially remove more
than one element, no?

(It would also be nice if this method could deal with no snippet being expired
yet, so we could call it in more places, e.g. at startup.)

[Marc Treib, 2016-03-01 11:11:39]

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:175: ScheduleExpiryTimer();
On 2016/03/01 11:05:04, Bernhard Bauer wrote:
> Do we also want to expire old items, if this is called at startup?

Haha, I just uploaded a new patch set that does that!

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:218: snippets_.erase(
On 2016/03/01 11:05:04, Bernhard Bauer wrote:
> This only erases a single element, but remove_if can potentially remove more
> than one element, no?

Nope, this erases all expired elements. std::remove_if is weird - it returns the
new end iterator. The erase() call around it then actually removes everything
between the new end and the old end.

> (It would also be nice if this method could deal with no snippet being expired
> yet, so we could call it in more places, e.g. at startup.)

Hmm, what do you mean? If nothing is expired, then nothing will be removed.

[Marc Treib, 2016-03-01 11:22:45]

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:206: for (const auto& snippet :
snippets_) {
On 2016/03/01 11:05:04, Bernhard Bauer wrote:
> You could go all-out here as well, and use std::min_element :)

Hm, I could, but then I'd have to define a comparator lambda and touch iterators
(eewww). It's actually more code and IMO harder to read than the loop.

[Bernhard Bauer, 2016-03-01 11:54:27]

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:206: for (const auto& snippet :
snippets_) {
On 2016/03/01 11:22:45, Marc Treib wrote:
> On 2016/03/01 11:05:04, Bernhard Bauer wrote:
> > You could go all-out here as well, and use std::min_element :)
> 
> Hm, I could, but then I'd have to define a comparator lambda and touch
iterators
> (eewww). It's actually more code and IMO harder to read than the loop.

Well, you already use a predicate lambda below... hence "all-out" :)

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:218: snippets_.erase(
On 2016/03/01 11:11:39, Marc Treib wrote:
> On 2016/03/01 11:05:04, Bernhard Bauer wrote:
> > This only erases a single element, but remove_if can potentially remove more
> > than one element, no?
> 
> Nope, this erases all expired elements. std::remove_if is weird - it returns
the
> new end iterator. The erase() call around it then actually removes everything
> between the new end and the old end.

Oh, your indentation on line 223 is misleading -- it's the second argument to
the top-level erase() call, but it looks like an argument to remove_if(). That's
why I thought it's the single-argument form of erase().

> > (It would also be nice if this method could deal with no snippet being
expired
> > yet, so we could call it in more places, e.g. at startup.)
> 
> Hmm, what do you mean? If nothing is expired, then nothing will be removed.

NM, that was under the assumption that you'd remove a single element (see
above).

https://codereview.chromium.org/1743333002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1743333002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:211: // Note: If |next_expiry|
is in the past, this will run immediately.
Would this still happen now? Should we rather DCHECK?

https://codereview.chromium.org/1743333002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:229: ScheduleExpiryTimer();
You could also merge these two methods now.

[Marc Treib, 2016-03-01 13:16:50]

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:206: for (const auto& snippet :
snippets_) {
On 2016/03/01 11:54:27, Bernhard Bauer wrote:
> On 2016/03/01 11:22:45, Marc Treib wrote:
> > On 2016/03/01 11:05:04, Bernhard Bauer wrote:
> > > You could go all-out here as well, and use std::min_element :)
> > 
> > Hm, I could, but then I'd have to define a comparator lambda and touch
> iterators
> > (eewww). It's actually more code and IMO harder to read than the loop.
> 
> Well, you already use a predicate lambda below... hence "all-out" :)

The code with min_element would look like this:

  auto min_it = std::min_element(
      snippets_.begin(), snippets_.end(),
      [](const scoped_ptr<NTPSnippet>& lhs, const scoped_ptr<NTPSnippet>& rhs) {
        return lhs->expiry_date() < rhs->expiry_date();
      });
  base::Time next_expiry = (*min_it)->expiry_date();

I prefer what's there now TBH, but if you insist...? :)

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:218: snippets_.erase(
On 2016/03/01 11:54:27, Bernhard Bauer wrote:
> On 2016/03/01 11:11:39, Marc Treib wrote:
> > On 2016/03/01 11:05:04, Bernhard Bauer wrote:
> > > This only erases a single element, but remove_if can potentially remove
more
> > > than one element, no?
> > 
> > Nope, this erases all expired elements. std::remove_if is weird - it returns
> the
> > new end iterator. The erase() call around it then actually removes
everything
> > between the new end and the old end.
> 
> Oh, your indentation on line 223 is misleading -- it's the second argument to
> the top-level erase() call, but it looks like an argument to remove_if().
That's
> why I thought it's the single-argument form of erase().
> 
> > > (It would also be nice if this method could deal with no snippet being
> expired
> > > yet, so we could call it in more places, e.g. at startup.)
> > 
> > Hmm, what do you mean? If nothing is expired, then nothing will be removed.
> 
> NM, that was under the assumption that you'd remove a single element (see
> above).

Fixed the indentation.

https://codereview.chromium.org/1743333002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1743333002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:211: // Note: If |next_expiry|
is in the past, this will run immediately.
On 2016/03/01 11:54:27, Bernhard Bauer wrote:
> Would this still happen now? Should we rather DCHECK?

Done.

https://codereview.chromium.org/1743333002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:229: ScheduleExpiryTimer();
On 2016/03/01 11:54:27, Bernhard Bauer wrote:
> You could also merge these two methods now.

Done.

[Bernhard Bauer, 2016-03-01 15:13:18]

LGTM

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1743333002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:206: for (const auto& snippet :
snippets_) {
On 2016/03/01 13:16:49, Marc Treib wrote:
> On 2016/03/01 11:54:27, Bernhard Bauer wrote:
> > On 2016/03/01 11:22:45, Marc Treib wrote:
> > > On 2016/03/01 11:05:04, Bernhard Bauer wrote:
> > > > You could go all-out here as well, and use std::min_element :)
> > > 
> > > Hm, I could, but then I'd have to define a comparator lambda and touch
> > iterators
> > > (eewww). It's actually more code and IMO harder to read than the loop.
> > 
> > Well, you already use a predicate lambda below... hence "all-out" :)
> 
> The code with min_element would look like this:
> 
>   auto min_it = std::min_element(
>       snippets_.begin(), snippets_.end(),
>       [](const scoped_ptr<NTPSnippet>& lhs, const scoped_ptr<NTPSnippet>& rhs)
{
>         return lhs->expiry_date() < rhs->expiry_date();
>       });
>   base::Time next_expiry = (*min_it)->expiry_date();
> 
> I prefer what's there now TBH, but if you insist...? :)

Eh, I don't think it's worse than what we currently do to expire the snippets
(calling a method with a lambda that returns an iterator, and then acting on
that iterator), but OTOH the code as currently written is pretty concise, and we
can't really do the expiration this way in an equally concises manner, so I
think this is fine.

[Marc Treib, 2016-03-01 15:14:12]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-03-01 15:14:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1743333002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1743333002/40001

[commit-bot: I haz the power, 2016-03-01 16:03:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-01 16:05:55]

Description was changed from

==========
[NTP Snippets] Implement snippets expiry

BUG=586134
==========

to

==========
[NTP Snippets] Implement snippets expiry

BUG=586134

Committed: https://crrev.com/3110b49ebf6342f75fefd539f50da36d8504c054
Cr-Commit-Position: refs/heads/master@{#378441}
==========

[commit-bot: I haz the power, 2016-03-01 16:05:56]

Patchset 3 (id:??) landed as
https://crrev.com/3110b49ebf6342f75fefd539f50da36d8504c054
Cr-Commit-Position: refs/heads/master@{#378441}