Only show Last N Pages in the UI when the corresponding tab is visible.

components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.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 "components/ntp_snippets/offline_pages/recent_tab_suggestions_provider.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/test/test_simple_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "components/ntp_snippets/category.h"
 #include "components/ntp_snippets/content_suggestions_provider.h"
 #include "components/ntp_snippets/mock_content_suggestions_provider_observer.h"
 #include "components/ntp_snippets/offline_pages/offline_pages_test_utils.h"
+#include "components/offline_pages/core/background/request_coordinator_stub_taco.h"
 #include "components/offline_pages/core/client_namespace_constants.h"
+#include "components/offline_pages/core/downloads/download_ui_adapter.h"
 #include "components/offline_pages/core/offline_page_item.h"
-#include "components/offline_pages/core/stub_offline_page_model.h"
+#include "components/offline_pages/core/recent_tabs/recent_tabs_ui_adapter_delegate.h"
 #include "components/prefs/testing_pref_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ntp_snippets::test::CaptureDismissedSuggestions;
 using ntp_snippets::test::FakeOfflinePageModel;
 using offline_pages::ClientId;
 using offline_pages::MultipleOfflinePageItemCallback;
 using offline_pages::OfflinePageItem;
-using offline_pages::StubOfflinePageModel;
 using testing::_;
 using testing::IsEmpty;
 using testing::Mock;
 using testing::Property;
 using testing::SizeIs;
 
 namespace ntp_snippets {
 
 namespace {
 
-OfflinePageItem CreateDummyRecentTab(int id) {
-  OfflinePageItem item =
-      test::CreateDummyOfflinePageItem(id, offline_pages::kLastNNamespace);
-  item.client_id.id = base::IntToString(id);
-  return item;
+// This global is used to assign unique tab IDs to pages.  Since offline IDs are
+// typically small integers like 1, 2, 3 etc, we start at 100 to ensure that
+// they are different, and can catch bugs where offline page ID is used in place
+// of tab ID and vice versa.
+int current_tab_id = 100;

[vitaliii, 2017/02/15 10:12:16]

Since tab_id is never checked in the tests explicitly, you could use tab_id =
offline_id + 1000 instead without having a global variable. But still please
leave the comment.

[dewittj, 2017/02/15 19:49:45]

Good idea, done.

+
+OfflinePageItem CreateDummyRecentTab(int offline_id) {
+  std::string tab_id = base::IntToString(++current_tab_id);
+  ClientId client_id(offline_pages::kLastNNamespace, tab_id);
+  return test::CreateDummyOfflinePageItem(offline_id, client_id);
 }
 
 std::vector<OfflinePageItem> CreateDummyRecentTabs(
     const std::vector<int>& ids) {
   std::vector<OfflinePageItem> result;
   for (int id : ids) {
     result.push_back(CreateDummyRecentTab(id));
   }
   return result;
 }
 
 OfflinePageItem CreateDummyRecentTab(int id, base::Time time) {
   OfflinePageItem item = CreateDummyRecentTab(id);
   item.creation_time = time;
   item.last_access_time = time;
   return item;
 }
 
 }  // namespace
 
 class RecentTabSuggestionsProviderTest : public testing::Test {
  public:
   RecentTabSuggestionsProviderTest()
-      : pref_service_(new TestingPrefServiceSimple()) {
+      : task_runner_(new base::TestSimpleTaskRunner()),
+        task_runner_handle_(task_runner_),
+        pref_service_(new TestingPrefServiceSimple()) {
     RecentTabSuggestionsProvider::RegisterProfilePrefs(
         pref_service()->registry());
 
-    provider_.reset(
-        new RecentTabSuggestionsProvider(&observer_, &model_, pref_service()));
+    taco_ = base::MakeUnique<offline_pages::RequestCoordinatorStubTaco>();
+    taco_->CreateRequestCoordinator();
+
+    provider_.reset(new RecentTabSuggestionsProvider(
+        &observer_, &model_, taco_->request_coordinator(), pref_service()));
+
+    ui_adapter_ = offline_pages::RecentTabsUIAdapterDelegate::
+        GetOrCreateRecentTabsUIAdapter(&model_, taco_->request_coordinator());
+    // The UI adapter always fires asynchronously upon loading, so we want to
+    // run past that moment before each test.
+    task_runner_->RunUntilIdle();
+    Mock::VerifyAndClearExpectations(observer());
   }
 
   Category recent_tabs_category() {
     return Category::FromKnownCategory(KnownCategories::RECENT_TABS);
   }
 
   ContentSuggestion::ID GetDummySuggestionId(int id) {
     return ContentSuggestion::ID(recent_tabs_category(), base::IntToString(id));
   }
 
+  void AddTabAndOfflinePageToModel(const OfflinePageItem& item) {
+    AddTab(offline_pages::RecentTabsUIAdapterDelegate::TabIdFromClientId(
+        item.client_id));
+    AddOfflinePageToModel(item);
+  }
+
+  void AddTab(int tab_id) {
+    offline_pages::RecentTabsUIAdapterDelegate* delegate_ =

[vitaliii, 2017/02/15 10:12:15]

Why does this have trailing underscore?
Should we have |delegate_| member and initialize it in the constructor?

[dewittj, 2017/02/15 19:49:45]

Done.

+        offline_pages::RecentTabsUIAdapterDelegate::FromDownloadUIAdapter(
+            ui_adapter_);
+    delegate_->RegisterTab(tab_id);
+  }
+
+  void RemoveTab(int tab_id) {
+    offline_pages::RecentTabsUIAdapterDelegate* delegate_ =
+        offline_pages::RecentTabsUIAdapterDelegate::FromDownloadUIAdapter(
+            ui_adapter_);
+    delegate_->UnregisterTab(tab_id);
+  }
+
   void AddOfflinePageToModel(const OfflinePageItem& item) {
-    model_.mutable_items()->push_back(item);
-    provider_->OfflinePageAdded(&model_, item);
+    ui_adapter_->OfflinePageAdded(&model_, item);
   }
 
   void FireOfflinePageDeleted(const OfflinePageItem& item) {
-    auto iter = std::remove(model_.mutable_items()->begin(),
-                            model_.mutable_items()->end(), item);
-    auto end = model_.mutable_items()->end();
-    model_.mutable_items()->erase(iter, end);
+    ui_adapter_->OfflinePageDeleted(item.offline_id, item.client_id);
 
-    provider_->OfflinePageDeleted(item.offline_id, item.client_id);
+    int tab_id = offline_pages::RecentTabsUIAdapterDelegate::TabIdFromClientId(
+        item.client_id);
+    RemoveTab(tab_id);

[vitaliii, 2017/02/15 10:12:15]

Wouldn't removing tab before |OfflinePageDeleted| be closer to reality?

[dewittj, 2017/02/15 19:49:45]

Done.

   }
 
   std::set<std::string> ReadDismissedIDsFromPrefs() {
     return provider_->ReadDismissedIDsFromPrefs();
   }
 
   RecentTabSuggestionsProvider* provider() { return provider_.get(); }
-  FakeOfflinePageModel* model() { return &model_; }
   MockContentSuggestionsProviderObserver* observer() { return &observer_; }
   TestingPrefServiceSimple* pref_service() { return pref_service_.get(); }
 
  private:
   FakeOfflinePageModel model_;
+  offline_pages::DownloadUIAdapter* ui_adapter_;
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  base::ThreadTaskRunnerHandle task_runner_handle_;
+  std::unique_ptr<offline_pages::RequestCoordinatorStubTaco> taco_;
   MockContentSuggestionsProviderObserver observer_;
   std::unique_ptr<TestingPrefServiceSimple> pref_service_;
   // Last so that the dependencies are deleted after the provider.
   std::unique_ptr<RecentTabSuggestionsProvider> provider_;
 
   DISALLOW_COPY_AND_ASSIGN(RecentTabSuggestionsProviderTest);
 };
 
 TEST_F(RecentTabSuggestionsProviderTest, ShouldConvertToSuggestions) {
-  auto recent_tabs_list = CreateDummyRecentTabs({1, 2});
   EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(2);
-  for (OfflinePageItem& recent_tab : recent_tabs_list)
-    AddOfflinePageToModel(recent_tab);
-
   EXPECT_CALL(
       *observer(),
       OnNewSuggestions(_, recent_tabs_category(),
                        UnorderedElementsAre(
                            Property(&ContentSuggestion::url,
                                     GURL("http://dummy.com/1")),
                            Property(&ContentSuggestion::url,
                                     GURL("http://dummy.com/2")),
                            Property(&ContentSuggestion::url,
                                     GURL("http://dummy.com/3")))));
-  AddOfflinePageToModel(CreateDummyRecentTab(3));
+
+  auto recent_tabs_list = CreateDummyRecentTabs({1, 2, 3});
+  for (OfflinePageItem& recent_tab : recent_tabs_list)

[vitaliii, 2017/02/15 10:12:15]

Please add curly brackets { }. In NTP we use curly brackets even for one line
bodies.

[dewittj, 2017/02/15 19:49:45]

Done.

+    AddTabAndOfflinePageToModel(recent_tab);
 }
 
 TEST_F(RecentTabSuggestionsProviderTest, ShouldSortByCreationTime) {
   base::Time now = base::Time::Now();
   base::Time yesterday = now - base::TimeDelta::FromDays(1);
   base::Time tomorrow = now + base::TimeDelta::FromDays(1);
 
   std::vector<OfflinePageItem> offline_pages = {
       CreateDummyRecentTab(1, now), CreateDummyRecentTab(2, yesterday),
       CreateDummyRecentTab(3, tomorrow)};
 
   EXPECT_CALL(
       *observer(),
       OnNewSuggestions(_, recent_tabs_category(),
                        ElementsAre(Property(&ContentSuggestion::url,
                                             GURL("http://dummy.com/1")))));
-  AddOfflinePageToModel(CreateDummyRecentTab(1, now));
+  AddTabAndOfflinePageToModel(CreateDummyRecentTab(1, now));
 
   EXPECT_CALL(
       *observer(),
       OnNewSuggestions(
           _, recent_tabs_category(),
           ElementsAre(
               Property(&ContentSuggestion::url, GURL("http://dummy.com/1")),
               Property(&ContentSuggestion::url, GURL("http://dummy.com/2")))));
-  AddOfflinePageToModel(CreateDummyRecentTab(2, yesterday));
+  AddTabAndOfflinePageToModel(CreateDummyRecentTab(2, yesterday));
 
   offline_pages[1].last_access_time =
       offline_pages[0].last_access_time + base::TimeDelta::FromHours(1);
 
   EXPECT_CALL(
       *observer(),
       OnNewSuggestions(
           _, recent_tabs_category(),
           ElementsAre(Property(&ContentSuggestion::url,
                                GURL("http://dummy.com/3")),
                       Property(&ContentSuggestion::url,
                                GURL("http://dummy.com/1")),
                       Property(&ContentSuggestion::url,
                                GURL("http://dummy.com/2")))));
-  AddOfflinePageToModel(CreateDummyRecentTab(3, tomorrow));
+  AddTabAndOfflinePageToModel(CreateDummyRecentTab(3, tomorrow));
 }
 
 TEST_F(RecentTabSuggestionsProviderTest, ShouldDeliverCorrectCategoryInfo) {
   EXPECT_FALSE(
       provider()->GetCategoryInfo(recent_tabs_category()).has_fetch_action());
   EXPECT_FALSE(provider()
                    ->GetCategoryInfo(recent_tabs_category())
                    .has_view_all_action());
 }
 
 // TODO(vitaliii): Break this test into multiple tests. Currently if it fails,
 // it takes long time to find which part of it actually fails.
 TEST_F(RecentTabSuggestionsProviderTest, ShouldDismiss) {
   EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(3);
   auto recent_tabs_list = CreateDummyRecentTabs({1, 2, 3});
   for (OfflinePageItem& recent_tab : recent_tabs_list) {
-    AddOfflinePageToModel(recent_tab);
+    AddTabAndOfflinePageToModel(recent_tab);
   }
 
+  Mock::VerifyAndClearExpectations(observer());

[vitaliii, 2017/02/15 10:12:15]

Why is this needed here?

[dewittj, 2017/02/15 19:49:45]

I think I added it during testing. It separates the concerns of the "setup"
phase from the actual testing.  Do you know whether having Times(0) from the
below EXPECT_CALL resets the expected number of calls, or whether it just adds 0
to the expected number of calls?

Will remove for now.

[vitaliii, 2017/02/16 08:11:04]

I guess it implicitly resets the expected number of call.

In this piece of code:

  EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(3);
  AddOfflinePageToModel(CreateDummyRecentTab(2));
  AddOfflinePageToModel(CreateDummyRecentTab(3));
  EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0);
  AddOfflinePageToModel(CreateDummyRecentTab(4));

Both EXPECT_CALLs are unsatisfied in the end.

[tschumann, 2017/02/16 09:22:11]

drive-by:
gmock builds up a stack of expectations. When a new call comes, it traverses the
stack and picks the first one that matches (argument matches). Because you're
using OnNewSuggestions() with the wildcard-matchers ('_'), the last expectation
in the stack will always 'win' -- even if it's already saturated.
So adding a new .Times(0) expectation will simply shadow the older one.

I find this a bit brittle though. If you care about methods not being called, it
might be worth considering using a StrictMock(). However, this might require
changes to existing tests and hence not be suitable for this CL.

In this particular case, I also think the call to VerifyAndClearExpectations is
not necessary.
(also note, that when dealing with interleaving calls, you can also specify
.RetiresOnSaturation() on a EXPECT_CALL() statement which removes an expectation
from the expectation-stack once saturated).

+
   // Dismiss 2 and 3.
   EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0);
   provider()->DismissSuggestion(GetDummySuggestionId(2));
   provider()->DismissSuggestion(GetDummySuggestionId(3));
   Mock::VerifyAndClearExpectations(observer());
 
   // They should disappear from the reported suggestions.
   EXPECT_CALL(
       *observer(),
       OnNewSuggestions(
           _, recent_tabs_category(),
           UnorderedElementsAre(
               Property(&ContentSuggestion::url, GURL("http://dummy.com/1")),
               Property(&ContentSuggestion::url, GURL("http://dummy.com/4")))));
 
-  AddOfflinePageToModel(CreateDummyRecentTab(4));
+  AddTabAndOfflinePageToModel(CreateDummyRecentTab(4));
   Mock::VerifyAndClearExpectations(observer());
 
   // And appear in the dismissed suggestions.
   std::vector<ContentSuggestion> dismissed_suggestions;
   provider()->GetDismissedSuggestionsForDebugging(
       recent_tabs_category(),
       base::Bind(&CaptureDismissedSuggestions, &dismissed_suggestions));
   EXPECT_THAT(
       dismissed_suggestions,
       UnorderedElementsAre(Property(&ContentSuggestion::url,
                                     GURL("http://dummy.com/2")),
                            Property(&ContentSuggestion::url,
                                     GURL("http://dummy.com/3"))));
 
   // Clear dismissed suggestions.
   provider()->ClearDismissedSuggestionsForDebugging(recent_tabs_category());
 
   // They should be gone from the dismissed suggestions.
   dismissed_suggestions.clear();
   provider()->GetDismissedSuggestionsForDebugging(
       recent_tabs_category(),
       base::Bind(&CaptureDismissedSuggestions, &dismissed_suggestions));
   EXPECT_THAT(dismissed_suggestions, IsEmpty());
 
   // And appear in the reported suggestions for the category again.
   EXPECT_CALL(*observer(),
               OnNewSuggestions(_, recent_tabs_category(), SizeIs(5)));
-  AddOfflinePageToModel(CreateDummyRecentTab(5));
+  AddTabAndOfflinePageToModel(CreateDummyRecentTab(5));
   Mock::VerifyAndClearExpectations(observer());
 }
 
 TEST_F(RecentTabSuggestionsProviderTest,
        ShouldInvalidateWhenOfflinePageDeleted) {
   EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(3);
   std::vector<OfflinePageItem> offline_pages = CreateDummyRecentTabs({1, 2, 3});
   for (OfflinePageItem& recent_tab : offline_pages)
-    AddOfflinePageToModel(recent_tab);
+    AddTabAndOfflinePageToModel(recent_tab);
 
   // Invalidation of suggestion 2 should be forwarded.
   EXPECT_CALL(*observer(), OnSuggestionInvalidated(_, GetDummySuggestionId(2)));
   FireOfflinePageDeleted(offline_pages[1]);
 }
 
 TEST_F(RecentTabSuggestionsProviderTest, ShouldClearDismissedOnInvalidate) {
   EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(3);
   std::vector<OfflinePageItem> offline_pages = CreateDummyRecentTabs({1, 2, 3});
   for (OfflinePageItem& recent_tab : offline_pages)
-    AddOfflinePageToModel(recent_tab);
+    AddTabAndOfflinePageToModel(recent_tab);
   EXPECT_THAT(ReadDismissedIDsFromPrefs(), IsEmpty());
 
   provider()->DismissSuggestion(GetDummySuggestionId(2));
   EXPECT_THAT(ReadDismissedIDsFromPrefs(), SizeIs(1));
 
   FireOfflinePageDeleted(offline_pages[1]);
   EXPECT_THAT(ReadDismissedIDsFromPrefs(), IsEmpty());
 }
 
 TEST_F(RecentTabSuggestionsProviderTest, ShouldClearDismissedOnFetch) {
   EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(3);
   std::vector<OfflinePageItem> offline_pages = CreateDummyRecentTabs({1, 2, 3});
   for (OfflinePageItem& recent_tab : offline_pages)
-    AddOfflinePageToModel(recent_tab);
+    AddTabAndOfflinePageToModel(recent_tab);
 
   provider()->DismissSuggestion(GetDummySuggestionId(2));
   provider()->DismissSuggestion(GetDummySuggestionId(3));
   EXPECT_THAT(ReadDismissedIDsFromPrefs(), SizeIs(2));
 
   FireOfflinePageDeleted(offline_pages[0]);
   FireOfflinePageDeleted(offline_pages[2]);
   EXPECT_THAT(ReadDismissedIDsFromPrefs(), SizeIs(1));
 
   FireOfflinePageDeleted(offline_pages[1]);
   EXPECT_THAT(ReadDismissedIDsFromPrefs(), IsEmpty());
 }
 
 TEST_F(RecentTabSuggestionsProviderTest, ShouldNotShowSameUrlMutlipleTimes) {
   base::Time now = base::Time::Now();
   base::Time yesterday = now - base::TimeDelta::FromDays(1);
   base::Time tomorrow = now + base::TimeDelta::FromDays(1);
   std::vector<OfflinePageItem> offline_pages = {
       CreateDummyRecentTab(1, yesterday), CreateDummyRecentTab(2, now),
       CreateDummyRecentTab(3, tomorrow)};
 
   // We leave IDs different, but make the URLs the same.
   offline_pages[2].url = offline_pages[0].url;
 
-  AddOfflinePageToModel(offline_pages[0]);
-  AddOfflinePageToModel(offline_pages[1]);
+  AddTabAndOfflinePageToModel(offline_pages[0]);
+  AddTabAndOfflinePageToModel(offline_pages[1]);
   Mock::VerifyAndClearExpectations(observer());
   EXPECT_CALL(*observer(),
               OnNewSuggestions(
                   _, recent_tabs_category(),
                   UnorderedElementsAre(
                       Property(&ContentSuggestion::publish_date, now),
                       Property(&ContentSuggestion::publish_date, tomorrow))));
 
-  AddOfflinePageToModel(offline_pages[2]);
+  AddTabAndOfflinePageToModel(offline_pages[2]);
 }
 
 TEST_F(RecentTabSuggestionsProviderTest,
        ShouldNotFetchIfAddedOfflinePageIsNotRecentTab) {
-  // The provider is not notified about the first recent tab yet.
-  model()->mutable_items()->push_back(CreateDummyRecentTab(1));
   // It should not fetch when not a recent tab is added, thus, it should not
   // report the first recent tab (which it is not aware about).
   EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0);

[vitaliii, 2017/02/15 10:12:16]

Removal of the recent tab changes meaning a bit. Currently, one may do the
fetch, but not report anything because there is no RecentTabs. Is there any way
to add a recent tab in this test without notifying observers?

[dewittj, 2017/02/15 19:49:45]

It's not clear what exactly you are trying to test.  What behavior are you
trying to simulate by adding a recent tab without notifying?

[vitaliii, 2017/02/16 08:11:04]

Previously, if an offline page is added, the provider should query all pages
only if it was a recent tab. As you said below, now we are notified only about
recent tabs, so please remove this test.

   AddOfflinePageToModel(ntp_snippets::test::CreateDummyOfflinePageItem(
       2, offline_pages::kDefaultNamespace));
 }
 
 TEST_F(RecentTabSuggestionsProviderTest,
        ShouldFetchIfAddedOfflinePageIsRecentTab) {
   // The provider is not notified about the first recent tab yet.

[vitaliii, 2017/02/15 10:12:15]

I am confused. In the test above you removed exactly the same line. Is there any
difference between these two tests?

Also |AddTabAndOfflinePageToModel| contradicts the comment - it (implicitly)
notifies the provider.

[dewittj, 2017/02/15 19:49:45]

I think the premise of this test doesn't exist any more, you are always notified
of recent tabs now. What I didn't test was the behavior on load, so changed this
test to reflect that.

[vitaliii, 2017/02/16 08:11:03]

ACK.

Yes, fair enough!

-  model()->mutable_items()->push_back(CreateDummyRecentTab(1));
+  AddTabAndOfflinePageToModel(CreateDummyRecentTab(1));
   // However, it must return the first recent tab (i.e. manually fetch it) even
   // when notified about a different recent tab.
   EXPECT_CALL(
       *observer(),
       OnNewSuggestions(
           _, recent_tabs_category(),
           UnorderedElementsAre(
               Property(&ContentSuggestion::url, GURL("http://dummy.com/1")),
               Property(&ContentSuggestion::url, GURL("http://dummy.com/2")))));
-  AddOfflinePageToModel(CreateDummyRecentTab(2));
+  AddTabAndOfflinePageToModel(CreateDummyRecentTab(2));
+}
+
+TEST_F(RecentTabSuggestionsProviderTest, ShouldNotShowPagesWithoutTab) {

[vitaliii, 2017/02/15 10:12:15]

Could you also test that InvalidateSuggestion is fired on tab removal? Probably
in a different test.

[dewittj, 2017/02/15 19:49:45]

Done.

+  EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0);
+  // The provider is not notified about the first recent tab yet (no tab).
+  OfflinePageItem tab = CreateDummyRecentTab(1);
+  AddOfflinePageToModel(tab);

