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. On 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.

TBR=brettw@chromium.org
BUG=631475

[jkrcal, 2016-07-29 08:41:47]

Description was changed from

==========
Add bookmark provider for content suggestions [DO NOT SUBMIT]

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

to

==========
Add bookmark provider for content suggestions.

This CL introduces a bookmark provider that lists the most recently
visited/created bookmarks. On 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
==========

[jkrcal, 2016-07-29 08:49:28]

jkrcal@chromium.org changed reviewers:
+ bauerb@chromium.org, skuhne@chromium.org

[jkrcal, 2016-07-29 08:49:28]

Bernhard, PTAL!

Stefan, PTAL at chrome/browser/profiles/profile_manager.cc

[chromium-reviews, 2016-07-29 10:08:12]

+Marc, Philipp: can you coordinate with Jan to make sure this CL fits into
the recent category changes? If Jan wants to submit before his vacation
next week, then this is a FYI to migrate with the upcoming changes.

On Fri, Jul 29, 2016 at 10:49 AM, <jkrcal@chromium.org> wrote:

> Reviewers: Bernhard Bauer, Mr4D
> CL: https://codereview.chromium.org/2190583002/
>
> Message:
> Bernhard, PTAL!
>
> Stefan, PTAL at chrome/browser/profiles/profile_manager.cc
>
>
> Description:
> Add bookmark provider for content suggestions.
>
> This CL introduces a bookmark provider that lists the most recently
> visited/created bookmarks. On 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
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+380, -14 lines):
> M chrome/app/generated_resources.grd
> M chrome/browser/about_flags.cc
> M chrome/browser/android/chrome_feature_list.h
> M chrome/browser/android/chrome_feature_list.cc
> A + chrome/browser/android/ntp/bookmark_suggestions_provider_factory.h
> A chrome/browser/android/ntp/bookmark_suggestions_provider_factory.cc
> M chrome/browser/profiles/profile_manager.cc
> M chrome/browser/ui/webui/snippets_internals_message_handler.cc
> M chrome/chrome_browser.gypi
> M components/ntp_snippets.gypi
> M components/ntp_snippets/BUILD.gn
> A components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h
> A components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc
> M components/ntp_snippets/content_suggestions_category.h
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "New Tab Page development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to ntp-dev+unsubscribe@chromium.org.
> To post to this group, send email to ntp-dev@chromium.org.
> To view this discussion on the web visit
>
https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/001a1137428a0f51d105...
>
<https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/001a1137428a0f51d105...>
> .
>


Tim Schumann

Software Engineer

tschumann@google.com


Google Germany GmbH

Erika-Mann-Str. 33 Munich, 80636

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den Absender und
löschen Sie die E-Mail und alle Anhänge. Vielen Dank.


This e-mail is confidential. If you are not the right addressee please do
not forward it, please inform the sender, and please erase this e-mail
including any attachments. Thanks.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Marc Treib, 2016-07-29 10:08:34]

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

[Marc Treib, 2016-07-29 10:08:35]

FYI: Some parts of this will conflict with pke's
https://codereview.chromium.org/2187233002/

+pke FYI

Also some drive-by comments :)

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

https://codereview.chromium.org/2190583002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:70: //
TODO(jkrcal): Make sure no image is displayed in the UI.
Might be nice to call the callback with a "null" image?

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

https://codereview.chromium.org/2190583002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h:22: using
bookmarks::BookmarkNode;
No using in headers

[Bernhard Bauer, 2016-07-29 10:28:51]

https://codereview.chromium.org/2190583002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2190583002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1235:
BookmarkSuggestionsProviderFactory::GetForProfile(profile);
Can we please not add any more initialization code here? Contrary to what seems
to be public opinion, this is not the place where every single profile-keyed
service should be initialized. By default, profile-keyed services are lazily
created when needed. If there is something that needs to happen at startup (like
scheduling the fetching task), the factory can declare that it wants to be
created with the profile. If we want to only add some providers under certain
conditions, we can move the provider registration code e.g. to the
ContentSuggestionsFactory, which would have the advantage that it would
centralize it.

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

https://codereview.chromium.org/2190583002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h:81: void
FetchBookmarksWhenLoaded();
This method is meant to be called when the bookmark model has been loaded,
right? Right now it reads more like it's qualifying the imperative (fetch
bookmarks when the model has been loaded, i.e. what the _current_
FetchBookmarks() does). Maybe FetchBookmarksAfterLoading()? (Or just
FetchBookmarksInternal() and add a comment about the precondition, and maybe a
DCHECK for good measure?)

[jkrcal, 2016-07-29 13:57:17]

Thanks, PTAL again!
(mixed with a rebase of the underlying CL, I am sorry about it.)

https://codereview.chromium.org/2190583002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2190583002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1235:
BookmarkSuggestionsProviderFactory::GetForProfile(profile);
On 2016/07/29 10:28:51, Bernhard Bauer wrote:
> Can we please not add any more initialization code here? Contrary to what
seems
> to be public opinion, this is not the place where every single profile-keyed
> service should be initialized. By default, profile-keyed services are lazily
> created when needed. If there is something that needs to happen at startup
(like
> scheduling the fetching task), the factory can declare that it wants to be
> created with the profile. If we want to only add some providers under certain
> conditions, we can move the provider registration code e.g. to the
> ContentSuggestionsFactory, which would have the advantage that it would
> centralize it.

Oh, did not know. Philipp is working on a CL that will move the
ContentSuggestionsServiceFactory to the components code. Maybe he can move both
the initialization lines away with that CL?

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

