Use server signal to decide whether to notify

chrome/browser/android/ntp/content_suggestions_notifier_service.cc

 // 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.
 
 #include "chrome/browser/android/ntp/content_suggestions_notifier_service.h"
 
 #include <algorithm>
 
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/android/ntp/content_suggestions_notification_helper.h"
@@ skipping 35 matching lines @@
 
 class ContentSuggestionsNotifierService::NotifyingObserver
     : public ContentSuggestionsService::Observer {
  public:
   NotifyingObserver(ContentSuggestionsService* service,
                     Profile* profile,
                     PrefService* prefs)
       : 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,
                                CategoryStatus new_status) override {
     if (!category.IsKnownCategory(KnownCategories::ARTICLES)) {
       return;
     }
     switch (new_status) {
       case CategoryStatus::AVAILABLE:
       case CategoryStatus::AVAILABLE_LOADING:
@@ skipping 20 matching lines @@
 
   void OnFullRefreshRequired() override {
     ContentSuggestionsNotificationHelper::HideNotification();
   }
 
   void ContentSuggestionsServiceShutdown() override {
     ContentSuggestionsNotificationHelper::HideNotification();
   }
 
  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,
                     const base::string16& publisher,
                     const gfx::Image& image) {
     // check if suggestion is still valid.
     DVLOG(1) << "Fetched " << image.Size().width() << "x"
              << image.Size().height() << " image for " << url.spec();
     prefs_->SetString(kNotificationIDWithinCategory, id.id_within_category());
     ContentSuggestionsNotificationHelper::SendNotification(
@@ skipping 18 matching lines @@
   suggestions->AddObserver(observer_.get());
 }
 
 ContentSuggestionsNotifierService::~ContentSuggestionsNotifierService() =
     default;
 
 void ContentSuggestionsNotifierService::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterStringPref(kNotificationIDWithinCategory, std::string());
 }