capture mode: Clear capture region from previous session after some time

It can be annoying to remember the capture region from previous sessions
that happen long time ago. This CL adds a 8 minutes timeout to forget
the capture region since the last time the user selected one. Note the
timer starts on the last time the user selected a capture region from
the previous session and will be used to compare with the current
session start time to see if we need to clear the capture region.

Bug: 1159117
Change-Id: Idc8c47f6272eb7b41e912a737b2c52fed7f1750a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2594558
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838507}

ash/capture_mode/capture_mode_controller.cc

@@ -74,6 +74,10 @@ constexpr char kDateFmtStr[] = "%d-%02d-%02d";
 constexpr char k24HourTimeFmtStr[] = "%02d.%02d.%02d";
 constexpr char kAmPmTimeFmtStr[] = "%d.%02d.%02d";
 
+// Duration to clear the capture region selection from the previous session.
+constexpr base::TimeDelta kResetCaptureRegionDuration =
+    base::TimeDelta::FromMinutes(8);
+
 // The screenshot notification button index.
 enum ScreenshotNotificationButtonIndex {
   BUTTON_EDIT = 0,
@@ -320,6 +324,14 @@ void CaptureModeController::Start(CaptureModeEntryType entry_type) {
   }
 
   RecordCaptureModeEntryType(entry_type);
+  // Reset the user capture region if enough time has passed as it can be
+  // annoying to still have the old capture region from the previous session
+  // long time ago.
+  if (!user_capture_region_.IsEmpty() &&
+      base::TimeTicks::Now() - last_capture_region_update_time_ >
+          kResetCaptureRegionDuration) {
+    SetUserCaptureRegion(gfx::Rect(), /*by_user=*/false);
+  }
   capture_mode_session_ = std::make_unique<CaptureModeSession>(this);
 }
 
@@ -328,6 +340,13 @@ void CaptureModeController::Stop() {
   capture_mode_session_.reset();
 }
 
+void CaptureModeController::SetUserCaptureRegion(const gfx::Rect& region,
+                                                 bool by_user) {
+  user_capture_region_ = region;
+  if (!user_capture_region_.IsEmpty() && by_user)
+    last_capture_region_update_time_ = base::TimeTicks::Now();
+}
+
 void CaptureModeController::PerformCapture() {
   DCHECK(IsActive());
   const base::Optional<CaptureParams> capture_params = GetCaptureParams();

ash/capture_mode/capture_mode_controller.h

@@ -63,9 +63,6 @@ class ASH_EXPORT CaptureModeController
     return capture_mode_session_.get();
   }
   gfx::Rect user_capture_region() const { return user_capture_region_; }
-  void set_user_capture_region(const gfx::Rect& region) {
-    user_capture_region_ = region;
-  }
   bool is_recording_in_progress() const { return is_recording_in_progress_; }
 
   // Returns true if a capture mode session is currently active.
@@ -83,6 +80,10 @@ class ASH_EXPORT CaptureModeController
   // Stops an existing capture session.
   void Stop();
 
+  // Sets the user capture region. If it's non-empty and changed by the user,
+  // update |last_capture_region_update_time_|.
+  void SetUserCaptureRegion(const gfx::Rect& region, bool by_user);
+
   // Called only while a capture session is in progress to perform the actual
   // capture depending on the current selected |source_| and |type_|, and ends
   // the capture session.
@@ -297,6 +298,12 @@ class ASH_EXPORT CaptureModeController
   // collection.
   base::TimeTicks recording_start_time_;
 
+  // The last time the user sets a non-empty capture region. It will be used to
+  // clear the user capture region from previous capture sessions if 8+ minutes
+  // has passed since the last time the user changes the capture region when the
+  // new capture session starts .
+  base::TimeTicks last_capture_region_update_time_;
+
   base::WeakPtrFactory<CaptureModeController> weak_ptr_factory_{this};
 };
 

ash/capture_mode/capture_mode_session.cc

@@ -993,7 +993,7 @@ void CaptureModeSession::OnLocatedEventPressed(
     // If the point is outside the capture region and not on the capture bar,
     // restart to the select phase.
     is_selecting_region_ = true;
-    UpdateCaptureRegion(gfx::Rect(), /*is_resizing=*/true);
+    UpdateCaptureRegion(gfx::Rect(), /*is_resizing=*/true, /*by_user=*/true);
     num_capture_region_adjusted_ = 0;
     return;
   }
