Last_n: do not save snapshot of custom tabs.

chrome/browser/android/offline_pages/recent_tab_helper_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 "chrome/browser/android/offline_pages/recent_tab_helper.h"
 
 #include <memory>
 
 #include "base/memory/ptr_util.h"
 #include "base/strings/string16.h"
@@ skipping 35 matching lines @@
       OfflinePageTestArchiver::Observer* observer,
       int tab_id,
       bool tab_id_result);
   ~TestDelegate() override {}
 
   std::unique_ptr<OfflinePageArchiver> CreatePageArchiver(
         content::WebContents* web_contents) override;
     // There is no expectations that tab_id is always present.
   bool GetTabId(content::WebContents* web_contents, int* tab_id) override;
   bool IsLowEndDevice() override { return is_low_end_device_; }
+  bool IsCustomTab(content::WebContents* web_contents) override {
+    return is_custom_tab_;
+  }
 
   void set_archive_result(
       offline_pages::OfflinePageArchiver::ArchiverResult result) {
     archive_result_ = result;
   }
 
   void set_archive_size(int64_t size) { archive_size_ = size; }
 
   void SetAsLowEndDevice() { is_low_end_device_ = true; }
 
+  void set_is_custom_tab(bool is_custom_tab) { is_custom_tab_ = is_custom_tab; }
+
  private:
   OfflinePageTestArchiver::Observer* observer_;  // observer owns this.
   int tab_id_;
   bool tab_id_result_;
 
   // These values can be updated so that new OfflinePageTestArchiver instances
   // will return different results.
   offline_pages::OfflinePageArchiver::ArchiverResult archive_result_ =
       offline_pages::OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED;
   int64_t archive_size_ = kArchiveSizeToReport;
   bool is_low_end_device_ = false;
+  bool is_custom_tab_ = false;
 };
 
 class RecentTabHelperTest
     : public ChromeRenderViewHostTestHarness,
       public OfflinePageModel::Observer,
       public OfflinePageTestArchiver::Observer {
  public:
   RecentTabHelperTest();
   ~RecentTabHelperTest() override {}
 
@@ skipping 275 matching lines @@
   ASSERT_EQ(0U, GetAllPages().size());
 
   // But the following download request should work normally
   recent_tab_helper()->ObserveAndDownloadCurrentPage(NewDownloadClientId(),
                                                      123L);
   RunUntilIdle();
   EXPECT_EQ(1U, page_added_count());
   ASSERT_EQ(1U, GetAllPages().size());
 }
 
+// Checks that last_n will not save a snapshot if the tab while the tab is
+// presented as a custom tab. Download requests should be unaffected though.
+TEST_F(RecentTabHelperTest, LastNWontSaveCustomTab) {
+  // Simulates the tab running as a custom tab.
+  default_test_delegate()->set_is_custom_tab(true);
+
+  // Navigate and finish loading then hide the tab. Nothing should be saved.
+  NavigateAndCommit(kTestPageUrl);
+  recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
+  FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
+  RunUntilIdle();
+  EXPECT_TRUE(model()->is_loaded());
+  EXPECT_EQ(0U, page_added_count());
+  ASSERT_EQ(0U, GetAllPages().size());
+
+  // But the following download request should work normally
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(NewDownloadClientId(),
+                                                     123L);
+  RunUntilIdle();
+  EXPECT_EQ(1U, page_added_count());
+  ASSERT_EQ(1U, GetAllPages().size());
+
+  // Simulates the tab being transfered from the CustomTabActivity back to a
+  // ChromeActivity.
+  default_test_delegate()->set_is_custom_tab(false);
+
+  // Upon the next hide a last_n snapshot should be saved.
+  recent_tab_helper()->WasHidden();
+  RunUntilIdle();
+  EXPECT_TRUE(model()->is_loaded());
+  EXPECT_EQ(2U, page_added_count());
+  ASSERT_EQ(2U, GetAllPages().size());
+}
+

[dewittj, 2017/03/27 20:37:45]

Is there a way to test with a real TabAndroid, and tell it to go from CustomTab
to NotCustomTab?  Could be an integration test.  This would be a nice safeguard
against unexpected WasHidden events.

[gone, 2017/03/27 22:22:10]

You could probably do this using a Java test... could be potentially really
flaky though because you'd need to start and track two different Android
Activities.

[Yusuf, 2017/03/27 22:28:27]

See CustomTabActivityTest#testTabReparentingBasic and the other ones there for
examples. Maybe you can call this API there and assert it gives the right value
before and after?

[carlosk, 2017/03/28 02:00:32]

I added asserts to CustomTabActivityTestBase.startCustomTabActivityWithIntent
and CustomTabActivityTest.reparentAndVerifyTab.

dewittj@: I think these changes should cover your concern. As long as that
method returns the correct state, WasHidden can be called at will. But while
updating these tests I found out that upon Activity reparenting there is a
specific assert to verify that WasHidden is *not* called [1]. So we should be
double-safe here.

yusufo@: should I also change CustomTabActivityTest.testOpenInBrowser to run
those checks? I experienced with it and it seemed it would make it very similar
to reparentAndVerifyTab. WDYT?


[1]
https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu...

[Yusuf, 2017/03/28 17:17:54]

testOpenInBrowser actually tests the default behavior only (where we dont have
any default browser set on the bot), so "Open in Browser" means no default is
set. "Open in Chrome" means the default is Chrome. So reparenting wont happen in
that test.

[carlosk, 2017/03/28 23:26:07]

I see. It's confusing though that in the comment of that test it is mentioned
"Open in Chrome".

As I couldn't find any test specifically named after Chrome I assumed you were
satisfied with the test changes as they are. Let me know if I missed something.

 // Triggers two last_n snapshot captures during a single page load. Should end
 // up with one snapshot, the 1st being replaced by the 2nd.
 TEST_F(RecentTabHelperTest, TwoCapturesSamePageLoad) {
   NavigateAndCommit(kTestPageUrl);
   // Set page loading state to the 1st snapshot-able stage. No capture so far.
   recent_tab_helper()->DocumentAvailableInMainFrame();
   FastForwardSnapshotController();
   EXPECT_TRUE(model()->is_loaded());
   EXPECT_EQ(0U, page_added_count());
 
@@ skipping 516 matching lines @@
   // moment a snapshot should be created.
   recent_tab_helper()->WasShown();
   recent_tab_helper()->WasHidden();
   RunUntilIdle();
   EXPECT_EQ(1U, page_added_count());
   EXPECT_EQ(0U, model_removed_count());
   ASSERT_EQ(1U, GetAllPages().size());
 }
 
 }  // namespace offline_pages