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

Issue 649009 link

Starred by 3 users
Status: Fixed
Owner:
█ tschumann@chromium.org
Last visit > 30 days ago
Closed: Oct 2016
Cc:
█ ramyan@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Feature



Sign in to add a comment

Replace suggestions by new ones instead of merging.

Project Member Reported by jkrcal@chromium.org, Sep 21 2016 
Currently, the provider of remote content suggestions uses a complicated logic to merge suggestions across fetches and to expire them.

In order to simplify things and let the server always decide, we should implement the following:
 - new (non-empty) list of suggestions for a category always replaces the previous content of the category,
 - we never expire current suggestions, only dismissed suggestions are removed after expiry date (we do not need a timer, this can happen before we use dismissed suggestions for filtering newly fetched suggestions),
 - we keep an archive of older suggestions in memory to support previously opened NTPs (these are never stored to DB and thus cleared upon restart; we trim the list after 200 suggestions to address users that never close Chrome),
 - upon start, we clear orphaned thumbnails in the DB (for which there is no suggestion in the DB).

Comment 1 by fi...@chromium.org, Sep 22 2016

Labels: zine-triaged
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 23 2016 

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

commit 928ed3fef6431a8815e32bb2d905fc3bd21f5024
Author: jkrcal <jkrcal@chromium.org>
Date: Fri Sep 23 18:47:06 2016

New snippets now replace old snippets and do not merge

Previously, the provider of remote content suggestions used a
complicated logic to merge suggestions across fetches and to expire
them.

After this CL,
 - new (non-empty) list of suggestions for a category always replaces
   the previous content of the category,
 - we never expire current suggestions, only dismissed suggestions are
   removed after expiry date (we do not need a timer, this can happen
   before we use dismissed suggestions for filtering newly fetched
   suggestions),
 - we keep an archive of older suggestions in memory to support
   previously opened NTPs (these are never stored to DB and thus
   cleared upon restart; we trim the list after 200 suggestions to
   address users that never close Chrome).

This CL does not deal with clearing orphaned suggestion thumbnails
from the DB which is necessary for the current design. This is left
for a follow-up CL.

As additional technical steps:
 - The suggestions->category map was removed from the
   ContentSuggestionService. This blocked image fetching queries
   for archived suggestions to get to the provider (as the service
   does not know about the archive). We now rely on the category
   being encoded into the suggestion_id string. To this end the CL
   moved the functions that build and parse suggestion_id strings
   into CategoryFactory to be accessible both from the service and
   the providers.

BUG= 649009 

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

[modify] https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024/components/ntp_snippets/category_factory.cc
[modify] https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024/components/ntp_snippets/category_factory.h
[modify] https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024/components/ntp_snippets/content_suggestions_provider.cc
[modify] https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024/components/ntp_snippets/content_suggestions_service.cc
[modify] https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024/components/ntp_snippets/content_suggestions_service.h
[modify] https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024/components/ntp_snippets/content_suggestions_service_unittest.cc
[modify] https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024/components/ntp_snippets/ntp_snippets_database.cc
[modify] https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024/components/ntp_snippets/ntp_snippets_database.h
[modify] https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024/components/ntp_snippets/ntp_snippets_database_unittest.cc
[modify] https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024/components/ntp_snippets/ntp_snippets_service.cc
[modify] https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024/components/ntp_snippets/ntp_snippets_service.h
[modify] https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024/components/ntp_snippets/ntp_snippets_service_unittest.cc

Comment 3 by tschumann@chromium.org, Sep 30 2016

Owner: tschumann@chromium.org
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 4 2016 

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

commit 108e9c223a70d6dc4604354ab5afb0b42b28d30f
Author: tschumann <tschumann@chromium.org>
Date: Tue Oct 04 13:34:57 2016

Extended the ProtoDatabase to provide LoadKeys() functionality.

Before this change, the only way to scan all keys was to call LoadEntries() which loads the whole database. In our use case, this means copying a lot of image data which is wasteful.
With LoadKeys(), we still need to iterate the whole database but we don't have to copy out (and parse) the values -- just the keys. LoadKeys() is useful when storing relatively large values and only access to the keys is required.

We'll use LoadKeys() for NTP content suggestions to garbage collect data. We do that at times when we have a list of all still alive elements and need to intersect that with the elements stored in the db.

BUG= 649009 

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

[modify] https://crrev.com/108e9c223a70d6dc4604354ab5afb0b42b28d30f/components/leveldb_proto/leveldb_database.cc
[modify] https://crrev.com/108e9c223a70d6dc4604354ab5afb0b42b28d30f/components/leveldb_proto/leveldb_database.h
[modify] https://crrev.com/108e9c223a70d6dc4604354ab5afb0b42b28d30f/components/leveldb_proto/proto_database.h
[modify] https://crrev.com/108e9c223a70d6dc4604354ab5afb0b42b28d30f/components/leveldb_proto/proto_database_impl.h
[modify] https://crrev.com/108e9c223a70d6dc4604354ab5afb0b42b28d30f/components/leveldb_proto/proto_database_impl_unittest.cc
[modify] https://crrev.com/108e9c223a70d6dc4604354ab5afb0b42b28d30f/components/leveldb_proto/testing/fake_db.h

Comment 6 by treib@chromium.org, Oct 17 2016

Status: Fixed (was: Started)

Comment 7 by treib@chromium.org, Oct 17 2016 

Issue 642667 has been merged into this issue.

Comment 9 by dimu@google.com, Nov 4 2016

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840
►

Sign in to add a comment