https://codereview.chromium.org/2190583002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:70: //
TODO(jkrcal): Make sure no image is displayed in the UI.
On 2016/07/29 10:08:34, Marc Treib wrote:
> Might be nice to call the callback with a "null" image?

Done.

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

https://codereview.chromium.org/2190583002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h:22: using
bookmarks::BookmarkNode;
On 2016/07/29 10:08:35, Marc Treib wrote:
> No using in headers

Done.

https://codereview.chromium.org/2190583002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h:81: void
FetchBookmarksWhenLoaded();
On 2016/07/29 10:28:51, Bernhard Bauer wrote:
> This method is meant to be called when the bookmark model has been loaded,
> right? Right now it reads more like it's qualifying the imperative (fetch
> bookmarks when the model has been loaded, i.e. what the _current_
> FetchBookmarks() does). Maybe FetchBookmarksAfterLoading()? (Or just
> FetchBookmarksInternal() and add a comment about the precondition, and maybe a
> DCHECK for good measure?)

Good point, done.

[jkrcal, 2016-07-29 14:01:40]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 14:01:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bernhard Bauer, 2016-07-29 14:32:42]

https://codereview.chromium.org/2190583002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2190583002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1235:
BookmarkSuggestionsProviderFactory::GetForProfile(profile);
On 2016/07/29 13:57:17, jkrcal wrote:
> On 2016/07/29 10:28:51, Bernhard Bauer wrote:
> > Can we please not add any more initialization code here? Contrary to what
> seems
> > to be public opinion, this is not the place where every single profile-keyed
> > service should be initialized. By default, profile-keyed services are lazily
> > created when needed. If there is something that needs to happen at startup
> (like
> > scheduling the fetching task), the factory can declare that it wants to be
> > created with the profile. If we want to only add some providers under
certain
> > conditions, we can move the provider registration code e.g. to the
> > ContentSuggestionsFactory, which would have the advantage that it would
> > centralize it.
> 
> Oh, did not know. Philipp is working on a CL that will move the
> ContentSuggestionsServiceFactory to the components code. Maybe he can move
both
> the initialization lines away with that CL?

You mean all four? :)

If you absolutely can't get rid of the initialization in this CL, can you add a
TODO to remove them, and a comment that people should not add any more
initialization here?

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

https://codereview.chromium.org/2190583002/diff/60001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:73:
gfx::Image image;
Just inline this.

[commit-bot: I haz the power, 2016-07-29 15:04:07]

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

[commit-bot: I haz the power, 2016-07-29 15:04:08]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2016-07-29 16:09:56]

Thanks. PTAL, again.

https://codereview.chromium.org/2190583002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2190583002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1235:
BookmarkSuggestionsProviderFactory::GetForProfile(profile);
On 2016/07/29 14:32:42, Bernhard Bauer wrote:
> On 2016/07/29 13:57:17, jkrcal wrote:
> > On 2016/07/29 10:28:51, Bernhard Bauer wrote:
> > > Can we please not add any more initialization code here? Contrary to what
> > seems
> > > to be public opinion, this is not the place where every single
profile-keyed
> > > service should be initialized. By default, profile-keyed services are
lazily
> > > created when needed. If there is something that needs to happen at startup
> > (like
> > > scheduling the fetching task), the factory can declare that it wants to be
> > > created with the profile. If we want to only add some providers under
> certain
> > > conditions, we can move the provider registration code e.g. to the
> > > ContentSuggestionsFactory, which would have the advantage that it would
> > > centralize it.
> > 
> > Oh, did not know. Philipp is working on a CL that will move the
> > ContentSuggestionsServiceFactory to the components code. Maybe he can move
> both
> > the initialization lines away with that CL?
> 
> You mean all four? :)
> 
> If you absolutely can't get rid of the initialization in this CL, can you add
a
> TODO to remove them, and a comment that people should not add any more
> initialization here?

I am leaving for vacation. Did not manage to finish it. I added this TODO
comment for the time being.

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

https://codereview.chromium.org/2190583002/diff/60001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:73:
gfx::Image image;
On 2016/07/29 14:32:42, Bernhard Bauer wrote:
> Just inline this.

Done.

[Bernhard Bauer, 2016-08-01 09:09:15]

lgtm

[Mr4D (OOO till 08-26), 2016-08-01 15:53:27]

chrome/browser/profiles/profile_manager.cc

lgtm

[Bernhard Bauer, 2016-08-01 16:02:18]

Description was changed from

==========
Add bookmark provider for content suggestions.

This CL introduces a bookmark provider that lists the most recently
visited/created bookmarks. On 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. On 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.

TBR=brettw@chromium.org
BUG=631475
==========

[Bernhard Bauer, 2016-08-01 16:02:46]

Sending to CQ in Jan's absence.

[Bernhard Bauer, 2016-08-01 16:02:53]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2016-08-01 16:03:02]

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

[commit-bot: I haz the power, 2016-08-01 16:03:03]

This CL has an open dependency (Issue 2184263005 Patch 40001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Philipp Keck, 2016-08-04 13:27:22]

On 2016/08/01 16:03:03, commit-bot: I haz the power wrote:
> This CL has an open dependency (Issue 2184263005 Patch 40001). Please resolve
> the dependency and try again.
> If you are sure that there is no real dependency, please use one of the
options
> listed in https://goo.gl/9Es4OR to land the CL.

I'm taking over this CL, the new CL is here:
https://codereview.chromium.org/2214683002/

Mr4D, thank you for reviewing this one. You won't have to look at the new one
because we refactored our factories, so we now don't have to touch
profile_manager.cc anymore.