Make bookmark suggestions dismissable

Make bookmark suggestions dismissable

Add a dismissed_from_ntp flag to the meta info of a bookmark.
Set the flag for all bookmarks with the same URL when a bookmark
suggestion is dismissed from the NTP.
Only query bookmarks without the flags set when providing suggestions.
Implement ClearCachedSuggestionsForDebugging in the provider.

BUG=631475
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/5ac4067d8f9f197f70cb5e15c62ffff8d5bc1e83
Cr-Commit-Position: refs/heads/master@{#410667}

[Philipp Keck, 2016-08-08 08:52:18]

pke@google.com changed reviewers:
+ jkrcal@chromium.org, treib@chromium.org

[Philipp Keck, 2016-08-08 08:52:19]

PTAL
Not thoroughly tested yet.
Maybe we should rename bookmark_last_visit_utils?

[Marc Treib, 2016-08-08 09:10:46]

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

https://codereview.chromium.org/2222933002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:77: for (const
BookmarkNode* node : nodes) {
nit: braces around single-line bodies or not?
Either way is fine with me, but try to stay consistent within each file.

https://codereview.chromium.org/2222933002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:88:
dismissed_from_ntp == "1";
If the meta info exists at all, it should never have any other value than "1",
right? Maybe DCHECK that?

https://codereview.chromium.org/2222933002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:134: return
has_recent && has_non_dismissed;
This is the wrong way around - you want to remove the ones that *don't* have a
recent visit or that *are* dismissed.
Also I'm not quite sure about the "has_non_dismissed" - what should happen if
some, but not all of the nodes are dismissed?

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

https://codereview.chromium.org/2222933002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:39: bool
GetDismissedFromNTPForBookmark(const bookmarks::BookmarkNode* node);
IsBookmarkDismissedFromNTP?

[Philipp Keck, 2016-08-08 09:42:26]

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

https://codereview.chromium.org/2222933002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:77: for (const
BookmarkNode* node : nodes) {
On 2016/08/08 09:10:46, Marc Treib wrote:
> nit: braces around single-line bodies or not?
> Either way is fine with me, but try to stay consistent within each file.

Done.

https://codereview.chromium.org/2222933002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:88:
dismissed_from_ntp == "1";
On 2016/08/08 09:10:46, Marc Treib wrote:
> If the meta info exists at all, it should never have any other value than "1",
> right? Maybe DCHECK that?

Done.

https://codereview.chromium.org/2222933002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:134: return
has_recent && has_non_dismissed;
On 2016/08/08 09:10:46, Marc Treib wrote:
> This is the wrong way around - you want to remove the ones that *don't* have a
> recent visit or that *are* dismissed.

Right. I changed it around, trying to get rid of all the confusing double
negations.

> Also I'm not quite sure about the "has_non_dismissed" - what should happen if
> some, but not all of the nodes are dismissed?

If not all are dismissed, the following must have happened:
1. suggestion appears
2. user dismisses
3. all bookmarks for the URL are marked dismissed
4. the user adds (or syncs in) a new bookmark for the same URL.

I talked to Patrick about that case and we want to show a new suggestion for
that URL. So it will reappear because not all are dismissed, and that's the
intended behavior.

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

https://codereview.chromium.org/2222933002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h:39: bool
GetDismissedFromNTPForBookmark(const bookmarks::BookmarkNode* node);
On 2016/08/08 09:10:46, Marc Treib wrote:
> IsBookmarkDismissedFromNTP?

Done.

[Marc Treib, 2016-08-08 10:04:50]

lgtm

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

https://codereview.chromium.org/2222933002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:86:
DCHECK(!result || dismissed_from_ntp == "1");
nit: DCHECK_IF(result, dismissed_from_ntp == "1");

https://codereview.chromium.org/2222933002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:123: // Check if
there is any recent and/or non-dismissed one.
Just "and", not "and/or" (?)

https://codereview.chromium.org/2222933002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:124: bool
none_recently_visited = true;
There's still a negation here. Make this "has_recent_visit", and "return
!has_recent_visit || all_dismissed" ?

[Philipp Keck, 2016-08-08 10:15:21]

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

https://codereview.chromium.org/2222933002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:86:
DCHECK(!result || dismissed_from_ntp == "1");
On 2016/08/08 10:04:50, Marc Treib wrote:
> nit: DCHECK_IF(result, dismissed_from_ntp == "1");

Done.

https://codereview.chromium.org/2222933002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:123: // Check if
there is any recent and/or non-dismissed one.
On 2016/08/08 10:04:50, Marc Treib wrote:
> Just "and", not "and/or" (?)

Changed the comment into two parts, bc it doesn't need to be the same bookmark
that is non-dismissed and recently visited.

https://codereview.chromium.org/2222933002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:124: bool
none_recently_visited = true;
On 2016/08/08 10:04:50, Marc Treib wrote:
> There's still a negation here. Make this "has_recent_visit", and "return
> !has_recent_visit || all_dismissed" ?

Also good. Done.

[Philipp Keck, 2016-08-08 13:02:21]

Description was changed from

==========
Make bookmark suggestions dismissable

Add a dismissed_from_ntp flag to the meta info of a bookmark.
Set the flag for all bookmarks with the same URL when a bookmark
suggestion is dismissed from the NTP.
Only query bookmarks without the flags set when providing suggestions.
Implement ClearCachedSuggestionsForDebugging in the provider.

BUG=631475
==========

to

==========
Make bookmark suggestions dismissable

Add a dismissed_from_ntp flag to the meta info of a bookmark.
Set the flag for all bookmarks with the same URL when a bookmark
suggestion is dismissed from the NTP.
Only query bookmarks without the flags set when providing suggestions.
Implement ClearCachedSuggestionsForDebugging in the provider.

BUG=631475
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Philipp Keck, 2016-08-08 13:05:59]