[vitaliii, 2017/02/15 10:12:15]

I guess this actually notifies the provider and so contradicts the comment.

[dewittj, 2017/02/15 19:49:45]

No, this doesn't add the tab, but the name of the offline page item is
misleading.

+
+  Mock::VerifyAndClearExpectations(observer());
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(1);
+  EXPECT_CALL(
+      *observer(),
+      OnNewSuggestions(
+          _, recent_tabs_category(),
+          UnorderedElementsAre(
+              Property(&ContentSuggestion::url, GURL("http://dummy.com/1")),
+              Property(&ContentSuggestion::url, GURL("http://dummy.com/2")))));
+
+  AddTab(offline_pages::RecentTabsUIAdapterDelegate::TabIdFromClientId(
+      tab.client_id));
+  OfflinePageItem tab2 = CreateDummyRecentTab(2);

[vitaliii, 2017/02/15 10:12:16]

Could you please use "first_tab" and "second_tab" variable names?

[dewittj, 2017/02/15 19:49:45]

Done.

+  AddTabAndOfflinePageToModel(tab2);
+
+  Mock::VerifyAndClearExpectations(observer());
+
+  EXPECT_CALL(
+      *observer(),
+      OnNewSuggestions(
+          _, recent_tabs_category(),
+          UnorderedElementsAre(
+              Property(&ContentSuggestion::url, GURL("http://dummy.com/1")),
+              Property(&ContentSuggestion::url, GURL("http://dummy.com/3")))));
+
+  // Remove Tab by itself doesn't cause OnNewSuggestions to be called.

[vitaliii, 2017/02/15 10:12:15]

|RemoveTab| or "Tab removal"

If it does not cause OnNewSuggestions, please move EXPECT_CALL(OnNewSuggestions
...) after it.

[dewittj, 2017/02/15 19:49:45]

Done.
I added another expectation in addition.

+  RemoveTab(offline_pages::RecentTabsUIAdapterDelegate::TabIdFromClientId(
+      tab2.client_id));
+  AddTabAndOfflinePageToModel(CreateDummyRecentTab(3));
 }
 
 }  // namespace ntp_snippets