Fix: frame throttling failed to unthrottle the last window in overview.

Bug: 1116643
Change-Id: Ie50f656ef08645b53c0841b8efb77abe0fa4ba87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425105Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Jun Liu <yjliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810338}

ash/frame_throttler/frame_throttling_controller.cc

@@ -61,11 +61,15 @@ FrameThrottlingController::~FrameThrottlingController() {
 
 void FrameThrottlingController::StartThrottling(
     const std::vector<aura::Window*>& windows) {
+  if (windows_throttled_)
+    EndThrottling();
+
+  windows_throttled_ = true;
   std::vector<viz::FrameSinkId> frame_sink_ids;
   frame_sink_ids.reserve(windows.size());
-
   CollectBrowserFrameSinkIds(windows, &frame_sink_ids);
-  StartThrottling(frame_sink_ids, fps_);
+  if (!frame_sink_ids.empty())
+    StartThrottling(frame_sink_ids, fps_);
 
   for (auto& observer : observers_) {
     observer.OnThrottlingStarted(windows);
@@ -76,7 +80,8 @@ void FrameThrottlingController::StartThrottling(
     const std::vector<viz::FrameSinkId>& frame_sink_ids,
     uint8_t fps) {
   DCHECK_GT(fps, 0);
-  if (context_factory_ && !frame_sink_ids.empty()) {
+  DCHECK(!frame_sink_ids.empty());
+  if (context_factory_) {
     context_factory_->GetHostFrameSinkManager()->StartThrottling(
         frame_sink_ids, base::TimeDelta::FromSeconds(1) / fps);
   }
@@ -89,6 +94,7 @@ void FrameThrottlingController::EndThrottling() {
   for (auto& observer : observers_) {
     observer.OnThrottlingEnded();
   }
+  windows_throttled_ = false;
 }
 
 void FrameThrottlingController::AddObserver(Observer* observer) {

ash/frame_throttler/frame_throttling_controller.h

@@ -57,6 +57,7 @@ class ASH_EXPORT FrameThrottlingController {
   base::ObserverList<Observer> observers_;
   // The fps used for throttling.
   uint8_t fps_ = kDefaultThrottleFps;
+  bool windows_throttled_ = false;
 };
 
 }  // namespace ash

ash/wm/overview/overview_grid_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "ash/wm/overview/overview_grid.h"
 
+#include "ash/frame_throttler/mock_frame_throttling_observer.h"
 #include "ash/screen_util.h"
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
@@ -84,6 +85,8 @@ class OverviewGridTest : public AshTestBase {
     return SplitViewController::Get(Shell::GetPrimaryRootWindow());
   }
 
+  OverviewGrid* grid() { return grid_.get(); }
+
  private:
   std::unique_ptr<OverviewGrid> grid_;
 
@@ -288,4 +291,40 @@ TEST_F(OverviewGridTest, SnappedWindow) {
   EXPECT_FALSE(item3->should_animate_when_entering());
 }
 
+TEST_F(OverviewGridTest, FrameThrottling) {
+  testing::NiceMock<MockFrameThrottlingObserver> observer;
+  FrameThrottlingController* frame_throttling_controller =
+      Shell::Get()->frame_throttling_controller();
+  frame_throttling_controller->AddObserver(&observer);
+  const int window_count = 5;
+  std::unique_ptr<aura::Window> created_windows[window_count];
+  std::vector<aura::Window*> windows(window_count, nullptr);
+  for (int i = 0; i < window_count; ++i) {
+    created_windows[i] = CreateTestWindow();
+    windows[i] = created_windows[i].get();
+  }
+  InitializeGrid(windows);
+  frame_throttling_controller->StartThrottling(windows);
+
+  // Add a new window to overview.
+  std::unique_ptr<aura::Window> new_window(CreateTestWindow());
+  windows.push_back(new_window.get());
+  EXPECT_CALL(observer, OnThrottlingEnded());
+  EXPECT_CALL(observer,
+              OnThrottlingStarted(testing::UnorderedElementsAreArray(windows)));
+  grid()->AppendItem(new_window.get(), /*reposition=*/false, /*animate=*/false,
+                     /*use_spawn_animation=*/false);
+
+  // Remove windows one by one.
+  for (int i = 0; i < window_count + 1; ++i) {
+    aura::Window* window = windows[0];
+    windows.erase(windows.begin());
+    EXPECT_CALL(observer, OnThrottlingEnded());
+    EXPECT_CALL(observer, OnThrottlingStarted(
+                              testing::UnorderedElementsAreArray(windows)));
+    OverviewItem* item = grid()->GetOverviewItemContaining(window);
+    grid()->RemoveItem(item, /*item_destroying=*/false, /*reposition=*/false);
+  }
+  frame_throttling_controller->RemoveObserver(&observer);
+}
 }  // namespace ash

components/viz/host/host_frame_sink_manager.h

@@ -173,10 +173,9 @@ class VIZ_HOST_EXPORT HostFrameSinkManager
 
   // Starts throttling the frame sinks specified by |frame_sink_ids| and all
   // their descendant sinks to send BeginFrames at an interval of |interval|.
-  // |interval| should be greater than zero. Calling this function before
-  // calling EndThrottling() to end a previous throttling operation will
-  // automatically end the previous operation before applying the current
-  // throttling operation.
+  // |interval| should be greater than zero. Previous throttling operation
+  // on any frame sinks must be ended by EndThrottling() before applying the
+  // current throttling operation.
   void StartThrottling(const std::vector<FrameSinkId>& frame_sink_ids,
                        base::TimeDelta interval);
 

components/viz/service/frame_sinks/frame_sink_manager_impl.cc

@@ -642,10 +642,9 @@ void FrameSinkManagerImpl::StartThrottling(
     const std::vector<FrameSinkId>& frame_sink_ids,
     base::TimeDelta interval) {
   DCHECK_GT(interval, base::TimeDelta());
-  if (frame_sinks_throttled)
-    EndThrottling();
+  DCHECK(!frame_sinks_throttled_);
 
-  frame_sinks_throttled = true;
+  frame_sinks_throttled_ = true;
   for (auto& frame_sink_id : frame_sink_ids) {
     UpdateThrottlingRecursively(frame_sink_id, interval);
   }
@@ -655,6 +654,6 @@ void FrameSinkManagerImpl::EndThrottling() {
   for (auto& support_map_item : support_map_) {
     support_map_item.second->ThrottleBeginFrame(base::TimeDelta());
   }
-  frame_sinks_throttled = false;
+  frame_sinks_throttled_ = false;
 }
 }  // namespace viz

components/viz/service/frame_sinks/frame_sink_manager_impl.h

@@ -341,7 +341,7 @@ class VIZ_SERVICE_EXPORT FrameSinkManagerImpl
   base::flat_map<uint32_t, base::ScopedClosureRunner> cached_back_buffers_;
 
   // This tells if any frame sinks are currently throttled.
-  bool frame_sinks_throttled = false;
+  bool frame_sinks_throttled_ = false;
 
   THREAD_CHECKER(thread_checker_);
 

services/viz/privileged/mojom/compositing/frame_sink_manager.mojom

@@ -127,10 +127,9 @@ interface FrameSinkManager {
 
   // Starts throttling the frame sinks specified by |frame_sink_ids| and all
   // their descendant sinks to send BeginFrames at an interval of |interval|.
-  // |interval| should be greater than zero. Calling this function before
-  // calling EndThrottling() to end a previous throttling operation will
-  // automatically end the previous operation before applying the current
-  // throttling operation.
+  // |interval| should be greater than zero. Previous throttling operation on
+  // any frame sinks must be first ended by calling EndThrottling() before
+  // applying the current throttling operation.
   StartThrottling(
       array<FrameSinkId> frame_sink_ids,
       mojo_base.mojom.TimeDelta interval);