Change NTPSnippetsBridge to read from ContentSuggestionsService

Change NTPSnippetsBridge to read from ContentSuggestionsService

Change the NTPSnippetsBridge to receive ContentSuggestions from the new
ContentSuggestionsService instead of receiving NTPSnippets directly
from the NTPSnippetsService. Change the bridge/UI to receive and handle
the ContentSuggestionsCategoryStatus instead of the state and
DisabledReason from the NTPSnippetsStatusService.

Modify the implementations of NTPSnippetsService
::DiscardSuggestion and ::FetchSuggestionImage to accept
combined unique_ids instead of within_category_ids, which they
wrongly accepted before. The new implementations conform to
the ContentSuggestionsProvider interface and these two methods
should not be called directly on the NTPSnippetsService with
an ID of a NTPSnippet anymore (as it would now fail).

BUG=619560
Committed: https://crrev.com/27632ab6ad198cc8aceec6d50930bea0c2d8faef
Cr-Commit-Position: refs/heads/master@{#407441}

[Philipp Keck, 2016-07-18 14:45:36]

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

[Philipp Keck, 2016-07-18 14:45:36]

PTAL

[Philipp Keck, 2016-07-18 14:58:30]

Description was changed from

==========
Change the NTPSnippetsBridge to receive ContentSuggestions from the new
ContentSuggestionsService instead of receiving NTPSnippets directly
from the NTPSnippetsService. Change the bridge/UI to receive and handle
the ContentSuggestionsCategoryStatus instead of the state and
DisabledReason from the NTPSnippetsStatusService.

Use a pipe character instead of a colon to separate the
within_category_id from the category in a unique_id to avoid
accidentally splitting URLs, which always contain a colon.

BUG=619560
==========

to

==========
Change NTPSnippetsBridge to read from ContentSuggestionsService

Change the NTPSnippetsBridge to receive ContentSuggestions from the new
ContentSuggestionsService instead of receiving NTPSnippets directly
from the NTPSnippetsService. Change the bridge/UI to receive and handle
the ContentSuggestionsCategoryStatus instead of the state and
DisabledReason from the NTPSnippetsStatusService.

Use a pipe character instead of a colon to separate the
within_category_id from the category in a unique_id to avoid
accidentally splitting URLs, which always contain a colon.

BUG=619560
==========

[Marc Treib, 2016-07-19 09:30:25]

Mostly looks good! A few things though:
- the switch from colon to pipe should be a separate CL
- the "not in use" comments on ContentSuggestionsService etc need to be updated
- there's some non-trivial changes in NTPSnippetsService (ID handling) that I'm
not entirely happy with... I'd have like to have a CL that really only does the
switch. If that's not feasible, then at least the NTPSnippetsService changes
should be mentioned in the CL description.

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

https://codereview.chromium.org/2158883002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:170:
|| categoryStatus == ContentSuggestionsCategoryStatus.AVAILABLE_LOADING) {
Hrm.. is this the equivalent of that helper function we have in C++?

https://codereview.chromium.org/2158883002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java
(right):

https://codereview.chromium.org/2158883002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:164:
// This should only be a transient state: during app launch, or when the sync
I think the indentation here was on purpose, since the comment refers to the
case above.

https://codereview.chromium.org/2158883002/diff/1/components/ntp_snippets/BUI...
File components/ntp_snippets/BUILD.gn (right):

https://codereview.chromium.org/2158883002/diff/1/components/ntp_snippets/BUI...
components/ntp_snippets/BUILD.gn:74: "ntp_snippets_status_service.h",
Is this still needed?

https://codereview.chromium.org/2158883002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider.cc (right):

https://codereview.chromium.org/2158883002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider.cc:34: size_t colon_index =
unique_id.find("|");
Make the pipe a constant? (kSeparator)

https://codereview.chromium.org/2158883002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (left):

https://codereview.chromium.org/2158883002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:281: suggestion_id,
Wait, so this only worked because suggestion_id was in fact a within-category-id
so far?

[Philipp Keck, 2016-07-19 12:21:50]

The separate CL is here: https://codereview.chromium.org/2155243003/

I will update this CL based on the other one and complete work on the remaining
comments.

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

https://codereview.chromium.org/2158883002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:170:
|| categoryStatus == ContentSuggestionsCategoryStatus.AVAILABLE_LOADING) {
On 2016/07/19 09:30:24, Marc Treib wrote:
> Hrm.. is this the equivalent of that helper function we have in C++?

Not exactly, here it also includes "initializing".

https://codereview.chromium.org/2158883002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider.cc (right):

https://codereview.chromium.org/2158883002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider.cc:34: size_t colon_index =
unique_id.find("|");
On 2016/07/19 09:30:24, Marc Treib wrote:
> Make the pipe a constant? (kSeparator)

Done.

https://codereview.chromium.org/2158883002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (left):

https://codereview.chromium.org/2158883002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:281: suggestion_id,
On 2016/07/19 09:30:25, Marc Treib wrote:
> Wait, so this only worked because suggestion_id was in fact a
within-category-id
> so far?

Yes. This method implements two different interfaces: The FetchImage method for
the old NTPSnippetsService and the
ContentSuggestionsProvider::FetchSuggestImage. Same for DiscardSuggestion. For a
cleaner solution, the implementation of this method should probably have been
different in a previous CL.

This previously only worked because it was never called in the context of the
ContentSuggestions interface. And in the future it will only work if nobody
calls it with an ID they retrieved from snippets(). Maybe should rename
snippets() to snippetsForDebugging() now, because only the snippets-internals
page should use it.

[Philipp Keck, 2016-07-19 13:12:04]

Description was changed from

==========
Change NTPSnippetsBridge to read from ContentSuggestionsService

Change the NTPSnippetsBridge to receive ContentSuggestions from the new
ContentSuggestionsService instead of receiving NTPSnippets directly
from the NTPSnippetsService. Change the bridge/UI to receive and handle
the ContentSuggestionsCategoryStatus instead of the state and
DisabledReason from the NTPSnippetsStatusService.

Use a pipe character instead of a colon to separate the
within_category_id from the category in a unique_id to avoid
accidentally splitting URLs, which always contain a colon.

BUG=619560
==========

to

==========
Change NTPSnippetsBridge to read from ContentSuggestionsService

Change the NTPSnippetsBridge to receive ContentSuggestions from the new
ContentSuggestionsService instead of receiving NTPSnippets directly
from the NTPSnippetsService. Change the bridge/UI to receive and handle
the ContentSuggestionsCategoryStatus instead of the state and
DisabledReason from the NTPSnippetsStatusService.

Modify the implementations of NTPSnippetsService
::DiscardSuggestion and ::FetchSuggestionImage to accept
combined unique_ids instead of within_category_ids, which they
wrongly accepted before. The new implementations conform to
the ContentSuggestionsProvider interface and these two methods
should not be called directly on the NTPSnippetsService with
an ID of a NTPSnippet anymore (as it would now fail).

BUG=619560
==========

[Philipp Keck, 2016-07-19 13:53:23]

Updated the CL description.
Removed the "not-in-use" comments from the code.

https://codereview.chromium.org/2158883002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java
(right):

https://codereview.chromium.org/2158883002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:164:
// This should only be a transient state: during app launch, or when the sync
On 2016/07/19 09:30:24, Marc Treib wrote:
> I think the indentation here was on purpose, since the comment refers to the
> case above.

The auto-formatter removed the indentation, and the formatter is right: There is
no statement that this comment applies to if it's indented. I still undid those
changes.

https://codereview.chromium.org/2158883002/diff/1/components/ntp_snippets/BUI...
File components/ntp_snippets/BUILD.gn (right):

https://codereview.chromium.org/2158883002/diff/1/components/ntp_snippets/BUI...
components/ntp_snippets/BUILD.gn:74: "ntp_snippets_status_service.h",
On 2016/07/19 09:30:24, Marc Treib wrote:
> Is this still needed?

No, I removed it along with the comment on the enum.

[Marc Treib, 2016-07-19 16:21:19]

https://codereview.chromium.org/2158883002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java
(right):

https://codereview.chromium.org/2158883002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:164:
// This should only be a transient state: during app launch, or when the sync
On 2016/07/19 13:53:23, Philipp Keck wrote:
> On 2016/07/19 09:30:24, Marc Treib wrote:
> > I think the indentation here was on purpose, since the comment refers to the
> > case above.
> 
> The auto-formatter removed the indentation, and the formatter is right: There
is
> no statement that this comment applies to if it's indented. I still undid
those
> changes. 

Well, it's a fancy "// fallthrough" comment, so there is no statement that it
could apply to.

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

https://codereview.chromium.org/2158883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:154:
// Observers should not be registered for that state
Not your doing, but: Is this still true when we can get into the
EXPLICITLY_DISABLED state during runtime? (Which can happen now:
https://codereview.chromium.org/2158373002/)

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

https://codereview.chromium.org/2158883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:31:
/** Called when the ARTICLES category is about to change its status. */
"about to change", really?

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

https://codereview.chromium.org/2158883002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:178:
ContentSuggestionsCategoryStatus new_status) {
You're missing an "if (category != ARTICLES) return"

https://codereview.chromium.org/2158883002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:179: // The user signed out or
disabled sync. Since snippets rely on those, we
This comment isn't appropriate anymore, please update

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

https://codereview.chromium.org/2158883002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.h:39: const
base::android::JavaParamRef<jstring>& snippet_id,
suggestion_id, also in the other methods

[Philipp Keck, 2016-07-19 16:47:35]

https://codereview.chromium.org/2158883002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java
(right):

https://codereview.chromium.org/2158883002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:164:
// This should only be a transient state: during app launch, or when the sync
On 2016/07/19 16:21:19, Marc Treib wrote:
> On 2016/07/19 13:53:23, Philipp Keck wrote:
> > On 2016/07/19 09:30:24, Marc Treib wrote:
> > > I think the indentation here was on purpose, since the comment refers to
the
> > > case above.
> > 
> > The auto-formatter removed the indentation, and the formatter is right:
There
> is
> > no statement that this comment applies to if it's indented. I still undid
> those
> > changes. 
> 
> Well, it's a fancy "// fallthrough" comment, so there is no statement that it
> could apply to.

Acknowledged.

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

https://codereview.chromium.org/2158883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:154:
// Observers should not be registered for that state
On 2016/07/19 16:21:19, Marc Treib wrote:
> Not your doing, but: Is this still true when we can get into the
> EXPLICITLY_DISABLED state during runtime? (Which can happen now:
> https://codereview.chromium.org/2158373002/)

You're right, this can definitely happen. With multi-sections, it can happen
anyway, only the all-sections-disabled is equally rare as the previous
"DisabledReason.EXPLICITLY_DISABLED". And even for that one, it would require
the UI to shut down/unregister the observer before the backend changes state --
I don't have any insights which will occur earlier and we probably shouldn't
rely on anything here and develop the UI such that it can handle anything.

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

https://codereview.chromium.org/2158883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:31:
/** Called when the ARTICLES category is about to change its status. */
On 2016/07/19 16:21:19, Marc Treib wrote:
> "about to change", really?

Done, replaced with "changed its status".

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

https://codereview.chromium.org/2158883002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:178:
ContentSuggestionsCategoryStatus new_status) {
On 2016/07/19 16:21:19, Marc Treib wrote:
> You're missing an "if (category != ARTICLES) return"

Done.

https://codereview.chromium.org/2158883002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:179: // The user signed out or
disabled sync. Since snippets rely on those, we
On 2016/07/19 16:21:19, Marc Treib wrote:
> This comment isn't appropriate anymore, please update

Removed the comment, I don't see any reason to explain what a call means in the
bridge, the bridge should just hand through these things and there should be
enough documentation on both ends.

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

https://codereview.chromium.org/2158883002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.h:39: const
base::android::JavaParamRef<jstring>& snippet_id,
On 2016/07/19 16:21:19, Marc Treib wrote:
> suggestion_id, also in the other methods

Yeah, though the entire class would need to be refactored. It now transports
ContentSuggestions, but is still called SnippetsBridge. And DiscardSnippet
should be DiscardSuggestion, etc.

My plan is to build a second "SuggestionBridge" with all the new methods next to
this one (working with Michael to make this an appropriate interface for the
refactored UI) and then delete this class completely. Otherwise I'd make a
separate renaming CL that renames everything: class, methods, parameters.

(I do see the point that "suggestion_id" would make it more explicit that this
is now the combined/unique ID. But consequently I also would have to rename that
in all the UI code. Since the idea has been and will be that the UI does not
understand this ID, it shouldn't really matter; the UI can't accidentally pass
in the wrong ID here, because it doesn't have it.)

[Philipp Keck, 2016-07-19 16:48:34]

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

[Philipp Keck, 2016-07-19 16:48:35]

bauerb@chromium.org: Please review the changes especially in the Android part.
Thank you!

[dgn, 2016-07-19 23:52:28]

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

https://codereview.chromium.org/2158883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:180:
+ " when provider for ARTICLES is not registered.");
Why mentioning ARTICLES explicitly here? Status cards are not specific to them.

How about something like "FATAL: No status card to display for the NOT_PROVIDED
status."? In this case, the category should just be ignored, right?

[Marc Treib, 2016-07-20 09:17:38]

LGTM with a few more nits, modulo that one assert in the UI. Hopefully the
Android folks can help there :)

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

https://codereview.chromium.org/2158883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:154:
// Observers should not be registered for that state
On 2016/07/19 16:47:34, Philipp Keck wrote:
> On 2016/07/19 16:21:19, Marc Treib wrote:
> > Not your doing, but: Is this still true when we can get into the
> > EXPLICITLY_DISABLED state during runtime? (Which can happen now:
> > https://codereview.chromium.org/2158373002/)
> 
> You're right, this can definitely happen. With multi-sections, it can happen
> anyway, only the all-sections-disabled is equally rare as the previous
> "DisabledReason.EXPLICITLY_DISABLED". And even for that one, it would require
> the UI to shut down/unregister the observer before the backend changes state
--
> I don't have any insights which will occur earlier and we probably shouldn't
> rely on anything here and develop the UI such that it can handle anything.

Well, can we just remove the assert (or change it to check for the
all-sections-disabled value), or will something else break? If so, then that
needs to be fixed before this can land...

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

https://codereview.chromium.org/2158883002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.h:39: const
base::android::JavaParamRef<jstring>& snippet_id,
On 2016/07/19 16:47:35, Philipp Keck wrote:
> On 2016/07/19 16:21:19, Marc Treib wrote:
> > suggestion_id, also in the other methods
> 
> Yeah, though the entire class would need to be refactored. It now transports
> ContentSuggestions, but is still called SnippetsBridge. And DiscardSnippet
> should be DiscardSuggestion, etc.
> 
> My plan is to build a second "SuggestionBridge" with all the new methods next
to
> this one (working with Michael to make this an appropriate interface for the
> refactored UI) and then delete this class completely. Otherwise I'd make a
> separate renaming CL that renames everything: class, methods, parameters.
> 
> (I do see the point that "suggestion_id" would make it more explicit that this
> is now the combined/unique ID. But consequently I also would have to rename
that
> in all the UI code. Since the idea has been and will be that the UI does not
> understand this ID, it shouldn't really matter; the UI can't accidentally pass
> in the wrong ID here, because it doesn't have it.)

Acknowledged.

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

https://codereview.chromium.org/2158883002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:16: #include
"chrome/browser/ntp_snippets/ntp_snippets_service_factory.h"
Not needed anymore

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

https://codereview.chromium.org/2158883002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.h:18: #include
"components/ntp_snippets/ntp_snippets_service.h"
Not needed anymore

https://codereview.chromium.org/2158883002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.h:80:
ScopedObserver<ntp_snippets::ContentSuggestionsService,
nit: I'd move this somewhere else, or at least put an empty line before it. As
it is, it looks like it's related to |observer_| and the comment above.

https://codereview.chromium.org/2158883002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2158883002/diff/60001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:64: // Stores and vends fresh
content data for the NTP.
nit: Update the comment, given that it's now a provider (except for the name)?

[Bernhard Bauer, 2016-07-20 11:09:04]

lgtm

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

https://codereview.chromium.org/2158883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:153:
public void onCategoryStatusChanged(int categoryStatus) {
So I guess the plan is to add the actual category to this as well at a later
point?

https://codereview.chromium.org/2158883002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java
(right):

https://codereview.chromium.org/2158883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:184:
Log.i(TAG, "Not showing ARTICLES suggestions because of provider error.");
Log.d maybe?

[Philipp Keck, 2016-07-20 13:20:02]

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

https://codereview.chromium.org/2158883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:154:
// Observers should not be registered for that state
On 2016/07/20 09:17:38, Marc Treib wrote:
> On 2016/07/19 16:47:34, Philipp Keck wrote:
> > On 2016/07/19 16:21:19, Marc Treib wrote:
> > > Not your doing, but: Is this still true when we can get into the
> > > EXPLICITLY_DISABLED state during runtime? (Which can happen now:
> > > https://codereview.chromium.org/2158373002/)
> > 
> > You're right, this can definitely happen. With multi-sections, it can happen
> > anyway, only the all-sections-disabled is equally rare as the previous
> > "DisabledReason.EXPLICITLY_DISABLED". And even for that one, it would
require
> > the UI to shut down/unregister the observer before the backend changes state
> --
> > I don't have any insights which will occur earlier and we probably shouldn't
> > rely on anything here and develop the UI such that it can handle anything.
> 
> Well, can we just remove the assert (or change it to check for the
> all-sections-disabled value), or will something else break? If so, then that
> needs to be fixed before this can land...

It is unclear whether the Zine UI should just be empty in this case or display
an error card or even be inactive (and we instead show "Bookmarks" and "Recent
tabs"). Until this is clarified, I implemented a temporary error card for the
LOADING_ERROR and CATEGORY_EXPLICITLY_DISABLED case. Both are very rare at the
moment, but we need more clarification on all of this (see comments in PRD)
before we implement the multi-section UI.

As for the assert here, it's meant to check against situations where the UI
thinks the feature is enabled, but the backend thinks it's not. This still makes
sense to check for the #enable-ntp-snippets flag, since that one is only changed
through a Chrome restart. So I changed the assert check here to
ALL_SUGGESTIONS_EXPLICITLY_DISABLED. The CATEGORY_EXPLICITLY_DISABLED case will
then be handled with an error card.

I tested the status card by making
NTPSnippetsStatusService::GetDisabledReasonFromDeps return
DisabledReason::EXPLICITLY_DISABLED.

This is the bug for proper behavior in these cases:
https://bugs.chromium.org/p/chromium/issues/detail?id=629817

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

https://codereview.chromium.org/2158883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:180:
+ " when provider for ARTICLES is not registered.");
On 2016/07/19 23:52:27, dgn wrote:
> Why mentioning ARTICLES explicitly here? Status cards are not specific to
them.
> 
> How about something like "FATAL: No status card to display for the
NOT_PROVIDED
> status."? In this case, the category should just be ignored, right?

This is on purpose. While this code would certainly compile and work for the
multi-section UI, it (probably) doesn't follow the specified behavior for the
multi-section case. The behavior is not yet specified, but I assume that a
section with "NOT_PROVIDED" should display a card on how to enable that section,
or no section at all.
By hardcoding the all-caps articles, it allows to find this place easily when
refactoring for the multi-section UI.

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

https://codereview.chromium.org/2158883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:153:
public void onCategoryStatusChanged(int categoryStatus) {
On 2016/07/20 11:09:04, Bernhard Bauer wrote:
> So I guess the plan is to add the actual category to this as well at a later
> point?

Yes. A draft of these changes is here:
https://codereview.chromium.org/2166523002/diff/1/chrome/android/java/src/org...

https://codereview.chromium.org/2158883002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java
(right):

https://codereview.chromium.org/2158883002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:184:
Log.i(TAG, "Not showing ARTICLES suggestions because of provider error.");
On 2016/07/20 11:09:04, Bernhard Bauer wrote:
> Log.d maybe?

Done.

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

https://codereview.chromium.org/2158883002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:16: #include
"chrome/browser/ntp_snippets/ntp_snippets_service_factory.h"
On 2016/07/20 09:17:38, Marc Treib wrote:
> Not needed anymore

It's still needed in the static methods FetchSnippets and RescheduleFetching.

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

https://codereview.chromium.org/2158883002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.h:18: #include
"components/ntp_snippets/ntp_snippets_service.h"
On 2016/07/20 09:17:38, Marc Treib wrote:
> Not needed anymore

Moved to .cc file, it's needed there.

https://codereview.chromium.org/2158883002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.h:80:
ScopedObserver<ntp_snippets::ContentSuggestionsService,
On 2016/07/20 09:17:38, Marc Treib wrote:
> nit: I'd move this somewhere else, or at least put an empty line before it. As
> it is, it looks like it's related to |observer_| and the comment above.

Done.

https://codereview.chromium.org/2158883002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2158883002/diff/60001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:64: // Stores and vends fresh
content data for the NTP.
On 2016/07/20 09:17:38, Marc Treib wrote:
> nit: Update the comment, given that it's now a provider (except for the name)?

Done, please read the new comment.

[Marc Treib, 2016-07-20 13:41:12]

Still LGTM!

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

https://codereview.chromium.org/2158883002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:16: #include
"chrome/browser/ntp_snippets/ntp_snippets_service_factory.h"
On 2016/07/20 13:20:02, Philipp Keck wrote:
> On 2016/07/20 09:17:38, Marc Treib wrote:
> > Not needed anymore
> 
> It's still needed in the static methods FetchSnippets and RescheduleFetching.

Ah, right. Hm, we could pipe both of these through the CSService of course, but
since so far no other provider has (or needs) these... maybe best to keep as-is,
at least for now.

https://codereview.chromium.org/2158883002/diff/60001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2158883002/diff/60001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:64: // Stores and vends fresh
content data for the NTP.
On 2016/07/20 13:20:02, Philipp Keck wrote:
> On 2016/07/20 09:17:38, Marc Treib wrote:
> > nit: Update the comment, given that it's now a provider (except for the
name)?
> 
> Done, please read the new comment.

Yup, better! :)

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

https://codereview.chromium.org/2158883002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:165:
// case. This should only be called when the enterprise policy pref has been
disabled.
nit: I'd formulate this as "when the category has been disabled via enterprise
policy" (that's technically when the policy is active, so your formulation
sounds a bit confusing IMO)

[Philipp Keck, 2016-07-20 14:51:15]

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

https://codereview.chromium.org/2158883002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:16: #include
"chrome/browser/ntp_snippets/ntp_snippets_service_factory.h"
On 2016/07/20 13:41:11, Marc Treib wrote:
> On 2016/07/20 13:20:02, Philipp Keck wrote:
> > On 2016/07/20 09:17:38, Marc Treib wrote:
> > > Not needed anymore
> > 
> > It's still needed in the static methods FetchSnippets and
RescheduleFetching.
> 
> Ah, right. Hm, we could pipe both of these through the CSService of course,
but
> since so far no other provider has (or needs) these... maybe best to keep
as-is,
> at least for now.

For now, I'd consider it an implementation detail of the NTPSnippetsService that
it needs to rely on the Android part to do the scheduling. A perfect
ContentSuggestionsProvider implementation would just magically start delivering
suggestions and manage its scheduling internally.

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

https://codereview.chromium.org/2158883002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:165:
// case. This should only be called when the enterprise policy pref has been
disabled.
On 2016/07/20 13:41:12, Marc Treib wrote:
> nit: I'd formulate this as "when the category has been disabled via enterprise
> policy" (that's technically when the policy is active, so your formulation
> sounds a bit confusing IMO)

Done.

[Philipp Keck, 2016-07-20 14:52:55]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-07-20 14:52:56]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2158883002/#ps100001
(title: "Adjust comment about enterprise policy")

[commit-bot: I haz the power, 2016-07-20 14:53:11]

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-07-20 14:55:04]

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

[commit-bot: I haz the power, 2016-07-20 14:55:05]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[Marc Treib, 2016-07-20 14:56:03]

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

https://codereview.chromium.org/2158883002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:16: #include
"chrome/browser/ntp_snippets/ntp_snippets_service_factory.h"
On 2016/07/20 14:51:15, Philipp Keck wrote:
> On 2016/07/20 13:41:11, Marc Treib wrote:
> > On 2016/07/20 13:20:02, Philipp Keck wrote:
> > > On 2016/07/20 09:17:38, Marc Treib wrote:
> > > > Not needed anymore
> > > 
> > > It's still needed in the static methods FetchSnippets and
> RescheduleFetching.
> > 
> > Ah, right. Hm, we could pipe both of these through the CSService of course,
> but
> > since so far no other provider has (or needs) these... maybe best to keep
> as-is,
> > at least for now.
> 
> For now, I'd consider it an implementation detail of the NTPSnippetsService
that
> it needs to rely on the Android part to do the scheduling. A perfect
> ContentSuggestionsProvider implementation would just magically start
delivering
> suggestions and manage its scheduling internally.

For the scheduling I agree (and it's really just out of laziness that that even
lives in the same Bridge class). But FetchSnippets can also be triggered by the
user. But since the whole "More" discussion is still ongoing, no point in
designing something now.

[Philipp Keck, 2016-07-22 11:39:23]

Marc, PTAL at:
https://codereview.chromium.org/2158883002/diff/120001/components/ntp_snippet...
https://codereview.chromium.org/2158883002/diff/120001/components/ntp_snippet...
https://codereview.chromium.org/2158883002/diff/120001/chrome/android/java/sr...

Michael, Bernhard, PTAL at:
https://codereview.chromium.org/2158883002/diff/120001/chrome/android/java/sr...
https://codereview.chromium.org/2158883002/diff/120001/chrome/android/junit/s...
https://codereview.chromium.org/2158883002/diff/120001/chrome/android/java/sr...

[Marc Treib, 2016-07-22 11:44:42]

https://codereview.chromium.org/2158883002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/2158883002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:36:
// Note: This code is duplicated in content_suggestions_category_status.h.
It's really in the .cc file :)

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

https://codereview.chromium.org/2158883002/diff/120001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_category_status.h:57:
ContentSuggestionsCategoryStatus status);
Hm, this almost seems so specific that callers might just as well hardcode the
individual values. Can we find some better name for this? Or maybe for its
negation?

