Remove deleted offline page suggestions from opened NTPs

Add OnSuggestionInvalidated methods to the ContentSuggestionsProvider,
the ContentSuggestionsService and its observer and to the SnippetsBridge.

Remove suggestions from the UI immediately when they're invalidated.

Invalidate offline page suggestions when the referenced offline page is
deleted, and also clear it from the dismissed_ids list.

Add new unit tests for the UI layer and the service layer.

BUG=628198

Review-Url: https://codereview.chromium.org/2244793002
Cr-Commit-Position: refs/heads/master@{#412813}

chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java

@@ -203,6 +203,13 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder>
         updateGroups();
     }
 
+    @Override
+    public void onSuggestionInvalidated(@CategoryInt int category, String suggestionId) {
+        if (!mSections.containsKey(category)) return;
+        mSections.get(category).removeSuggestionById(suggestionId);
+        updateGroups();
+    }
+
     @Override
     @NewTabPageItem.ViewType
     public int getItemViewType(int position) {
@@ -348,7 +355,7 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder>
 
         mSuggestionsSource.dismissSuggestion(suggestion);
         SuggestionsSection section = (SuggestionsSection) getGroup(position);
-        section.dismissSuggestion(suggestion);
+        section.removeSuggestion(suggestion);
 
         if (section.hasSuggestions()) {
             // If one of many suggestions was dismissed, it's a simple item removal, which can be

chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java

@@ -57,10 +57,19 @@ public class SuggestionsSection implements ItemGroup {
         return Collections.unmodifiableList(items);
     }
 
-    public void dismissSuggestion(SnippetArticle suggestion) {
+    public void removeSuggestion(SnippetArticle suggestion) {
         mSuggestions.remove(suggestion);
     }
 
+    public void removeSuggestionById(String suggestionId) {
+        for (SnippetArticle suggestion : mSuggestions) {
+            if (suggestion.mId.equals(suggestionId)) {
+                removeSuggestion(suggestion);
+                return;
+            }
+        }
+    }
+
     public boolean hasSuggestions() {
         return !mSuggestions.isEmpty();
     }

chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java

@@ -167,6 +167,11 @@ public class SnippetsBridge implements SuggestionsSource {
         if (mObserver != null) mObserver.onCategoryStatusChanged(category, newStatus);
     }
 
+    @CalledByNative
+    private void onSuggestionInvalidated(/* @CategoryInt */ int category, String suggestionId) {
+        if (mObserver != null) mObserver.onSuggestionInvalidated(category, suggestionId);
+    }
+
     private native long nativeInit(Profile profile);
     private native void nativeDestroy(long nativeNTPSnippetsBridge);
     private static native void nativeFetchSnippets(boolean forceRequest);

chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java

@@ -21,10 +21,17 @@ public interface SuggestionsSource {
      */
     interface Observer {
         /** Called when a category has a new list of content suggestions. */
-        void onNewSuggestions(int category);
+        void onNewSuggestions(@CategoryInt int category);
 
         /** Called when a category changed its status. */
-        void onCategoryStatusChanged(int category, @CategoryStatusEnum int newStatus);
+        void onCategoryStatusChanged(@CategoryInt int category, @CategoryStatusEnum int newStatus);
+
+        /**
+         * Called when a suggestion is invalidated, which means it needs to be removed from the UI
+         * immediately. This event may be fired for a category or suggestion that does not
+         * currently exist or has never existed and should be ignored in that case.
+         */
+        void onSuggestionInvalidated(@CategoryInt int category, String suggestionId);
     }
 
     /**

chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java

@@ -66,6 +66,16 @@ public class NewTabPageAdapterTest {
             mCategoryInfo.put(category, info);
         }
 
+        public void fireSuggestionInvalidated(@CategoryInt int category, String suggestionId) {
+            for (SnippetArticle suggestion : mSuggestions.get(category)) {
+                if (suggestion.mId.equals(suggestionId)) {
+                    mSuggestions.get(category).remove(suggestion);
+                    break;
+                }
+            }
+            mObserver.onSuggestionInvalidated(category, suggestionId);
+        }
+
         public void silentlyRemoveCategory(int category) {
             mSuggestions.remove(category);
             mCategoryStatus.remove(category);
@@ -417,6 +427,9 @@ public class NewTabPageAdapterTest {
         assertItemsFor();
     }
 
+    /**
+     * Tests that the more button is shown for sections that declare it.
+     */
     @Test
     @Feature({"Ntp"})
     public void testMoreButton() {
@@ -434,6 +447,23 @@ public class NewTabPageAdapterTest {
         assertItemsFor(sectionWithMoreButton(10), section(3));
     }
 
+    /**
+     * Tests that invalidated suggestions are immediately removed.
+     */
+    @Test
+    @Feature({"Ntp"})
+    public void testSuggestionInvalidated() {
+        List<SnippetArticle> articles = createDummySnippets(3);
+        mSnippetsSource.setStatusForCategory(KnownCategories.ARTICLES, CategoryStatus.AVAILABLE);
+        mSnippetsSource.setSuggestionsForCategory(KnownCategories.ARTICLES, articles);
+        assertItemsFor(section(3));
+        assertEquals(articles, mNtpAdapter.getItems().subList(2, 5));
+
+        SnippetArticle removed = articles.remove(1);
+        mSnippetsSource.fireSuggestionInvalidated(KnownCategories.ARTICLES, removed.mId);
+        assertEquals(articles, mNtpAdapter.getItems().subList(2, 4));
+    }
+
     private List<SnippetArticle> createDummySnippets(int count) {
         List<SnippetArticle> snippets = new ArrayList<>();
         for (int index = 0; index < count; index++) {

chrome/browser/android/ntp/ntp_snippets_bridge.cc

@@ -228,6 +228,18 @@ void NTPSnippetsBridge::OnCategoryStatusChanged(Category category,
                                               static_cast<int>(new_status));
 }
 
+void NTPSnippetsBridge::OnSuggestionInvalidated(
+    Category category,
+    const std::string& suggestion_id) {
+  if (observer_.is_null())
+    return;
+
+  JNIEnv* env = base::android::AttachCurrentThread();
+  Java_SnippetsBridge_onSuggestionInvalidated(
+      env, observer_.obj(), static_cast<int>(category.id()),
+      ConvertUTF8ToJavaString(env, suggestion_id).obj());
+}
+
 void NTPSnippetsBridge::ContentSuggestionsServiceShutdown() {
   observer_.Reset();
   content_suggestions_service_observer_.Remove(content_suggestions_service_);

chrome/browser/android/ntp/ntp_snippets_bridge.h

@@ -79,6 +79,8 @@ class NTPSnippetsBridge
   void OnCategoryStatusChanged(
       ntp_snippets::Category category,
       ntp_snippets::CategoryStatus new_status) override;
+  void OnSuggestionInvalidated(ntp_snippets::Category category,
+                               const std::string& suggestion_id) override;
   void ContentSuggestionsServiceShutdown() override;
 
   void OnImageFetched(base::android::ScopedJavaGlobalRef<jobject> callback,

chrome/browser/ui/webui/snippets_internals_message_handler.cc

@@ -128,6 +128,14 @@ void SnippetsInternalsMessageHandler::OnCategoryStatusChanged(
   SendContentSuggestions();
 }
 
+void SnippetsInternalsMessageHandler::OnSuggestionInvalidated(
+    ntp_snippets::Category category,
+    const std::string& suggestion_id) {
+  if (!dom_loaded_)
+    return;
+  SendContentSuggestions();
+}
+
 void SnippetsInternalsMessageHandler::ContentSuggestionsServiceShutdown() {}
 
 void SnippetsInternalsMessageHandler::HandleRefreshContent(

chrome/browser/ui/webui/snippets_internals_message_handler.h

@@ -36,6 +36,8 @@ class SnippetsInternalsMessageHandler
   void OnCategoryStatusChanged(
       ntp_snippets::Category category,
       ntp_snippets::CategoryStatus new_status) override;
+  void OnSuggestionInvalidated(ntp_snippets::Category category,
+                               const std::string& suggestion_id) override;
   void ContentSuggestionsServiceShutdown() override;
 
   void HandleRefreshContent(const base::ListValue* args);

components/ntp_snippets/category_factory.h

@@ -13,7 +13,7 @@
 
 namespace ntp_snippets {
 
-// Creates and orders Category instancesCategory.
+// Creates and orders Category instances.
 class CategoryFactory {
  public:
   CategoryFactory();

components/ntp_snippets/content_suggestions_provider.h

@@ -64,6 +64,19 @@ class ContentSuggestionsProvider {
     virtual void OnCategoryStatusChanged(ContentSuggestionsProvider* provider,
                                          Category category,
                                          CategoryStatus new_status) = 0;
+
+    // Called when a suggestion has been invalidated. It will not be provided
+    // through |OnNewSuggestions| anymore, is not supported by
+    // |FetchSuggestionImage| or |DismissSuggestion| anymore, and should
+    // immediately be cleared from the UI and caches. This happens, for example,
+    // when the content that the suggestion refers to is gone.
+    // Note that this event may be fired even if the corresponding |category| is
+    // not currently AVAILABLE, because open UIs may still be showing the
+    // suggestion that is to be removed. This event may also be fired for
+    // |suggestion_id|s that never existed and should be ignored in that case.
+    virtual void OnSuggestionInvalidated(ContentSuggestionsProvider* provider,
+                                         Category category,
+                                         const std::string& suggestion_id) = 0;
   };
 
   virtual ~ContentSuggestionsProvider();

components/ntp_snippets/content_suggestions_service.cc

@@ -135,19 +135,10 @@ void ContentSuggestionsService::DismissSuggestion(
   providers_by_category_[category]->DismissSuggestion(suggestion_id);
 
   // Remove the suggestion locally.
-  id_category_map_.erase(suggestion_id);
-  std::vector<ContentSuggestion>* suggestions =
-      &suggestions_by_category_[category];
-  auto position =
-      std::find_if(suggestions->begin(), suggestions->end(),
-                   [&suggestion_id](const ContentSuggestion& suggestion) {
-                     return suggestion_id == suggestion.id();
-                   });
-  DCHECK(position != suggestions->end())
-      << "The dismissed suggestion " << suggestion_id
-      << " has already been removed. Providers must not call OnNewSuggestions"
-         " in response to DismissSuggestion.";
-  suggestions->erase(position);
+  bool removed = RemoveSuggestionByID(category, suggestion_id);
+  DCHECK(removed) << "The dismissed suggestion " << suggestion_id
+                  << " has already been removed. Providers must not call"
+                  << " OnNewSuggestions in response to DismissSuggestion.";
 }
 
 void ContentSuggestionsService::AddObserver(Observer* observer) {
@@ -221,6 +212,15 @@ void ContentSuggestionsService::OnCategoryStatusChanged(
   NotifyCategoryStatusChanged(category);
 }
 
+void ContentSuggestionsService::OnSuggestionInvalidated(
+    ContentSuggestionsProvider* provider,
+    Category category,
+    const std::string& suggestion_id) {
+  RemoveSuggestionByID(category, suggestion_id);
+  FOR_EACH_OBSERVER(Observer, observers_,
+                    OnSuggestionInvalidated(category, suggestion_id));
+}
+
 bool ContentSuggestionsService::RegisterCategoryIfRequired(
     ContentSuggestionsProvider* provider,
     Category category) {
@@ -243,6 +243,23 @@ bool ContentSuggestionsService::RegisterCategoryIfRequired(
   return true;
 }
 
+bool ContentSuggestionsService::RemoveSuggestionByID(
+    Category category,
+    const std::string& suggestion_id) {
+  id_category_map_.erase(suggestion_id);
+  std::vector<ContentSuggestion>* suggestions =
+      &suggestions_by_category_[category];
+  auto position =
+      std::find_if(suggestions->begin(), suggestions->end(),
+                   [&suggestion_id](const ContentSuggestion& suggestion) {
+                     return suggestion_id == suggestion.id();
+                   });
+  if (position == suggestions->end())
+    return false;
+  suggestions->erase(position);
+  return true;
+}
+
 void ContentSuggestionsService::NotifyCategoryStatusChanged(Category category) {
   FOR_EACH_OBSERVER(
       Observer, observers_,

components/ntp_snippets/content_suggestions_service.h

@@ -50,6 +50,16 @@ class ContentSuggestionsService : public KeyedService,
     virtual void OnCategoryStatusChanged(Category category,
                                          CategoryStatus new_status) = 0;
 
+    // Fired when a suggestion has been invalidated. The UI must immediately
+    // clear the suggestion even from open NTPs. Invalidation happens, for
+    // example, when the content that the suggestion refers to is gone.
+    // Note that this event may be fired even if the corresponding |category| is
+    // not currently AVAILABLE, because open UIs may still be showing the
+    // suggestion that is to be removed. This event may also be fired for
+    // |suggestion_id|s that never existed and should be ignored in that case.
+    virtual void OnSuggestionInvalidated(Category category,
+                                         const std::string& suggestion_id) = 0;
+
     // Sent when the service is shutting down. After the service has shut down,
     // it will not provide any data anymore, though calling the getters is still
     // safe.
@@ -155,6 +165,9 @@ class ContentSuggestionsService : public KeyedService,
   void OnCategoryStatusChanged(ContentSuggestionsProvider* provider,
                                Category category,
                                CategoryStatus new_status) override;
+  void OnSuggestionInvalidated(ContentSuggestionsProvider* provider,
+                               Category category,
+                               const std::string& suggestion_id) override;
 
   // Registers the given |provider| for the given |category|, unless it is
   // already registered. Returns true if the category was newly registered or
@@ -162,6 +175,12 @@ class ContentSuggestionsService : public KeyedService,
   bool RegisterCategoryIfRequired(ContentSuggestionsProvider* provider,
                                   Category category);
 
+  // Removes a suggestion from the local stores |id_category_map_| and
+  // |suggestions_by_category_|, if it exists. Returns true if a suggestion was
+  // removed.
+  bool RemoveSuggestionByID(Category category,
+                            const std::string& suggestion_id);
+
   // Fires the OnCategoryStatusChanged event for the given |category|.
   void NotifyCategoryStatusChanged(Category category);
 

components/ntp_snippets/content_suggestions_service_unittest.cc

@@ -87,6 +87,11 @@ class MockProvider : public ContentSuggestionsProvider {
     observer()->OnCategoryStatusChanged(this, category, new_status);
   }
 
+  void FireSuggestionInvalidated(Category category,
+                                 const std::string& suggestion_id) {
+    observer()->OnSuggestionInvalidated(this, category, suggestion_id);
+  }
+
   MOCK_METHOD1(ClearCachedSuggestionsForDebugging, void(Category category));
   MOCK_METHOD1(GetDismissedSuggestionsForDebugging,
                std::vector<ContentSuggestion>(Category category));
@@ -106,6 +111,8 @@ class MockServiceObserver : public ContentSuggestionsService::Observer {
   MOCK_METHOD1(OnNewSuggestions, void(Category category));
   MOCK_METHOD2(OnCategoryStatusChanged,
                void(Category changed_category, CategoryStatus new_status));
+  MOCK_METHOD2(OnSuggestionInvalidated,
+               void(Category category, const std::string& suggestion_id));
   MOCK_METHOD0(ContentSuggestionsServiceShutdown, void());
   ~MockServiceObserver() override {}
 };
@@ -261,7 +268,7 @@ TEST_F(ContentSuggestionsServiceTest, ShouldRedirectFetchSuggestionImage) {
   provider1->FireSuggestionsChanged(articles_category, {1});
   std::string suggestion_id = CreateSuggestion(1).id();
 
-  EXPECT_CALL(*provider1, FetchSuggestionImage(suggestion_id, _)).Times(1);
+  EXPECT_CALL(*provider1, FetchSuggestionImage(suggestion_id, _));
   EXPECT_CALL(*provider2, FetchSuggestionImage(_, _)).Times(0);
   service()->FetchSuggestionImage(
       suggestion_id, base::Bind(&ContentSuggestionsServiceTest::OnImageFetched,
@@ -295,10 +302,39 @@ TEST_F(ContentSuggestionsServiceTest, ShouldRedirectDismissSuggestion) {
   std::string suggestion_id = CreateSuggestion(11).id();
 
   EXPECT_CALL(*provider1, DismissSuggestion(_)).Times(0);
-  EXPECT_CALL(*provider2, DismissSuggestion(suggestion_id)).Times(1);
+  EXPECT_CALL(*provider2, DismissSuggestion(suggestion_id));
   service()->DismissSuggestion(suggestion_id);
 }
 
+TEST_F(ContentSuggestionsServiceTest, ShouldRedirectSuggestionInvalidated) {
+  Category articles_category = FromKnownCategory(KnownCategories::ARTICLES);
+
+  MockProvider* provider = MakeProvider(articles_category);
+  MockServiceObserver observer;
+  service()->AddObserver(&observer);
+
+  provider->FireSuggestionsChanged(articles_category, {11, 12, 13});
+  ExpectThatSuggestionsAre(articles_category, {11, 12, 13});
+
+  std::string suggestion_id = CreateSuggestion(12).id();
+  EXPECT_CALL(observer,
+              OnSuggestionInvalidated(articles_category, suggestion_id));
+  provider->FireSuggestionInvalidated(articles_category, suggestion_id);
+  ExpectThatSuggestionsAre(articles_category, {11, 13});
+  Mock::VerifyAndClearExpectations(&observer);
+
+  // Unknown IDs must be forwarded (though no change happens to the service's
+  // internal data structures) because previously opened UIs, which can still
+  // show the invalidated suggestion, must be notified.
+  std::string unknown_id = CreateSuggestion(1234).id();
+  EXPECT_CALL(observer, OnSuggestionInvalidated(articles_category, unknown_id));
+  provider->FireSuggestionInvalidated(articles_category, unknown_id);
+  ExpectThatSuggestionsAre(articles_category, {11, 13});
+  Mock::VerifyAndClearExpectations(&observer);
+
+  service()->RemoveObserver(&observer);
+}
+
 TEST_F(ContentSuggestionsServiceTest, ShouldForwardSuggestions) {
   Category articles_category = FromKnownCategory(KnownCategories::ARTICLES);
   Category offline_pages_category =
@@ -315,27 +351,27 @@ TEST_F(ContentSuggestionsServiceTest, ShouldForwardSuggestions) {
   service()->AddObserver(&observer);
 
   // Send suggestions 1 and 2
-  EXPECT_CALL(observer, OnNewSuggestions(articles_category)).Times(1);
+  EXPECT_CALL(observer, OnNewSuggestions(articles_category));
   provider1->FireSuggestionsChanged(articles_category, {1, 2});
   ExpectThatSuggestionsAre(articles_category, {1, 2});
   Mock::VerifyAndClearExpectations(&observer);
 
   // Send them again, make sure they're not reported twice
-  EXPECT_CALL(observer, OnNewSuggestions(articles_category)).Times(1);
+  EXPECT_CALL(observer, OnNewSuggestions(articles_category));
   provider1->FireSuggestionsChanged(articles_category, {1, 2});
   ExpectThatSuggestionsAre(articles_category, {1, 2});
   ExpectThatSuggestionsAre(offline_pages_category, std::vector<int>());
   Mock::VerifyAndClearExpectations(&observer);
 
   // Send suggestions 13 and 14
-  EXPECT_CALL(observer, OnNewSuggestions(offline_pages_category)).Times(1);
+  EXPECT_CALL(observer, OnNewSuggestions(offline_pages_category));
   provider2->FireSuggestionsChanged(offline_pages_category, {13, 14});
   ExpectThatSuggestionsAre(articles_category, {1, 2});
   ExpectThatSuggestionsAre(offline_pages_category, {13, 14});
   Mock::VerifyAndClearExpectations(&observer);
 
   // Send suggestion 1 only
-  EXPECT_CALL(observer, OnNewSuggestions(articles_category)).Times(1);
+  EXPECT_CALL(observer, OnNewSuggestions(articles_category));
   provider1->FireSuggestionsChanged(articles_category, {1});
   ExpectThatSuggestionsAre(articles_category, {1});
   ExpectThatSuggestionsAre(offline_pages_category, {13, 14});
@@ -344,8 +380,7 @@ TEST_F(ContentSuggestionsServiceTest, ShouldForwardSuggestions) {
   // provider2 reports BOOKMARKS as unavailable
   EXPECT_CALL(observer, OnCategoryStatusChanged(
                             offline_pages_category,
-                            CategoryStatus::CATEGORY_EXPLICITLY_DISABLED))
-      .Times(1);
+                            CategoryStatus::CATEGORY_EXPLICITLY_DISABLED));
   provider2->FireCategoryStatusChanged(
       offline_pages_category, CategoryStatus::CATEGORY_EXPLICITLY_DISABLED);
   EXPECT_THAT(service()->GetCategoryStatus(articles_category),

components/ntp_snippets/mock_content_suggestions_provider_observer.h

@@ -37,6 +37,10 @@ class MockContentSuggestionsProviderObserver
                void(ContentSuggestionsProvider* provider,
                     Category category,
                     CategoryStatus new_status));
+  MOCK_METHOD3(OnSuggestionInvalidated,
+               void(ContentSuggestionsProvider* provider,
+                    Category category,
+                    const std::string& suggestion_id));
 };
 
 }  // namespace ntp_snippets

components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc

@@ -216,7 +216,38 @@ void OfflinePageSuggestionsProvider::OfflinePageModelChanged(
 void OfflinePageSuggestionsProvider::OfflinePageDeleted(
     int64_t offline_id,
     const offline_pages::ClientId& client_id) {
-  // TODO(pke): Implement, suggestion has to be removed from UI immediately.
+  // Because we never switch to NOT_PROVIDED dynamically, there can be no open
+  // UI containing an invalidated suggestion unless the status is something
+  // other than NOT_PROVIDED, so only notify invalidation in that case.
+  std::string offline_page_id = base::IntToString(offline_id);
+  if (recent_tabs_status_ != CategoryStatus::NOT_PROVIDED &&
+      client_id.name_space == offline_pages::kLastNNamespace) {
+    auto it = std::find(dismissed_recent_tab_ids_.begin(),
+                        dismissed_recent_tab_ids_.end(), offline_page_id);
+    if (it == dismissed_recent_tab_ids_.end()) {
+      observer()->OnSuggestionInvalidated(
+          this, recent_tabs_category_,
+          MakeUniqueID(recent_tabs_category_, offline_page_id));
+    } else {
+      dismissed_recent_tab_ids_.erase(it);
+      StoreDismissedIDsToPrefs(prefs::kDismissedRecentOfflineTabSuggestions,
+                               dismissed_recent_tab_ids_);
+    }
+  } else if (downloads_status_ != CategoryStatus::NOT_PROVIDED &&
+             client_id.name_space == offline_pages::kAsyncNamespace &&
+             base::IsValidGUID(client_id.id)) {
+    auto it = std::find(dismissed_download_ids_.begin(),
+                        dismissed_download_ids_.end(), offline_page_id);
+    if (it == dismissed_download_ids_.end()) {
+      observer()->OnSuggestionInvalidated(
+          this, downloads_category_,
+          MakeUniqueID(downloads_category_, offline_page_id));
+    } else {
+      dismissed_download_ids_.erase(it);
+      StoreDismissedIDsToPrefs(prefs::kDismissedDownloadSuggestions,
+                               dismissed_download_ids_);
+    }
+  }
 }
 
 void OfflinePageSuggestionsProvider::FetchOfflinePages() {
@@ -245,6 +276,7 @@ void OfflinePageSuggestionsProvider::OnOfflinePagesLoaded(
 
     // TODO(pke): Use kDownloadNamespace once the OfflinePageModel uses that.
     // The current logic is taken from DownloadUIAdapter::IsVisibleInUI.
+    // Note: This is also copied in OfflinePageDeleted above.
     if (need_downloads &&
         item.client_id.name_space == offline_pages::kAsyncNamespace &&
         base::IsValidGUID(item.client_id.id) &&