Change NTPSnippetsBridge to read from ContentSuggestionsService

components/ntp_snippets/content_suggestions_category_status.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_CATEGORY_STATUS_H_
 #define COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_CATEGORY_STATUS_H_
 
 namespace ntp_snippets {
 
 // Represents the status of a category of content suggestions.
@@ skipping 32 matching lines @@
 
   // Content suggestions are not available because an error occured when loading
   // or updating them.
   LOADING_ERROR
 };
 
 // Determines whether the given status is one of the AVAILABLE statuses.
 bool IsContentSuggestionsCategoryStatusAvailable(
     ContentSuggestionsCategoryStatus status);
 
+// Determines whether the given status is INITIALIZING or one of the AVAILABLE
+// statuses. All of these statuses have in common that there is or will soon be
+// content.
+bool IsContentSuggestionsCategoryStatusInitOrAvailable(
+    ContentSuggestionsCategoryStatus status);

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

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:42]

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]

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).
That might be okay, though it also looks a bit weird.

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

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

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 :)

+
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_CATEGORY_STATUS_H_