[Philipp Keck, 2016-07-22 11:50:43]

https://codereview.chromium.org/2158883002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/2158883002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:36:
// Note: This code is duplicated in content_suggestions_category_status.h.
On 2016/07/22 11:44:42, Marc Treib wrote:
> It's really in the .cc file :)

Done.

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

https://codereview.chromium.org/2158883002/diff/120001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_category_status.h:57:
ContentSuggestionsCategoryStatus status);
On 2016/07/22 11:44:42, Marc Treib wrote:
> Hm, this almost seems so specific that callers might just as well hardcode the
> individual values. Can we find some better name for this? Or maybe for its
> negation?

Is..StatusNonError
Is..StatusNotErroneous

Alternative: Don't hardcode all the values (because there might be more
AVAILABLE_X values in the future and we want to avoid forgetting places to
update those and want to avoid verbose code). But we could instead use "status
== INITIALIZING || Is..StatusAvailable(status)" everywhere we use this function.

[Marc Treib, 2016-07-22 12:05:33]

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

https://codereview.chromium.org/2158883002/diff/120001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_category_status.h:57:
ContentSuggestionsCategoryStatus status);
On 2016/07/22 11:50:42, Philipp Keck wrote:
> On 2016/07/22 11:44:42, Marc Treib wrote:
> > Hm, this almost seems so specific that callers might just as well hardcode
the
> > individual values. Can we find some better name for this? Or maybe for its
> > negation?
> 
> Is..StatusNonError
> Is..StatusNotErroneous

