New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 631475 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Feature

Blocked on:
issue 631474

Blocking:
issue 629015



Sign in to add a comment

Implement the first version of bookmark suggestion provider for mobile NTP

Project Member Reported by jkrcal@chromium.org, Jul 26 2016

Issue description

Fetch the lastly visited bookmarks from the BookmarkModel and serve them to the ContentSuggestionService. 
 - The bookmarks should be sorted by the date of their last visit.
 - The list should be limited by two factors - max_length (defaults to ??) and max_age (defaults to 6 weeks). Both factors should be possible to configure via Finch.
 - The provider should store its own list of dismissed bookmark cards of the form <url, last_visit_date> so that
   => we can clear items from the list after |max_age| after |last_visit_date| elapses (and not care about them any more),
   => we can clear items from the list when visited after being dismissed (and show them in the UI again).
 

Comment 1 by jkrcal@chromium.org, Jul 26 2016

Labels: zine-pm
Patrick, 
 - what is the maximum number of bookmarks to show (assuming there are way too many visited in the last 6 weeks).
 - Do we stick to the 6 week time frame given we are not bound to local history cache any more?

Comment 2 by jkrcal@chromium.org, Jul 26 2016

Blocking: 629015

Comment 3 by nepper@chromium.org, Jul 27 2016

Labels: -zine-pm
Commented in the PRD:
- 6 weeks: yes, let's keep that
- max: 10
- limit to bookmarks opened on mobile

Please re-add zine-pm label in case you have follow up questions.

Comment 4 by jkrcal@chromium.org, Jul 29 2016

I have the first simple CL ready, not LGTMed, yet.
https://codereview.chromium.org/2190583002/

Comment 5 by fi...@chromium.org, Aug 4 2016

Labels: zine-triaged M-54
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2f529e9cad21a05815bf9be712c0e89212ded24c

commit 2f529e9cad21a05815bf9be712c0e89212ded24c
Author: pke <pke@google.com>
Date: Fri Aug 05 11:52:07 2016

Add bookmark provider for content suggestions.

This CL introduces a bookmark provider that lists the most recently
visited/created bookmarks. Only visits on the same device are
considered.

As an initial version, it does not support
 - dismissing of bookmarks and
 - configuring parameters (count, recency threshold) via Finch.

BUG= 631475 

Review-Url: https://codereview.chromium.org/2214683002
Cr-Commit-Position: refs/heads/master@{#410033}

[modify] https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c/chrome/app/generated_resources.grd
[modify] https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c/chrome/browser/about_flags.cc
[modify] https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
[modify] https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c/chrome/browser/ui/webui/snippets_internals_message_handler.cc
[modify] https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c/components/ntp_snippets.gypi
[modify] https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c/components/ntp_snippets/BUILD.gn
[add] https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc
[add] https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h
[modify] https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c/components/ntp_snippets/category.h
[modify] https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c/components/ntp_snippets/category_factory.cc
[modify] https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c/components/ntp_snippets/features.cc
[modify] https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c/components/ntp_snippets/features.h

Labels: zine-16-08-08
Almost done. Dismissing bookmark cards is in code review, I'll work on configuring parameters today.
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5ac4067d8f9f197f70cb5e15c62ffff8d5bc1e83

commit 5ac4067d8f9f197f70cb5e15c62ffff8d5bc1e83
Author: pke <pke@google.com>
Date: Tue Aug 09 14:41:21 2016

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

Review-Url: https://codereview.chromium.org/2222933002
Cr-Commit-Position: refs/heads/master@{#410667}

[modify] https://crrev.com/5ac4067d8f9f197f70cb5e15c62ffff8d5bc1e83/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc
[modify] https://crrev.com/5ac4067d8f9f197f70cb5e15c62ffff8d5bc1e83/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h
[modify] https://crrev.com/5ac4067d8f9f197f70cb5e15c62ffff8d5bc1e83/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc
[modify] https://crrev.com/5ac4067d8f9f197f70cb5e15c62ffff8d5bc1e83/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6fa8d8e9ed67273296a29654de3dbd5facdc1091

commit 6fa8d8e9ed67273296a29654de3dbd5facdc1091
Author: jkrcal <jkrcal@chromium.org>
Date: Thu Aug 11 16:58:47 2016

Update bookmark suggestions also after undismiss/delete.

The CL makes the bookmark suggestion provider react to more corner-case
events:
 - it updates the list if a relevant bookmark is undismissed (from
   chrome://snippets-internals)
 - it updates the list if a relevant bookmark is deleted.

The CL also slightly simplifies fetching recent bookmarks in
bookmark_last_visit_utils.cc.

BUG= 631475 

Review-Url: https://codereview.chromium.org/2241463002
Cr-Commit-Position: refs/heads/master@{#411361}

[modify] https://crrev.com/6fa8d8e9ed67273296a29654de3dbd5facdc1091/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc
[modify] https://crrev.com/6fa8d8e9ed67273296a29654de3dbd5facdc1091/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h
[modify] https://crrev.com/6fa8d8e9ed67273296a29654de3dbd5facdc1091/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc
[modify] https://crrev.com/6fa8d8e9ed67273296a29654de3dbd5facdc1091/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h

Status: Fixed (was: Assigned)

Sign in to add a comment