Refactor and extend SnippetsBridge for multi-section support

Refactor and extend SnippetsBridge for multi-section support

Add a category parameter to ContentSuggestionsService::Observer
::OnNewSuggestions. Add getters and observer events equivalent to the
ContentSuggestionsService to the SnippetsBridge. Adjust the UI layer to
use the new methods on the bridge. Modify the NewTabPageAdapterTest to
correctly mock the new bridge.

BUG=635794
Committed: https://crrev.com/222d8a5248288b4bd3f059e25951dfbeb1f5af62
Cr-Commit-Position: refs/heads/master@{#411027}

[Philipp Keck, 2016-08-09 13:01:52]

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

[Philipp Keck, 2016-08-09 13:01:52]

PTAL

[Bernhard Bauer, 2016-08-09 13:10:41]

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:266:
if (info == null) return;
Similar to https://codereview.chromium.org/2207493002/, do we actually expect
this to happen?

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:23:
interface ContentSuggestionsSourceObserver {
Do we need to repeat the class name in this interface? If we just call it
Observer, we can still refer to it as ContentSuggestionsSource.Observer to get
the same information, but we can also shorten it.

[Marc Treib, 2016-08-09 13:38:16]

https://codereview.chromium.org/2230473002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (left):

https://codereview.chromium.org/2230473002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:96: OnNewSuggestions();
Why this change? Does the UI now explicitly query everything on startup?

https://codereview.chromium.org/2230473002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:106: return
ToJavaIntArray(env, reinterpret_cast<int*>(categories.data()),
Ooh, I'm not sure this is safe. Rule of thumb, reinterpret_cast is the Wrong
Thing.
Probably better to explicitly make a vector<int> which we can then convert
normally.

https://codereview.chromium.org/2230473002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:185:
base::Bind(&SnippetVisitedHistoryRequestCallback, callback),
nit: Rename to match the method

https://codereview.chromium.org/2230473002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.h (right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.h:66: void GetSuggestionVisited(
Call this GetUrlVisited? It doesn't really have anything to do with a
suggestion. (The other methods talking about suggestions take a suggestion_id,
while this takes a URL)

[Michael van Ouwerkerk, 2016-08-09 13:52:19]

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

[Michael van Ouwerkerk, 2016-08-09 13:52:21]

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/new_tab_page_snippets_header.xml (right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_snippets_header.xml:8:
android:id="@+id/suggestions_section_header_text"
nit: omit the _text suffix, it seems redundant

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:48:
private ContentSuggestionsSource mSnippetsSource;
Please also update the variable name to e.g. mSuggestionsSource

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:122:
ContentSuggestionsSource snippetsSource, UiConfig uiConfig) {
Also update the param name here and in the docs above to suggestionsSource

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:128:
for (int category : mSnippetsSource.getCategories()) {
If there are no categories, this means the above the fold section is also not
shown, as setSuggestions is never called. You'll need to separate building up
the suggestion section items from the rest (mAboveTheFold and mBottomSpacer).

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:266:
if (info == null) return;
Returning here is not appropriate, considering what the rest of the method does.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:19:
public interface ContentSuggestionsSource {
Could this be shortened to SuggestionsSource? Other classes like
SuggestionsCategoryInfo and SuggestionsSection also omit the Content prefix.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:25:
void onNewSuggestions(@KnownCategoriesEnum int category);
Is it always a known category? Then use the annotation in the adapter, otherwise
remove it here.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:29:
@KnownCategoriesEnum int category, @CategoryStatusEnum int newStatus);
Same thing for this category, is it a known one?

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:58:
void fetchSuggestionImage(SnippetArticleListItem suggestion, Callback<Bitmap>
callback);
Is the callback guaranteed to never be called synchronously? Please document
that.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:68:
void getSuggestionVisited(SnippetArticleListItem suggestion, Callback<Boolean>
callback);
Is the callback guaranteed to never be called synchronously? Please document
that.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:71:
private final ContentSuggestionsSource mSnippetsSource;
Please update the variable name accordingly.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:93:
ContentSuggestionsSource snippetsSource, UiConfig uiConfig) {
Please update the param name accordingly, here and in the javadoc.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:30:
private SnippetHeaderListItem mHeader;
Let's disambiguate this one from mHeaderTextView by renaming it to
mHeaderListItem.

[Philipp Keck, 2016-08-09 14:02:54]

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:266:
if (info == null) return;
On 2016/08/09 13:10:41, Bernhard Bauer wrote:
> Similar to https://codereview.chromium.org/2207493002/, do we actually expect
> this to happen?

True. I removed the check. If it should ever be null, SuggestionsSection.java:30
will fail with an NPE that clearly points at the problem.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:23:
interface ContentSuggestionsSourceObserver {
On 2016/08/09 13:10:41, Bernhard Bauer wrote:
> Do we need to repeat the class name in this interface? If we just call it
> Observer, we can still refer to it as ContentSuggestionsSource.Observer to get
> the same information, but we can also shorten it.

Done.

https://codereview.chromium.org/2230473002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (left):

https://codereview.chromium.org/2230473002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:96: OnNewSuggestions();
On 2016/08/09 13:38:16, Marc Treib wrote:
> Why this change? Does the UI now explicitly query everything on startup?

Correct:
https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...

https://codereview.chromium.org/2230473002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:106: return
ToJavaIntArray(env, reinterpret_cast<int*>(categories.data()),
On 2016/08/09 13:38:16, Marc Treib wrote:
> Ooh, I'm not sure this is safe. Rule of thumb, reinterpret_cast is the Wrong
> Thing.
> Probably better to explicitly make a vector<int> which we can then convert
> normally.

Done.

https://codereview.chromium.org/2230473002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:185:
base::Bind(&SnippetVisitedHistoryRequestCallback, callback),
On 2016/08/09 13:38:16, Marc Treib wrote:
> nit: Rename to match the method

Done.

https://codereview.chromium.org/2230473002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.h (right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.h:66: void GetSuggestionVisited(
On 2016/08/09 13:38:16, Marc Treib wrote:
> Call this GetUrlVisited? It doesn't really have anything to do with a
> suggestion. (The other methods talking about suggestions take a suggestion_id,
> while this takes a URL)

Done.

[PEConn, 2016-08-09 14:14:50]

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

[PEConn, 2016-08-09 14:14:51]

https://codereview.chromium.org/2230473002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2230473002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:152:
mSnippetsSource.getSuggestionsForCategory(category);
Does this change the behaviour at all?

Previously it looks like onSuggestionsReceived took a list of (what I assume)
would be brand new suggestions. Here you are told there are new suggestions and
then you go and get them all.

Are we sure that whenever onNewSuggestions is called, all the old suggestions
are wiped?

[Bernhard Bauer, 2016-08-09 14:30:12]

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:266:
if (info == null) return;
On 2016/08/09 14:02:53, Philipp Keck wrote:
> On 2016/08/09 13:10:41, Bernhard Bauer wrote:
> > Similar to https://codereview.chromium.org/2207493002/, do we actually
expect
> > this to happen?
> 
> True. I removed the check. If it should ever be null,
SuggestionsSection.java:30
> will fail with an NPE that clearly points at the problem.

Exactly. Thanks!

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:25:
void onNewSuggestions(@KnownCategoriesEnum int category);
On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> Is it always a known category? Then use the annotation in the adapter,
otherwise
> remove it here.

This was a leftover from my CL. There could be server-defined categories (which
are not known at compile time), but I liked having an annotation. Really, we
could make an (otherwise empty) @Category annotation that would just serve as
documentation.

https://codereview.chromium.org/2230473002/diff/100001/chrome/browser/android...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2230473002/diff/100001/chrome/browser/android...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:104: std::vector<int>
categoryIds;
Nit: C++ local variables are underscore_style (Yeah, it's confusing).

[Marc Treib, 2016-08-09 14:42:57]

C++ parts LGTM with the remaining comments addressed (I didn't look at the Java
parts)

https://codereview.chromium.org/2230473002/diff/120001/chrome/browser/android...
File chrome/browser/android/ntp/ntp_snippets_bridge.h (right):

https://codereview.chromium.org/2230473002/diff/120001/chrome/browser/android...
chrome/browser/android/ntp/ntp_snippets_bridge.h:65: // Checks if the URL has
been visited. The callback must not be called
s/must/will/
You're talking to whoever calls this method.

https://codereview.chromium.org/2230473002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2230473002/diff/120001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_provider.h:93: // fails, the
callback gets a null image. The callback must not be called
Same here

https://codereview.chromium.org/2230473002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2230473002/diff/120001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_service.h:93: // the callback gets
an empty image. The callback must not be called
and here

[Philipp Keck, 2016-08-09 16:15:07]

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/new_tab_page_snippets_header.xml (right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_snippets_header.xml:8:
android:id="@+id/suggestions_section_header_text"
On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> nit: omit the _text suffix, it seems redundant

Done.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:48:
private ContentSuggestionsSource mSnippetsSource;
On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> Please also update the variable name to e.g. mSuggestionsSource

Done.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:122:
ContentSuggestionsSource snippetsSource, UiConfig uiConfig) {
On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> Also update the param name here and in the docs above to suggestionsSource

Done.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:128:
for (int category : mSnippetsSource.getCategories()) {
On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> If there are no categories, this means the above the fold section is also not
> shown, as setSuggestions is never called. You'll need to separate building up
> the suggestion section items from the rest (mAboveTheFold and mBottomSpacer).

Moved code from setSuggestions to a new method "updateGroups()". This loop stays
as is, but the updateGroups() is always called afterwards to initialize the UI.
I tested what happens if there are no sections (see also
https://bugs.chromium.org/p/chromium/issues/detail?id=629817). And it doesn't
add the bottom spacer anymore if there are no sections. This will result in an
empty Zine. To test this, you can make SnippetsBridge#getCategories() return an
empty array. The NTP has no below-the-fold and is not scrollable on my tablet.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:266:
if (info == null) return;
On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> Returning here is not appropriate, considering what the rest of the method
does.

Acknowledged.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:19:
public interface ContentSuggestionsSource {
On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> Could this be shortened to SuggestionsSource? Other classes like
> SuggestionsCategoryInfo and SuggestionsSection also omit the Content prefix.

Done.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:25:
void onNewSuggestions(@KnownCategoriesEnum int category);
On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> Is it always a known category? Then use the annotation in the adapter,
otherwise
> remove it here.

Done. Good catch.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:29:
@KnownCategoriesEnum int category, @CategoryStatusEnum int newStatus);
On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> Same thing for this category, is it a known one?

Done.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:58:
void fetchSuggestionImage(SnippetArticleListItem suggestion, Callback<Bitmap>
callback);
On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> Is the callback guaranteed to never be called synchronously? Please document
> that.

Done.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:68:
void getSuggestionVisited(SnippetArticleListItem suggestion, Callback<Boolean>
callback);
On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> Is the callback guaranteed to never be called synchronously? Please document
> that.

Done.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:71:
private final ContentSuggestionsSource mSnippetsSource;
On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> Please update the variable name accordingly.

Done.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:93:
ContentSuggestionsSource snippetsSource, UiConfig uiConfig) {
On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> Please update the param name accordingly, here and in the javadoc.

Done.

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:30:
private SnippetHeaderListItem mHeader;
On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> Let's disambiguate this one from mHeaderTextView by renaming it to
> mHeaderListItem.

Done.

[Philipp Keck, 2016-08-09 16:59:16]

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:25:
void onNewSuggestions(@KnownCategoriesEnum int category);
On 2016/08/09 14:30:11, Bernhard Bauer wrote:
> On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> > Is it always a known category? Then use the annotation in the adapter,
> otherwise
> > remove it here.
> 
> This was a leftover from my CL. There could be server-defined categories
(which
> are not known at compile time), but I liked having an annotation. Really, we
> could make an (otherwise empty) @Category annotation that would just serve as
> documentation.

You guys can add such a @Category annotation. The @KnownCategoriesEnum needs to
go from here, we want to support dynamic sections in M54 and the corresponding
server-provider will land soon.

https://codereview.chromium.org/2230473002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2230473002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:152:
mSnippetsSource.getSuggestionsForCategory(category);
On 2016/08/09 14:14:51, PEConn wrote:
> Does this change the behaviour at all?
> 
> Previously it looks like onSuggestionsReceived took a list of (what I assume)
> would be brand new suggestions. Here you are told there are new suggestions
and
> then you go and get them all.
> 
> Are we sure that whenever onNewSuggestions is called, all the old suggestions
> are wiped?

No wiping. Line 142 cancels any update if there were suggestions previously. We
don't want to update, we only want to fill previously empty sections because
they were loading in the background.

https://codereview.chromium.org/2230473002/diff/100001/chrome/browser/android...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2230473002/diff/100001/chrome/browser/android...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:104: std::vector<int>
categoryIds;
On 2016/08/09 14:30:12, Bernhard Bauer wrote:
> Nit: C++ local variables are underscore_style (Yeah, it's confusing).

Done.

https://codereview.chromium.org/2230473002/diff/120001/chrome/browser/android...
File chrome/browser/android/ntp/ntp_snippets_bridge.h (right):

https://codereview.chromium.org/2230473002/diff/120001/chrome/browser/android...
chrome/browser/android/ntp/ntp_snippets_bridge.h:65: // Checks if the URL has
been visited. The callback must not be called
On 2016/08/09 14:42:57, Marc Treib wrote:
> s/must/will/
> You're talking to whoever calls this method.

Done.

https://codereview.chromium.org/2230473002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2230473002/diff/120001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_provider.h:93: // fails, the
callback gets a null image. The callback must not be called
On 2016/08/09 14:42:57, Marc Treib wrote:
> Same here

Done.

https://codereview.chromium.org/2230473002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2230473002/diff/120001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_service.h:93: // the callback gets
an empty image. The callback must not be called
On 2016/08/09 14:42:57, Marc Treib wrote:
> and here

Done.

[Bernhard Bauer, 2016-08-10 09:01:52]

lgtm

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:25:
void onNewSuggestions(@KnownCategoriesEnum int category);
On 2016/08/09 16:59:16, Philipp Keck wrote:
> On 2016/08/09 14:30:11, Bernhard Bauer wrote:
> > On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> > > Is it always a known category? Then use the annotation in the adapter,
> > otherwise
> > > remove it here.
> > 
> > This was a leftover from my CL. There could be server-defined categories
> (which
> > are not known at compile time), but I liked having an annotation. Really, we
> > could make an (otherwise empty) @Category annotation that would just serve
as
> > documentation.
> 
> You guys can add such a @Category annotation. The @KnownCategoriesEnum needs
to
> go from here, we want to support dynamic sections in M54 and the corresponding
> server-provider will land soon.

Keep in mind that these annotations are only used in linting, i.e. nothing will
break at runtime if we have a value that is not a known category.

[Philipp Keck, 2016-08-10 09:05:09]

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:25:
void onNewSuggestions(@KnownCategoriesEnum int category);
On 2016/08/10 09:01:52, Bernhard Bauer wrote:
> On 2016/08/09 16:59:16, Philipp Keck wrote:
> > On 2016/08/09 14:30:11, Bernhard Bauer wrote:
> > > On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> > > > Is it always a known category? Then use the annotation in the adapter,
> > > otherwise
> > > > remove it here.
> > > 
> > > This was a leftover from my CL. There could be server-defined categories
> > (which
> > > are not known at compile time), but I liked having an annotation. Really,
we
> > > could make an (otherwise empty) @Category annotation that would just serve
> as
> > > documentation.
> > 
> > You guys can add such a @Category annotation. The @KnownCategoriesEnum needs
> to
> > go from here, we want to support dynamic sections in M54 and the
corresponding
> > server-provider will land soon.
> 
> Keep in mind that these annotations are only used in linting, i.e. nothing
will
> break at runtime if we have a value that is not a known category.

Ok. Didn't know that. Would it break though if we had unit tests that test want
to test for dynamic categories? Or does the linter ignore the test files?
Still, I think that "known category" would be the wrong documentation signal to
the reader, because it's actually "any category", whether it's known or not.

[Michael van Ouwerkerk, 2016-08-10 09:24:40]

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:250:
if (firstHeaderPosition == RecyclerView.NO_POSITION) return
RecyclerView.NO_POSITION;
This retains the assumption that there is only a card if there is a header, and
it must follow the header. Could we explicitly look for a card instead, instead
of relying on getFirstHeaderPosition? That was just a convenient shortcut, shown
to be brittle.

You can leave a TODO if you feel this change doesn't fit in this CL.

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:255:
int bottomSpacerPosition = getBottomSpacerPosition();
Same for this one, we probably should not rely on getBottomSpacerPosition here.

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java
(right):

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java:34:
public int getCardLayout() {
@ContentSuggestionsCardLayoutEnum

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
(right):

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:32:
mMoreButton = null; // TODO(pke): Read from info => ? new ActionListItem() :
null;
What happened here? It looks like the more button was removed, it's always null
now.

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:89:
* @param suggestionsSource The SnippetsBridge used to retrieve the thumbnails.
It's not exactly of type SnippetsBridge now :-) How about "The source used to
retrieve the thumbnails."

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java
(right):

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:27:
private final TextView mHeaderListItemTextView;
nit: I would prefer something shorter without ListItem in it as it's not one of
those. Maybe mHeaderView?

[Bernhard Bauer, 2016-08-10 09:39:11]

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:25:
void onNewSuggestions(@KnownCategoriesEnum int category);
On 2016/08/10 09:05:09, Philipp Keck wrote:
> On 2016/08/10 09:01:52, Bernhard Bauer wrote:
> > On 2016/08/09 16:59:16, Philipp Keck wrote:
> > > On 2016/08/09 14:30:11, Bernhard Bauer wrote:
> > > > On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> > > > > Is it always a known category? Then use the annotation in the adapter,
> > > > otherwise
> > > > > remove it here.
> > > > 
> > > > This was a leftover from my CL. There could be server-defined categories
> > > (which
> > > > are not known at compile time), but I liked having an annotation.
Really,
> we
> > > > could make an (otherwise empty) @Category annotation that would just
serve
> > as
> > > > documentation.
> > > 
> > > You guys can add such a @Category annotation. The @KnownCategoriesEnum
needs
> > to
> > > go from here, we want to support dynamic sections in M54 and the
> corresponding
> > > server-provider will land soon.
> > 
> > Keep in mind that these annotations are only used in linting, i.e. nothing
> will
> > break at runtime if we have a value that is not a known category.
> 
> Ok. Didn't know that. Would it break though if we had unit tests that test
want
> to test for dynamic categories? Or does the linter ignore the test files?

Well, you might get a lint warning, and that would be about it... :)

> Still, I think that "known category" would be the wrong documentation signal
to
> the reader, because it's actually "any category", whether it's known or not.

Oh yes, I just haven't gotten around to making that change :)

[Philipp Keck, 2016-08-10 09:56:41]

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java
(right):

https://codereview.chromium.org/2230473002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ContentSuggestionsSource.java:25:
void onNewSuggestions(@KnownCategoriesEnum int category);
On 2016/08/10 09:39:11, Bernhard Bauer wrote:
> On 2016/08/10 09:05:09, Philipp Keck wrote:
> > On 2016/08/10 09:01:52, Bernhard Bauer wrote:
> > > On 2016/08/09 16:59:16, Philipp Keck wrote:
> > > > On 2016/08/09 14:30:11, Bernhard Bauer wrote:
> > > > > On 2016/08/09 13:52:20, Michael van Ouwerkerk wrote:
> > > > > > Is it always a known category? Then use the annotation in the
adapter,
> > > > > otherwise
> > > > > > remove it here.
> > > > > 
> > > > > This was a leftover from my CL. There could be server-defined
categories
> > > > (which
> > > > > are not known at compile time), but I liked having an annotation.
> Really,
> > we
> > > > > could make an (otherwise empty) @Category annotation that would just
> serve
> > > as
> > > > > documentation.
> > > > 
> > > > You guys can add such a @Category annotation. The @KnownCategoriesEnum
> needs
> > > to
> > > > go from here, we want to support dynamic sections in M54 and the
> > corresponding
> > > > server-provider will land soon.
> > > 
> > > Keep in mind that these annotations are only used in linting, i.e. nothing
> > will
> > > break at runtime if we have a value that is not a known category.
> > 
> > Ok. Didn't know that. Would it break though if we had unit tests that test
> want
> > to test for dynamic categories? Or does the linter ignore the test files?
> 
> Well, you might get a lint warning, and that would be about it... :)
> 
> > Still, I think that "known category" would be the wrong documentation signal
> to
> > the reader, because it's actually "any category", whether it's known or not.
> 
> Oh yes, I just haven't gotten around to making that change :)

Ok. I will leave this as is, if that's okay.

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:250:
if (firstHeaderPosition == RecyclerView.NO_POSITION) return
RecyclerView.NO_POSITION;
On 2016/08/10 09:24:39, Michael van Ouwerkerk wrote:
> This retains the assumption that there is only a card if there is a header,
and
> it must follow the header. Could we explicitly look for a card instead,
instead
> of relying on getFirstHeaderPosition? That was just a convenient shortcut,
shown
> to be brittle.
> 
> You can leave a TODO if you feel this change doesn't fit in this CL.

Left TODO. Wasn't really sure what "instanceof"s to check for. The first card
can be a status card or regular card or error card or action button (maybe not
the latter).
Also you're right, it doesn't really fit in the snippets bridge CL.

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:255:
int bottomSpacerPosition = getBottomSpacerPosition();
On 2016/08/10 09:24:39, Michael van Ouwerkerk wrote:
> Same for this one, we probably should not rely on getBottomSpacerPosition
here.

Same here, not sure what qualifies as "content".

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java
(right):

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java:34:
public int getCardLayout() {
On 2016/08/10 09:24:40, Michael van Ouwerkerk wrote:
> @ContentSuggestionsCardLayoutEnum

Done.

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
(right):

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:32:
mMoreButton = null; // TODO(pke): Read from info => ? new ActionListItem() :
null;
On 2016/08/10 09:24:40, Michael van Ouwerkerk wrote:
> What happened here? It looks like the more button was removed, it's always
null
> now.

It was always null before, too, because all callers would pass in
"showMoreButton=false". In an upcoming CL I will change this to read directly
from info (currently, info.hasMoreButton() does not exist, yet).

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:89:
* @param suggestionsSource The SnippetsBridge used to retrieve the thumbnails.
On 2016/08/10 09:24:40, Michael van Ouwerkerk wrote:
> It's not exactly of type SnippetsBridge now :-) How about "The source used to
> retrieve the thumbnails."

Done.

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java
(right):

https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:27:
private final TextView mHeaderListItemTextView;
On 2016/08/10 09:24:40, Michael van Ouwerkerk wrote:
> nit: I would prefer something shorter without ListItem in it as it's not one
of
> those. Maybe mHeaderView?

Done. mHeaderTextView for consistency with SnippetArticleViewHolder, where all
names for TextViews also end in "TextView".

[Michael van Ouwerkerk, 2016-08-10 10:13:44]

lgtm thanks!

[Philipp Keck, 2016-08-10 10:16:06]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-08-10 10:16:06]

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/2230473002/#ps200001
(title: "Michael's comments")

[commit-bot: I haz the power, 2016-08-10 10:16:15]

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

[dgn, 2016-08-10 10:31:21]

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

[dgn, 2016-08-10 10:31:23]

https://codereview.chromium.org/2230473002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java
(right):

https://codereview.chromium.org/2230473002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java:26:
this.mTitle = title;
the `this` usage is unnecessary here

https://codereview.chromium.org/2230473002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java
(right):

https://codereview.chromium.org/2230473002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:5:
package org.chromium.chrome.browser.ntp.snippets;
should this be in a ntp.suggestions package, and snippets is reserved for
articles stuff? or just rename the snippets package to suggestions?

https://codereview.chromium.org/2230473002/diff/200001/chrome/browser/android...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (left):

https://codereview.chromium.org/2230473002/diff/200001/chrome/browser/android...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:110: OnNewSuggestions();
We currently pull snippets when setting the observer. The suggestions would stay
empty until the next explicit or scheduled fetch otherwise. I couldn't see if
you are replacing it by something else in this patch.

[Philipp Keck, 2016-08-10 12:36:13]

Thank you everyone for reviewing.

https://codereview.chromium.org/2230473002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java
(right):

https://codereview.chromium.org/2230473002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java:26:
this.mTitle = title;
On 2016/08/10 10:31:22, dgn wrote:
> the `this` usage is unnecessary here

I consider this a nit and will fix it in a subsequent CL.

https://codereview.chromium.org/2230473002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java
(right):

https://codereview.chromium.org/2230473002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:5:
package org.chromium.chrome.browser.ntp.snippets;
On 2016/08/10 10:31:22, dgn wrote:
> should this be in a ntp.suggestions package, and snippets is reserved for
> articles stuff? or just rename the snippets package to suggestions?

This should just all be named suggestions. The SnippetArticleListItem and
ViewHolder should also be called suggestion. We should have very little
article-specific code. The renaming is part of the planned refactoring (Nicole).

We should also rename the SnippetsBridge. And the ntp_snippets namespace in the
C++ part.

https://codereview.chromium.org/2230473002/diff/200001/chrome/browser/android...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (left):

https://codereview.chromium.org/2230473002/diff/200001/chrome/browser/android...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:110: OnNewSuggestions();
On 2016/08/10 10:31:22, dgn wrote:
> We currently pull snippets when setting the observer. The suggestions would
stay
> empty until the next explicit or scheduled fetch otherwise. I couldn't see if
> you are replacing it by something else in this patch. 

Replaced with this:
https://codereview.chromium.org/2230473002/diff/200001/chrome/android/java/sr...
(so the callsite of SetObserver)

[commit-bot: I haz the power, 2016-08-10 12:38:09]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-08-10 12:39:59]

Description was changed from

==========
Refactor and extend SnippetsBridge for multi-section support

Add a category parameter to ContentSuggestionsService::Observer
::OnNewSuggestions. Add getters and observer events equivalent to the
ContentSuggestionsService to the SnippetsBridge. Adjust the UI layer to
use the new methods on the bridge. Modify the NewTabPageAdapterTest to
correctly mock the new bridge.

BUG=635794
==========

to

==========
Refactor and extend SnippetsBridge for multi-section support

Add a category parameter to ContentSuggestionsService::Observer
::OnNewSuggestions. Add getters and observer events equivalent to the
ContentSuggestionsService to the SnippetsBridge. Adjust the UI layer to
use the new methods on the bridge. Modify the NewTabPageAdapterTest to
correctly mock the new bridge.

BUG=635794

Committed: https://crrev.com/222d8a5248288b4bd3f059e25951dfbeb1f5af62
Cr-Commit-Position: refs/heads/master@{#411027}
==========

[commit-bot: I haz the power, 2016-08-10 12:40:00]

Patchset 11 (id:??) landed as
https://crrev.com/222d8a5248288b4bd3f059e25951dfbeb1f5af62
Cr-Commit-Position: refs/heads/master@{#411027}

[Bernhard Bauer, 2016-08-10 14:42:26]

https://codereview.chromium.org/2230473002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java
(right):

https://codereview.chromium.org/2230473002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:5:
package org.chromium.chrome.browser.ntp.snippets;
On 2016/08/10 12:36:12, Philipp Keck wrote:
> On 2016/08/10 10:31:22, dgn wrote:
> > should this be in a ntp.suggestions package, and snippets is reserved for
> > articles stuff? or just rename the snippets package to suggestions?
> 
> This should just all be named suggestions. The SnippetArticleListItem and
> ViewHolder should also be called suggestion. We should have very little
> article-specific code. The renaming is part of the planned refactoring
(Nicole).

(Who is on leave ☺️)

> We should also rename the SnippetsBridge. And the ntp_snippets namespace in
the
> C++ part.

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

On 2016/08/10 14:42:26, Bernhard Bauer wrote:
>
https://codereview.chromium.org/2230473002/diff/200001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java
> (right):
> 
>
https://codereview.chromium.org/2230473002/diff/200001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:5:
> package org.chromium.chrome.browser.ntp.snippets;
> On 2016/08/10 12:36:12, Philipp Keck wrote:
> > On 2016/08/10 10:31:22, dgn wrote:
> > > should this be in a ntp.suggestions package, and snippets is reserved for
> > > articles stuff? or just rename the snippets package to suggestions?
> > 
> > This should just all be named suggestions. The SnippetArticleListItem and
> > ViewHolder should also be called suggestion. We should have very little
> > article-specific code. The renaming is part of the planned refactoring
> (Nicole).
> 
> (Who is on leave ☺️)
> 
> > We should also rename the SnippetsBridge. And the ntp_snippets namespace in
> the
> > C++ part.

Renaming stuff -- that's what you hired the intern for ;-)