I'd like to avoid negated functions like that, they tend to make conditions very
hard to read.
Something like Is...Error might work, though I wouldn't really consider all of
these statuses errors. IsNotAvailable would be more accurate, but then you'd
expect it to be the opposite of Available, which is also not true... argh.
Essentially it's a tri-state grouping: Available, Not Available, Initializing
(i.e. Unknown).

> Alternative: Don't hardcode all the values (because there might be more
> AVAILABLE_X values in the future and we want to avoid forgetting places to
> update those and want to avoid verbose code). But we could instead use "status
> == INITIALIZING || Is..StatusAvailable(status)" everywhere we use this
function.

That might be okay, though it also looks a bit weird.

[Philipp Keck, 2016-07-22 12:12:17]

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

https://codereview.chromium.org/2158883002/diff/120001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_category_status.h:57:
ContentSuggestionsCategoryStatus status);
On 2016/07/22 12:05:33, Marc Treib wrote:
> On 2016/07/22 11:50:42, Philipp Keck wrote:
> > On 2016/07/22 11:44:42, Marc Treib wrote:
> > > Hm, this almost seems so specific that callers might just as well hardcode
> the
> > > individual values. Can we find some better name for this? Or maybe for its
> > > negation?
> > 
> > Is..StatusNonError
> > Is..StatusNotErroneous
> 
> I'd like to avoid negated functions like that, they tend to make conditions
very
> hard to read.
> Something like Is...Error might work, though I wouldn't really consider all of
> these statuses errors. IsNotAvailable would be more accurate, but then you'd
> expect it to be the opposite of Available, which is also not true... argh.
> Essentially it's a tri-state grouping: Available, Not Available, Initializing
> (i.e. Unknown).

