Remove invisible tabs from ThumbnailTracker

Fixes: 1015132
Change-Id: I8b99f9fdd044b249f44c681bafe10bb2037046a7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1898305Reviewed-by: John Lee <johntlee@chromium.org>
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712658}

chrome/browser/ui/webui/tab_strip/tab_strip_ui.cc

@@ -392,11 +392,10 @@ class TabStripUIHandler : public content::WebUIMessageHandler,
       return;
     }
 
-    if (thumbnail_tracked) {
-      thumbnail_tracker_.WatchTab(tab);
-    } else {
-      // TODO(crbug.com/991393): un-watch tabs when we are done.
-    }
+    if (thumbnail_tracked)
+      thumbnail_tracker_.AddTab(tab);
+    else
+      thumbnail_tracker_.RemoveTab(tab);
   }
 
   // Callback passed to |thumbnail_tracker_|. Called when a tab's thumbnail

chrome/browser/ui/webui/tab_strip/thumbnail_tracker.cc

@@ -68,7 +68,7 @@ ThumbnailTracker::ThumbnailTracker(ThumbnailUpdatedCallback callback,
 
 ThumbnailTracker::~ThumbnailTracker() = default;
 
-void ThumbnailTracker::WatchTab(content::WebContents* contents) {
+void ThumbnailTracker::AddTab(content::WebContents* contents) {
   auto data_it = contents_data_.find(contents);
   if (data_it == contents_data_.end()) {
     data_it = contents_data_.emplace_hint(
@@ -78,6 +78,10 @@ void ThumbnailTracker::WatchTab(content::WebContents* contents) {
   data_it->second->RequestThumbnail();
 }
 
+void ThumbnailTracker::RemoveTab(content::WebContents* contents) {
+  contents_data_.erase(contents);
+}
+
 void ThumbnailTracker::ThumbnailUpdated(content::WebContents* contents,
                                         CompressedThumbnailData image) {
   callback_.Run(contents, image);

chrome/browser/ui/webui/tab_strip/thumbnail_tracker.h

@@ -43,7 +43,8 @@ class ThumbnailTracker {
 
   // Registers a tab to receive thumbnail updates for. Also immediately requests
   // the current thumbnail.
-  void WatchTab(content::WebContents* contents);
+  void AddTab(content::WebContents* contents);
+  void RemoveTab(content::WebContents* contents);
 
  private:
   void ThumbnailUpdated(content::WebContents* contents,

chrome/browser/ui/webui/tab_strip/thumbnail_tracker_unittest.cc

@@ -25,10 +25,16 @@ namespace {
 class TestThumbnailImageDelegate : public ThumbnailImage::Delegate {
  public:
   TestThumbnailImageDelegate() = default;
-
   ~TestThumbnailImageDelegate() override = default;
 
-  void ThumbnailImageBeingObservedChanged(bool is_being_observed) override {}
+  void ThumbnailImageBeingObservedChanged(bool is_being_observed) override {
+    is_being_observed_ = is_being_observed;
+  }
+
+  bool is_being_observed() const { return is_being_observed_; }
+
+ private:
+  bool is_being_observed_ = false;
 };
 
 class ThumbnailTrackerTest : public ::testing::Test,
@@ -85,7 +91,7 @@ class ThumbnailTrackerTest : public ::testing::Test,
 
 using ::testing::_;
 
-TEST_F(ThumbnailTrackerTest, WatchTabGetsCurrentThumbnail) {
+TEST_F(ThumbnailTrackerTest, AddTabGetsCurrentThumbnail) {
   auto contents = CreateWebContents();
   auto thumbnail = GetTestingThumbnail(contents.get());
 
@@ -96,12 +102,13 @@ TEST_F(ThumbnailTrackerTest, WatchTabGetsCurrentThumbnail) {
   thumbnail->AssignSkBitmap(CreateTestingBitmap());
   encode_loop.Run();
 
-  // Verify that WatchTab() gets the current image, waiting for the decoding to
-  // happen.
+  // Verify that AddTab() gets the current image. This should happen
+  // immediately.
   EXPECT_CALL(thumbnail_updated_callback_, Run(contents.get(), _)).Times(1);
   thumbnail->set_async_operation_finished_callback_for_testing(
       base::RepeatingClosure());
-  thumbnail_tracker_.WatchTab(contents.get());
+  thumbnail_tracker_.AddTab(contents.get());
+  EXPECT_TRUE(tab_thumbnails_[contents.get()].delegate.is_being_observed());
 }
 
 TEST_F(ThumbnailTrackerTest, PropagatesThumbnailUpdate) {
@@ -111,8 +118,8 @@ TEST_F(ThumbnailTrackerTest, PropagatesThumbnailUpdate) {
   auto thumbnail2 = GetTestingThumbnail(contents2.get());
 
   // Since no thumbnail image exists yet, this shouldn't notify our callback.
-  thumbnail_tracker_.WatchTab(contents1.get());
-  thumbnail_tracker_.WatchTab(contents2.get());
+  thumbnail_tracker_.AddTab(contents1.get());
+  thumbnail_tracker_.AddTab(contents2.get());
 
   {
     ::testing::InSequence seq;
@@ -136,7 +143,7 @@ TEST_F(ThumbnailTrackerTest, PropagatesThumbnailUpdate) {
 TEST_F(ThumbnailTrackerTest, StopsObservingOnTabClose) {
   auto contents = CreateWebContents();
   auto thumbnail = GetTestingThumbnail(contents.get());
-  thumbnail_tracker_.WatchTab(contents.get());
+  thumbnail_tracker_.AddTab(contents.get());
 
   // |thumbnail| is still valid, but |thumbnail_tracker_| should stop watching
   // it when |contents| goes away.
@@ -149,3 +156,11 @@ TEST_F(ThumbnailTrackerTest, StopsObservingOnTabClose) {
   thumbnail->AssignSkBitmap(CreateTestingBitmap());
   update_loop.Run();
 }
+
+TEST_F(ThumbnailTrackerTest, RemoveTabStopsObservingThumbnail) {
+  auto contents = CreateWebContents();
+  auto thumbnail = GetTestingThumbnail(contents.get());
+  thumbnail_tracker_.AddTab(contents.get());
+  thumbnail_tracker_.RemoveTab(contents.get());
+  EXPECT_FALSE(tab_thumbnails_[contents.get()].delegate.is_being_observed());
+}