On 2016/08/08 10:15:21, Philipp Keck wrote:
>
https://codereview.chromium.org/2222933002/diff/40001/components/ntp_snippets...
> File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):
> 
>
https://codereview.chromium.org/2222933002/diff/40001/components/ntp_snippets...
> components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:86:
> DCHECK(!result || dismissed_from_ntp == "1");
> On 2016/08/08 10:04:50, Marc Treib wrote:
> > nit: DCHECK_IF(result, dismissed_from_ntp == "1");
> 
> Done.
> 
>
https://codereview.chromium.org/2222933002/diff/40001/components/ntp_snippets...
> components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:123: // Check
if
> there is any recent and/or non-dismissed one.
> On 2016/08/08 10:04:50, Marc Treib wrote:
> > Just "and", not "and/or" (?)
> 
> Changed the comment into two parts, bc it doesn't need to be the same bookmark
> that is non-dismissed and recently visited.
> 
>
https://codereview.chromium.org/2222933002/diff/40001/components/ntp_snippets...
> components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:124: bool
> none_recently_visited = true;
> On 2016/08/08 10:04:50, Marc Treib wrote:
> > There's still a negation here. Make this "has_recent_visit", and "return
> > !has_recent_visit || all_dismissed" ?
> 
> Also good. Done.

To avoid conflicts, I rebased on another CL and implemented new functions.
Please review the diff between patch sets 4 and 7 (patch set 6 is rubbish).

[Marc Treib, 2016-08-08 14:03:59]

lgtm

https://codereview.chromium.org/2222933002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2222933002/diff/120001/components/ntp_snippet...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:158:
bookmarks::BookmarkModel* bookmark_model) {
"bookmarks::" not needed

https://codereview.chromium.org/2222933002/diff/120001/components/ntp_snippet...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:171: for (const
auto* node : bookmarks_for_url) {
Use the actual type?

https://codereview.chromium.org/2222933002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2222933002/diff/120001/components/ntp_snippet...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:96: for
(const BookmarkNode* bookmark : bookmarks) {
nit: braces not required

[Philipp Keck, 2016-08-08 14:20:11]

https://codereview.chromium.org/2222933002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2222933002/diff/120001/components/ntp_snippet...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:158:
bookmarks::BookmarkModel* bookmark_model) {
On 2016/08/08 14:03:59, Marc Treib wrote:
> "bookmarks::" not needed

Done.

https://codereview.chromium.org/2222933002/diff/120001/components/ntp_snippet...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:171: for (const
auto* node : bookmarks_for_url) {
On 2016/08/08 14:03:59, Marc Treib wrote:
> Use the actual type?

Done.

https://codereview.chromium.org/2222933002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2222933002/diff/120001/components/ntp_snippet...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:96: for
(const BookmarkNode* bookmark : bookmarks) {
On 2016/08/08 14:03:59, Marc Treib wrote:
> nit: braces not required

Done.

[Philipp Keck, 2016-08-09 14:04:34]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-08-09 14:04:35]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2222933002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-09 14:05:04]

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-08-09 14:41:40]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-09 14:45:15]

Description was changed from

==========
Make bookmark suggestions dismissable

Add a dismissed_from_ntp flag to the meta info of a bookmark.
Set the flag for all bookmarks with the same URL when a bookmark
suggestion is dismissed from the NTP.
Only query bookmarks without the flags set when providing suggestions.
Implement ClearCachedSuggestionsForDebugging in the provider.

BUG=631475
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Make bookmark suggestions dismissable

Add a dismissed_from_ntp flag to the meta info of a bookmark.
Set the flag for all bookmarks with the same URL when a bookmark
suggestion is dismissed from the NTP.
Only query bookmarks without the flags set when providing suggestions.
Implement ClearCachedSuggestionsForDebugging in the provider.

BUG=631475
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/5ac4067d8f9f197f70cb5e15c62ffff8d5bc1e83
Cr-Commit-Position: refs/heads/master@{#410667}
==========

[commit-bot: I haz the power, 2016-08-09 14:45:16]

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