Use server signal to decide whether to notify

chrome/browser/android/ntp/content_suggestions_notifier_service.cc

Index: chrome/browser/android/ntp/content_suggestions_notifier_service.cc
diff --git a/chrome/browser/android/ntp/content_suggestions_notifier_service.cc b/chrome/browser/android/ntp/content_suggestions_notifier_service.cc
index 07f305e977bb5db8ed0fb830f14f7e07319817d7..606e5a4fac38325398df37ebbe4d98171ec90bd1 100644
--- a/chrome/browser/android/ntp/content_suggestions_notifier_service.cc
+++ b/chrome/browser/android/ntp/content_suggestions_notifier_service.cc
@@ -53,19 +53,16 @@ class ContentSuggestionsNotifierService::NotifyingObserver
       : service_(service), prefs_(prefs), weak_ptr_factory_(this) {}
 
   void OnNewSuggestions(Category category) override {
-    if (!category.IsKnownCategory(KnownCategories::ARTICLES)) {
+    const ContentSuggestion* suggestion = GetSuggestionToNotifyAbout(category);
+    if (!suggestion) {
       return;
     }
-    const auto& suggestions = service_->GetSuggestionsForCategory(category);
-    if (!suggestions.empty()) {
-      const auto& suggestion = suggestions[0];
-      service_->FetchSuggestionImage(
-          suggestions[0].id(),
-          base::Bind(&NotifyingObserver::ImageFetched,
-                     weak_ptr_factory_.GetWeakPtr(), suggestion.id(),
-                     suggestion.url(), suggestion.title(),
-                     suggestion.publisher_name()));
-    }
+    service_->FetchSuggestionImage(
+        suggestion->id(),
+        base::Bind(&NotifyingObserver::ImageFetched,
+                   weak_ptr_factory_.GetWeakPtr(), suggestion->id(),
+                   suggestion->url(), suggestion->title(),
+                   suggestion->publisher_name()));
   }
 
   void OnCategoryStatusChanged(Category category,
@@ -106,6 +103,24 @@ class ContentSuggestionsNotifierService::NotifyingObserver
   }
 
  private:
+  const ContentSuggestion* GetSuggestionToNotifyAbout(Category category) {
+    const auto& suggestions = service_->GetSuggestionsForCategory(category);
+    if (false /* DO_NOT_SUBMIT: always_notify after crrev.com/2613863003 */) {

[Bernhard Bauer, 2017/01/05 17:31:17]

So, if the feature is enabled, we will always notify about the first suggestion,
but only for ARTICLES, never for any other category, whereas if the feature is
disabled, we will notify for any category that has the notification signal. Is
that intended behavior or should the conditions be jiggled around?

[sfiera, 2017/01/05 19:14:37]

This condition will use AlwaysNotifyAboutContentSuggestions(), which checks a
variation parameter. There are three possibilities: the feature is disabled (in
which case this service is never even created), it's enabled and we use the
server's signal (by default), and the experimental behavior of this branch when
always_notify=true.

AlwaysNotifyAboutContentSuggestions() is a simple helper function, so I just
inlined it with a TODO for now.

[Bernhard Bauer, 2017/01/06 13:26:59]

Yes, but my point was, in the third case (if the always notify experiment is
one), we will always notify about articles, but never about anything else, even
if it has the server's signal. Is that intended?

[sfiera, 2017/01/06 15:39:52]

Oh, I see. Yes, this is the intended behavior. The intent is to replace the
server's signal entirely.

+      if (category.IsKnownCategory(KnownCategories::ARTICLES) &&
+          suggestions.size()) {

[Bernhard Bauer, 2017/01/05 17:31:17]

Maybe use !suggestions.empty()?

[sfiera, 2017/01/05 19:14:37]

Done.

+        return &suggestions[0];
+      }
+      return nullptr;
+    }
+
+    for (const ContentSuggestion& suggestion : suggestions) {
+      if (suggestion.notification_extra()) {
+        return &suggestion;
+      }
+    }
+    return nullptr;
+  }
+
   void ImageFetched(const ContentSuggestion::ID& id,
                     const GURL& url,
                     const base::string16& title,