@@ -1026,7 +1026,7 @@ void CaptureModeSession::OnLocatedEventDragged(
   if (is_selecting_region_) {
     UpdateCaptureRegion(
         GetRectEnclosingPoints({initial_location_in_root_, location_in_root}),
-        /*is_resizing=*/true);
+        /*is_resizing=*/true, /*by_user=*/true);
     return;
   }
 
@@ -1040,7 +1040,8 @@ void CaptureModeSession::OnLocatedEventDragged(
     gfx::Rect new_capture_region = controller_->user_capture_region();
     new_capture_region.Offset(location_in_root - previous_location_in_root);
     new_capture_region.AdjustToFit(current_root_->bounds());
-    UpdateCaptureRegion(new_capture_region, /*is_resizing=*/false);
+    UpdateCaptureRegion(new_capture_region, /*is_resizing=*/false,
+                        /*by_user=*/true);
     return;
   }
 
@@ -1064,7 +1065,8 @@ void CaptureModeSession::OnLocatedEventDragged(
     resizing_point.set_x(points.front().x());
   }
   points.push_back(resizing_point);
-  UpdateCaptureRegion(GetRectEnclosingPoints(points), /*is_resizing=*/true);
+  UpdateCaptureRegion(GetRectEnclosingPoints(points), /*is_resizing=*/true,
+                      /*by_user=*/true);
   MaybeShowMagnifierGlassAtPoint(location_in_root);
 }
 