Exactly. And if those were A, B and C, at least I would prefer
if (A || B)
over
if (!C)

And the equivalent of A||B is "InitOrAvailable" ...
It may sound a bit specific, but it's just one of the interesting cases. Of
course, we shouldn't be making specific helper functions for all combinations.

> > Alternative: Don't hardcode all the values (because there might be more
> > AVAILABLE_X values in the future and we want to avoid forgetting places to
> > update those and want to avoid verbose code). But we could instead use
"status
> > == INITIALIZING || Is..StatusAvailable(status)" everywhere we use this
> function.
> 
> That might be okay, though it also looks a bit weird.

We could introduce Is..Unknown or Is..Initializing that just checks for the one
value (at the moment) to make the "A||B" call look more symmetric.

[Marc Treib, 2016-07-22 12:23:34]

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

https://codereview.chromium.org/2158883002/diff/120001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_category_status.h:57:
ContentSuggestionsCategoryStatus status);
On 2016/07/22 12:12:17, Philipp Keck wrote:
> On 2016/07/22 12:05:33, Marc Treib wrote:
> > On 2016/07/22 11:50:42, Philipp Keck wrote:
> > > On 2016/07/22 11:44:42, Marc Treib wrote:
> > > > Hm, this almost seems so specific that callers might just as well
hardcode
> > the
> > > > individual values. Can we find some better name for this? Or maybe for
its
> > > > negation?
> > > 
> > > Is..StatusNonError
> > > Is..StatusNotErroneous
> > 
> > I'd like to avoid negated functions like that, they tend to make conditions
> very
> > hard to read.
> > Something like Is...Error might work, though I wouldn't really consider all
of
> > these statuses errors. IsNotAvailable would be more accurate, but then you'd
> > expect it to be the opposite of Available, which is also not true... argh.
> > Essentially it's a tri-state grouping: Available, Not Available,
Initializing
> > (i.e. Unknown).
> 
> Exactly. And if those were A, B and C, at least I would prefer
> if (A || B)
> over
> if (!C)

