[New CL] Add bookmark provider for content suggestions

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
Committed: https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c
Cr-Commit-Position: refs/heads/master@{#410033}

[Philipp Keck, 2016-08-04 13:12:45]

Description was changed from

==========
[New CL] Add bookmark provider for content suggestions

See https://codereview.chromium.org/2190583002#ps80001

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

to

==========
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
==========

[Philipp Keck, 2016-08-04 13:12:45]

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

[Philipp Keck, 2016-08-04 13:26:16]

pke@google.com changed reviewers:
+ bauerb@chromium.org

[Philipp Keck, 2016-08-04 13:26:17]

This is a copy of https://codereview.chromium.org/2190583002#ps80001 because
jkrcal is on holiday.

I rebased, moved the factory code into the new ContentSuggestionsServiceFactory,
moved the feature declaration to our new features file in components and adapted
the new provider to the new ContentSuggestionsProvider API.

PTAL!

[Marc Treib, 2016-08-04 13:57:14]

lgtm

https://codereview.chromium.org/2214683002/diff/20001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2214683002/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:39:
&ntp_snippets::kBookmarkSuggestionsFeature,
I don't think this needs to be here? Does any Java code query this feature?

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

https://codereview.chromium.org/2214683002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:71:
callback.Run(suggestion_id, gfx::Image());
Running the callback synchronously might be surprising to the caller - maybe we
should do base::ThreadTaskRunnerHandle::Get()->PostTask(...) ?

https://codereview.chromium.org/2214683002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:109:
FetchBookmarks();
We probably can't get here before the bookmark model is loaded? Then you could
call FetchBookmarksInternal directly.

[Philipp Keck, 2016-08-04 14:43:43]

Thank you

https://codereview.chromium.org/2214683002/diff/20001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2214683002/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:39:
&ntp_snippets::kBookmarkSuggestionsFeature,
On 2016/08/04 13:57:14, Marc Treib wrote:
> I don't think this needs to be here? Does any Java code query this feature?

Done.

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

https://codereview.chromium.org/2214683002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:71:
callback.Run(suggestion_id, gfx::Image());
On 2016/08/04 13:57:14, Marc Treib wrote:
> Running the callback synchronously might be surprising to the caller - maybe
we
> should do base::ThreadTaskRunnerHandle::Get()->PostTask(...) ?

So far, it has always worked fine. The ContentSuggestionsService also does that
when the provider doesn't exist. Put it on my list to think about it.

btw, NTPSnippetsStatusService also calls the OnDisabledReasonChanged when you
initialize it (I think).

https://codereview.chromium.org/2214683002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:109:
FetchBookmarks();
On 2016/08/04 13:57:14, Marc Treib wrote:
> We probably can't get here before the bookmark model is loaded? Then you could
> call FetchBookmarksInternal directly.

I don't know if the BookmarkModel guarantees that. The respective methods
https://codesearch.chromium.org/chromium/src/components/bookmarks/browser/boo...
don't really guard against |loaded_| being false.

[Marc Treib, 2016-08-04 14:46:27]

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

https://codereview.chromium.org/2214683002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:71:
callback.Run(suggestion_id, gfx::Image());
On 2016/08/04 14:43:42, Philipp Keck wrote:
> On 2016/08/04 13:57:14, Marc Treib wrote:
> > Running the callback synchronously might be surprising to the caller - maybe
> we
> > should do base::ThreadTaskRunnerHandle::Get()->PostTask(...) ?
> 
> So far, it has always worked fine. The ContentSuggestionsService also does
that
> when the provider doesn't exist. Put it on my list to think about it.
> 
> btw, NTPSnippetsStatusService also calls the OnDisabledReasonChanged when you
> initialize it (I think).

Acknowledged.

https://codereview.chromium.org/2214683002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:109:
FetchBookmarks();
On 2016/08/04 14:43:42, Philipp Keck wrote:
> On 2016/08/04 13:57:14, Marc Treib wrote:
> > We probably can't get here before the bookmark model is loaded? Then you
could
> > call FetchBookmarksInternal directly.
> 
> I don't know if the BookmarkModel guarantees that. The respective methods
>
https://codesearch.chromium.org/chromium/src/components/bookmarks/browser/boo...
> don't really guard against |loaded_| being false.

Alright. Carry on then I guess...

[Bernhard Bauer, 2016-08-04 18:10:55]

LGTM w/ some comments (sorry, didn't see those in my first review):

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

https://codereview.chromium.org/2214683002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:71:
callback.Run(suggestion_id, gfx::Image());
On 2016/08/04 14:46:26, Marc Treib wrote:
> On 2016/08/04 14:43:42, Philipp Keck wrote:
> > On 2016/08/04 13:57:14, Marc Treib wrote:
> > > Running the callback synchronously might be surprising to the caller -
maybe
> > we
> > > should do base::ThreadTaskRunnerHandle::Get()->PostTask(...) ?
> > 
> > So far, it has always worked fine. The ContentSuggestionsService also does
> that
> > when the provider doesn't exist. Put it on my list to think about it.
> > 
> > btw, NTPSnippetsStatusService also calls the OnDisabledReasonChanged when
you
> > initialize it (I think).
> 
> Acknowledged.

+1 for asynchronously running the callback. Reentrancy issues are very nasty.

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

https://codereview.chromium.org/2214683002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:19: namespace
{
Nit: empty line after this one, to be symmetric with the closing namespace.

[Philipp Keck, 2016-08-05 08:00:21]

Thank you for reviewing.

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

https://codereview.chromium.org/2214683002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:71:
callback.Run(suggestion_id, gfx::Image());
On 2016/08/04 18:10:55, Bernhard Bauer wrote:
> On 2016/08/04 14:46:26, Marc Treib wrote:
> > On 2016/08/04 14:43:42, Philipp Keck wrote:
> > > On 2016/08/04 13:57:14, Marc Treib wrote:
> > > > Running the callback synchronously might be surprising to the caller -
> maybe
> > > we
> > > > should do base::ThreadTaskRunnerHandle::Get()->PostTask(...) ?
> > > 
> > > So far, it has always worked fine. The ContentSuggestionsService also does
> > that
> > > when the provider doesn't exist. Put it on my list to think about it.
> > > 
> > > btw, NTPSnippetsStatusService also calls the OnDisabledReasonChanged when
> you
> > > initialize it (I think).
> > 
> > Acknowledged.
> 
> +1 for asynchronously running the callback. Reentrancy issues are very nasty.

Ok. CL coming up.

https://codereview.chromium.org/2214683002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:109:
FetchBookmarks();
On 2016/08/04 14:46:26, Marc Treib wrote:
> On 2016/08/04 14:43:42, Philipp Keck wrote:
> > On 2016/08/04 13:57:14, Marc Treib wrote:
> > > We probably can't get here before the bookmark model is loaded? Then you
> could
> > > call FetchBookmarksInternal directly.
> > 
> > I don't know if the BookmarkModel guarantees that. The respective methods
> >
>
https://codesearch.chromium.org/chromium/src/components/bookmarks/browser/boo...
> > don't really guard against |loaded_| being false.
> 
> Alright. Carry on then I guess...

Acknowledged.

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

https://codereview.chromium.org/2214683002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:19: namespace
{
On 2016/08/04 18:10:55, Bernhard Bauer wrote:
> Nit: empty line after this one, to be symmetric with the closing namespace.

Done.

[Philipp Keck, 2016-08-05 09:53:55]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-08-05 09:53:55]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2214683002/#ps60001
(title: "Insert a blank line")

[commit-bot: I haz the power, 2016-08-05 09:54:06]

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-05 11:06:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 11:06:41]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Philipp Keck, 2016-08-05 11:08:00]

The CQ bit was checked by pke@google.com

[commit-bot: I haz the power, 2016-08-05 11:08:24]

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-05 11:52:26]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-08-05 11:52:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-05 11:54:14]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c
Cr-Commit-Position: refs/heads/master@{#410033}
==========

[commit-bot: I haz the power, 2016-08-05 11:54:15]

Patchset 4 (id:??) landed as
https://crrev.com/2f529e9cad21a05815bf9be712c0e89212ded24c
Cr-Commit-Position: refs/heads/master@{#410033}