Do not select old bookmarks in ntp_snippets::BookmarkSuggestionsProvider

Do not select old bookmarks in ntp_snippets::BookmarkSuggestionsProvider

Previosly old boolmarks could be selected when there is not enough
recent ones. After this CL old bookmarks are ignored.

BUG=642734
Committed: https://crrev.com/9e34a1c3b6e2a94b5d335e388b0b6d7d7a38eaa9
Cr-Commit-Position: refs/heads/master@{#416212}

[vitaliii, 2016-09-01 09:24:22]

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

[vitaliii, 2016-09-01 09:24:23]

Hi Marc,

Could you have a look, please?

[Marc Treib, 2016-09-01 09:46:01]

https://codereview.chromium.org/2305583002/diff/1/components/ntp_snippets/boo...
File components/ntp_snippets/bookmarks/DEPS (right):

https://codereview.chromium.org/2305583002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/DEPS:3: "+components/bookmarks/test",
Might as well combine these two to components/bookmarks, so we don't have to
touch this again if we need some other file.

https://codereview.chromium.org/2305583002/diff/1/components/ntp_snippets/boo...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (left):

https://codereview.chromium.org/2305583002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:159:
bookmarks.push_back({most_recent, visited_recently});
We want to keep this, so that the creation_date_fallback behavior keeps working
(in case the PMs change their minds again and we need it after all).

https://codereview.chromium.org/2305583002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:187: // Insert
the first |max_count| items from |bookmarks| into |result|.
I think the problem is here: We're missing a check that the bookmark dates
aren't older than min_visit_time. I.e. break out of the loop if the current
bookmark is too old.

https://codereview.chromium.org/2305583002/diff/1/components/ntp_snippets/boo...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2305583002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:154: // There
aren't enough recently-visited bookmarks, do not show any.
This is not the behavior we want (sorry, I must have been unclear in our
discussions): We still want to show the recently-visited bookmarks that we have
in this case, even if it's only one.

[vitaliii, 2016-09-01 10:15:42]

Hi Marc,

would you mind having a look on my fix?

https://codereview.chromium.org/2305583002/diff/1/components/ntp_snippets/boo...
File components/ntp_snippets/bookmarks/DEPS (right):

https://codereview.chromium.org/2305583002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/DEPS:3: "+components/bookmarks/test",
On 2016/09/01 09:46:01, Marc Treib wrote:
> Might as well combine these two to components/bookmarks, so we don't have to
> touch this again if we need some other file.

Done.

https://codereview.chromium.org/2305583002/diff/1/components/ntp_snippets/boo...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (left):

https://codereview.chromium.org/2305583002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:159:
bookmarks.push_back({most_recent, visited_recently});
On 2016/09/01 09:46:01, Marc Treib wrote:
> We want to keep this, so that the creation_date_fallback behavior keeps
working
> (in case the PMs change their minds again and we need it after all).

Done.

https://codereview.chromium.org/2305583002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:187: // Insert
the first |max_count| items from |bookmarks| into |result|.
On 2016/09/01 09:46:01, Marc Treib wrote:
> I think the problem is here: We're missing a check that the bookmark dates
> aren't older than min_visit_time. I.e. break out of the loop if the current
> bookmark is too old.

Done.

https://codereview.chromium.org/2305583002/diff/1/components/ntp_snippets/boo...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2305583002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:154: // There
aren't enough recently-visited bookmarks, do not show any.
On 2016/09/01 09:46:01, Marc Treib wrote:
> This is not the behavior we want (sorry, I must have been unclear in our
> discussions): We still want to show the recently-visited bookmarks that we
have
> in this case, even if it's only one.

Done.

[Marc Treib, 2016-09-01 11:16:32]

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/BUILD.gn (right):

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/BUILD.gn:125: "//components/image_fetcher",
I think you need to add //components/bookmarks/browser and
//components/bookmarks/test here?

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:191:
GetLastVisitDateForBookmark(bookmark.node, creation_date_fallback))
I think this should be
if (!creation_date_fallback &&
    GetLastVisitDateForBookmark(...) < min_visit_time)

If the creation date fallback is enabled, we do want to show older bookmarks
too.

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:192: break;
nit: braces if the condition doesn't fit on one line

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc
(right):

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc:71:
base::Time threshold_time;
threshold_time_ (trailing underscore)

Also this should be private (or at least protected). If you make it private, you
could add a threshold_time() accessor.

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc:98:
GetRecentlyVisitedBookmarks(model.get(), number_of_recent - 1,
nit: make a max_count variable, so it's clear what this value means?

[vitaliii, 2016-09-01 11:38:43]

Hi Marc,

would you mind checking my new fix?

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/BUILD.gn (right):

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/BUILD.gn:125: "//components/image_fetcher",
On 2016/09/01 11:16:32, Marc Treib wrote:
> I think you need to add //components/bookmarks/browser and
> //components/bookmarks/test here?

Done.

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:191:
GetLastVisitDateForBookmark(bookmark.node, creation_date_fallback))
On 2016/09/01 11:16:32, Marc Treib wrote:
> I think this should be
> if (!creation_date_fallback &&
>     GetLastVisitDateForBookmark(...) < min_visit_time)
> 
> If the creation date fallback is enabled, we do want to show older bookmarks
> too.

Done.

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:192: break;
On 2016/09/01 11:16:32, Marc Treib wrote:
> nit: braces if the condition doesn't fit on one line

Done.

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc
(right):

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc:71:
base::Time threshold_time;
On 2016/09/01 11:16:32, Marc Treib wrote:
> threshold_time_ (trailing underscore)
> 
> Also this should be private (or at least protected). If you make it private,
you
> could add a threshold_time() accessor.

Done.

https://codereview.chromium.org/2305583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc:98:
GetRecentlyVisitedBookmarks(model.get(), number_of_recent - 1,
On 2016/09/01 11:16:32, Marc Treib wrote:
> nit: make a max_count variable, so it's clear what this value means?

Done.

[Marc Treib, 2016-09-01 11:41:43]

LGTM, thanks!
(You'll also need approval from a components/bookmarks owner for the new
dependency)

[vitaliii, 2016-09-01 11:48:24]

vitaliii@chromium.org changed reviewers:
+ sky@chromium.org

[vitaliii, 2016-09-01 11:48:24]

Hi Sky,

We use BookmarkModel and needed to create tests.
In order to create the model for tests we use TestBookmarkClient and that is why
we needed to change the dependency, which requires your approval. Would you mind
having a look?

[sky, 2016-09-01 18:31:14]

New DEPS LGTM

[vitaliii, 2016-09-02 06:55:46]

The CQ bit was checked by vitaliii@chromium.org

[commit-bot: I haz the power, 2016-09-02 06:55:57]

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, 2016-09-02 07:53:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-02 07:55:30]

Description was changed from

==========
Do not select old bookmarks in ntp_snippets::BookmarkSuggestionsProvider

Previosly old boolmarks could be selected when there is not enough
recent ones. After this CL old bookmarks are ignored.

BUG=642734
==========

to

==========
Do not select old bookmarks in ntp_snippets::BookmarkSuggestionsProvider

Previosly old boolmarks could be selected when there is not enough
recent ones. After this CL old bookmarks are ignored.

BUG=642734

Committed: https://crrev.com/9e34a1c3b6e2a94b5d335e388b0b6d7d7a38eaa9
Cr-Commit-Position: refs/heads/master@{#416212}
==========

[commit-bot: I haz the power, 2016-09-02 07:55:31]

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