Eh, depends on the context... if we just want to check that it's not in a "bad"
state, then (!C) would be the right thing.
Anyway, just to be clear: I'm not blocking the CL over this; mostly just
bikeshedding here :)

> And the equivalent of A||B is "InitOrAvailable" ...
> It may sound a bit specific, but it's just one of the interesting cases. Of
> course, we shouldn't be making specific helper functions for all combinations.
> 
> > > Alternative: Don't hardcode all the values (because there might be more
> > > AVAILABLE_X values in the future and we want to avoid forgetting places to
> > > update those and want to avoid verbose code). But we could instead use
> "status
> > > == INITIALIZING || Is..StatusAvailable(status)" everywhere we use this
> > function.
> > 
> > That might be okay, though it also looks a bit weird.
> 
> We could introduce Is..Unknown or Is..Initializing that just checks for the
one
> value (at the moment) to make the "A||B" call look more symmetric.

[Philipp Keck, 2016-07-25 09:25:00]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-07-25 09:25:00]

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/2158883002/#ps140001
(title: "Marc's comments; Rebase")

[commit-bot: I haz the power, 2016-07-25 09:25:12]

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-07-25 10:20:42]

Description was changed from

==========
Change NTPSnippetsBridge to read from ContentSuggestionsService

Change the NTPSnippetsBridge to receive ContentSuggestions from the new
ContentSuggestionsService instead of receiving NTPSnippets directly
from the NTPSnippetsService. Change the bridge/UI to receive and handle
the ContentSuggestionsCategoryStatus instead of the state and
DisabledReason from the NTPSnippetsStatusService.