@@ -1086,7 +1088,8 @@ void CaptureModeSession::OnLocatedEventReleased(
 
 void CaptureModeSession::UpdateCaptureRegion(
     const gfx::Rect& new_capture_region,
-    bool is_resizing) {
+    bool is_resizing,
+    bool by_user) {
   const gfx::Rect old_capture_region = controller_->user_capture_region();
   if (old_capture_region == new_capture_region)
     return;
@@ -1099,7 +1102,7 @@ void CaptureModeSession::UpdateCaptureRegion(
   damage_region.Inset(gfx::Insets(-kDamageInsetDp));
   layer()->SchedulePaint(damage_region);
 
-  controller_->set_user_capture_region(new_capture_region);
+  controller_->SetUserCaptureRegion(new_capture_region, by_user);
   UpdateDimensionsLabelWidget(is_resizing);
   UpdateCaptureLabelWidget();
 }
@@ -1406,7 +1409,7 @@ void CaptureModeSession::MaybeChangeRoot(aura::Window* new_root) {
 
   // Start with a new region when we switch displays.
   is_selecting_region_ = true;
-  UpdateCaptureRegion(gfx::Rect(), /*is_resizing=*/false);
+  UpdateCaptureRegion(gfx::Rect(), /*is_resizing=*/false, /*by_user=*/false);
 
   UpdateRootWindowDimmers();
 }
@@ -1522,7 +1525,7 @@ void CaptureModeSession::UpdateCaptureBarWidgetOpacity(float opacity,
 void CaptureModeSession::ClampCaptureRegionToRootWindowSize() {
   gfx::Rect new_capture_region = controller_->user_capture_region();
   new_capture_region.AdjustToFit(current_root_->bounds());
-  controller_->set_user_capture_region(new_capture_region);
+  controller_->SetUserCaptureRegion(new_capture_region, /*by_user=*/false);
 }
 
 void CaptureModeSession::EndSelection(bool is_event_on_capture_bar,
@@ -1558,7 +1561,8 @@ void CaptureModeSession::SelectDefaultRegion() {
   gfx::Rect default_capture_region = current_root_->bounds();
   default_capture_region.ClampToCenteredSize(gfx::ScaleToCeiledSize(
       default_capture_region.size(), kRegionDefaultRatio));
-  UpdateCaptureRegion(default_capture_region, /*is_resizing=*/false);
+  UpdateCaptureRegion(default_capture_region, /*is_resizing=*/false,
+                      /*by_user=*/true);
 }
 
 void CaptureModeSession::UpdateRegionHorizontally(bool left,
@@ -1596,7 +1600,8 @@ void CaptureModeSession::UpdateRegionHorizontally(bool left,
     ClipRectToFit(&new_capture_region, current_root_->bounds());
   }
 
-  UpdateCaptureRegion(new_capture_region, /*is_resizing=*/false);
+  UpdateCaptureRegion(new_capture_region, /*is_resizing=*/false,
+                      /*by_user=*/true);
 }
 
 void CaptureModeSession::UpdateRegionVertically(bool up, bool is_shift_down) {
@@ -1636,7 +1641,8 @@ void CaptureModeSession::UpdateRegionVertically(bool up, bool is_shift_down) {
     ClipRectToFit(&new_capture_region, current_root_->bounds());
   }
 
-  UpdateCaptureRegion(new_capture_region, /*is_resizing=*/false);
+  UpdateCaptureRegion(new_capture_region, /*is_resizing=*/false,
+                      /*by_user=*/true);
 }
 
 }  // namespace ash

ash/capture_mode/capture_mode_session.h

@@ -137,9 +137,11 @@ class ASH_EXPORT CaptureModeSession : public ui::LayerOwner,
                               bool region_intersects_capture_bar);
 
   // Updates the capture region and the capture region widgets depending on the
-  // value of |is_resizing|.
+  // value of |is_resizing|. |by_user| is true if the capture region is changed
+  // by user.
   void UpdateCaptureRegion(const gfx::Rect& new_capture_region,
-                           bool is_resizing);
+                           bool is_resizing,
+                           bool by_user);
 
   // Updates the dimensions label widget shown during a region capture session.
   // If not |is_resizing|, not a region capture session or the capture region is

ash/capture_mode/capture_mode_test_api.cc

@@ -43,7 +43,7 @@ void CaptureModeTestApi::StartForRegion(bool for_video) {
 }
 
 void CaptureModeTestApi::SetUserSelectedRegion(const gfx::Rect& region) {
-  controller_->set_user_capture_region(region);
+  controller_->SetUserCaptureRegion(region, /*by_user=*/true);
 }
 
 void CaptureModeTestApi::PerformCapture() {

ash/capture_mode/capture_mode_unittests.cc

@@ -2059,6 +2059,35 @@ TEST_F(CaptureModeMockTimeTest, ConsecutiveScreenshotsHistograms) {
   histogram_tester.ExpectBucketCount(kConsecutiveScreenshotsHistogram, 2, 1);
 }
 
+// Tests that the user capture region will be cleared up after a period of time.
+TEST_F(CaptureModeMockTimeTest, ClearUserCaptureRegionBetweenSessions) {
+  UpdateDisplay("800x800");
+  auto* controller = StartImageRegionCapture();
+  EXPECT_EQ(gfx::Rect(), controller->user_capture_region());
+
+  const gfx::Rect capture_region(100, 100, 600, 600);
+  SelectRegion(capture_region);
+  EXPECT_EQ(capture_region, controller->user_capture_region());
+  controller->PerformCapture();
+  EXPECT_EQ(capture_region, controller->user_capture_region());
+
+  // Start region image capture again shortly after the previous capture
+  // session, we should still be able to reuse the previous capture region.
+  task_environment()->FastForwardBy(base::TimeDelta::FromMinutes(1));
+  StartImageRegionCapture();
+  EXPECT_EQ(capture_region, controller->user_capture_region());
+  auto* event_generator = GetEventGenerator();
+  // Even if the capture is cancelled, we still remember the capture region.
+  SendKey(ui::VKEY_ESCAPE, event_generator);
+  EXPECT_EQ(capture_region, controller->user_capture_region());
+
+  // Wait for 8 second and then start region image capture again. We should have
+  // forgot the previous capture region.
+  task_environment()->FastForwardBy(base::TimeDelta::FromMinutes(8));
+  StartImageRegionCapture();
+  EXPECT_EQ(gfx::Rect(), controller->user_capture_region());
+}
+
 // Tests that in Region mode, the capture bar hides and shows itself correctly.
 TEST_F(CaptureModeTest, CaptureBarOpacity) {
   UpdateDisplay("800x800");