Modify the implementations of NTPSnippetsService
::DiscardSuggestion and ::FetchSuggestionImage to accept
combined unique_ids instead of within_category_ids, which they
wrongly accepted before. The new implementations conform to
the ContentSuggestionsProvider interface and these two methods
should not be called directly on the NTPSnippetsService with
an ID of a NTPSnippet anymore (as it would now fail).

BUG=619560
==========

to

==========
Change NTPSnippetsBridge to read from ContentSuggestionsService

Change the NTPSnippetsBridge to receive ContentSuggestions from the new
ContentSuggestionsService instead of receiving NTPSnippets directly
from the NTPSnippetsService. Change the bridge/UI to receive and handle
the ContentSuggestionsCategoryStatus instead of the state and
DisabledReason from the NTPSnippetsStatusService.

Modify the implementations of NTPSnippetsService
::DiscardSuggestion and ::FetchSuggestionImage to accept
combined unique_ids instead of within_category_ids, which they
wrongly accepted before. The new implementations conform to
the ContentSuggestionsProvider interface and these two methods
should not be called directly on the NTPSnippetsService with
an ID of a NTPSnippet anymore (as it would now fail).

BUG=619560
==========

[commit-bot: I haz the power, 2016-07-25 10:20:44]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-25 10:22:53]

Description was changed from

==========
Change NTPSnippetsBridge to read from ContentSuggestionsService

Change the NTPSnippetsBridge to receive ContentSuggestions from the new
ContentSuggestionsService instead of receiving NTPSnippets directly
from the NTPSnippetsService. Change the bridge/UI to receive and handle
the ContentSuggestionsCategoryStatus instead of the state and
DisabledReason from the NTPSnippetsStatusService.

Modify the implementations of NTPSnippetsService
::DiscardSuggestion and ::FetchSuggestionImage to accept
combined unique_ids instead of within_category_ids, which they
wrongly accepted before. The new implementations conform to
the ContentSuggestionsProvider interface and these two methods
should not be called directly on the NTPSnippetsService with
an ID of a NTPSnippet anymore (as it would now fail).

BUG=619560
==========

to

==========
Change NTPSnippetsBridge to read from ContentSuggestionsService

Change the NTPSnippetsBridge to receive ContentSuggestions from the new
ContentSuggestionsService instead of receiving NTPSnippets directly
from the NTPSnippetsService. Change the bridge/UI to receive and handle
the ContentSuggestionsCategoryStatus instead of the state and
DisabledReason from the NTPSnippetsStatusService.

Modify the implementations of NTPSnippetsService
::DiscardSuggestion and ::FetchSuggestionImage to accept
combined unique_ids instead of within_category_ids, which they
wrongly accepted before. The new implementations conform to
the ContentSuggestionsProvider interface and these two methods
should not be called directly on the NTPSnippetsService with
an ID of a NTPSnippet anymore (as it would now fail).

BUG=619560

Committed: https://crrev.com/27632ab6ad198cc8aceec6d50930bea0c2d8faef
Cr-Commit-Position: refs/heads/master@{#407441}
==========

[commit-bot: I haz the power, 2016-07-25 10:22:54]

Patchset 8 (id:??) landed as
https://crrev.com/27632ab6ad198cc8aceec6d50930bea0c2d8faef
Cr-Commit-Position: refs/heads/master@{#407441}