Create a pref to store the last used marker color for Projector

- Register the new pref in ProjectorControllerImpl - Update the APIs in ProjectorUiController to separate setting the annotation tool from enabling the canvas - Read the last used color pref upon user login Tested: Switching accounts, making sure the correct marker was set and that all marker behavior is WAI. Bug: b/227502316 Change-Id: I1fbee6fda61fe851f28f8e6156270b48f888cee5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3628493 Commit-Queue: Courtney Wong <courtneywong@chromium.org> Reviewed-by: Li Lin <llin@chromium.org> Reviewed-by: Ahmed Fakhry <afakhry@chromium.org> Cr-Commit-Position: refs/heads/main@{#1002883}
chromium · May 12, 2022 · fa3b719 · fa3b719
1 parent 6ce4f1c
commit fa3b719
Show file tree

Hide file tree

Showing 13 changed files with 111 additions and 39 deletions.
diff --git a/ash/ash_prefs.cc b/ash/ash_prefs.cc
@@ -22,6 +22,7 @@
 #include "ash/login/ui/login_expanded_public_account_view.h"
 #include "ash/media/media_controller_impl.h"
 #include "ash/metrics/feature_discovery_duration_reporter_impl.h"
+#include "ash/projector/projector_controller_impl.h"
 #include "ash/public/cpp/holding_space/holding_space_prefs.h"
 #include "ash/quick_pair/keyed_service/quick_pair_mediator.h"
 #include "ash/session/fullscreen_controller.h"
@@ -102,6 +103,7 @@ void RegisterProfilePrefs(PrefRegistrySimple* registry, bool for_test) {
   PciePeripheralNotificationController::RegisterProfilePrefs(registry);
   PersistentDesksBarController::RegisterProfilePrefs(registry);
   PrivacyScreenController::RegisterProfilePrefs(registry);
+  ProjectorControllerImpl::RegisterProfilePrefs(registry);
   quick_pair::Mediator::RegisterProfilePrefs(registry);
   ShelfController::RegisterProfilePrefs(registry);
   SnoopingProtectionController::RegisterProfilePrefs(registry);

diff --git a/ash/capture_mode/capture_mode_unittests.cc b/ash/capture_mode/capture_mode_unittests.cc
@@ -5038,11 +5038,11 @@ TEST_F(ProjectorCaptureModeIntegrationTests, RecordingOverlayWidget) {
   VerifyOverlayEnabledState(overlay_window, /*overlay_enabled_state=*/false);
 
   auto* projector_controller = ProjectorControllerImpl::Get();
-  projector_controller->OnMarkerPressed();
+  projector_controller->EnableAnnotatorTool();
   EXPECT_TRUE(overlay_controller->is_enabled());
   VerifyOverlayEnabledState(overlay_window, /*overlay_enabled_state=*/true);
 
-  projector_controller->OnMarkerPressed();
+  projector_controller->ResetTools();
   EXPECT_FALSE(overlay_controller->is_enabled());
   VerifyOverlayEnabledState(overlay_window, /*overlay_enabled_state=*/false);
 }
@@ -5060,7 +5060,7 @@ TEST_F(ProjectorCaptureModeIntegrationTests, RecordingOverlayDockedMagnifier) {
       test_api.GetRecordingOverlayController();
 
   auto* projector_controller = ProjectorControllerImpl::Get();
-  projector_controller->OnMarkerPressed();
+  projector_controller->EnableAnnotatorTool();
   EXPECT_TRUE(overlay_controller->is_enabled());
   auto* overlay_window = overlay_controller->GetOverlayNativeWindow();
 
@@ -5162,7 +5162,7 @@ TEST_F(ProjectorCaptureModeIntegrationTests, RecordingOverlayWidgetTargeting) {
       test_api.GetRecordingOverlayController();
 
   auto* projector_controller = ProjectorControllerImpl::Get();
-  projector_controller->OnMarkerPressed();
+  projector_controller->EnableAnnotatorTool();
   EXPECT_TRUE(overlay_controller->is_enabled());
   auto* overlay_window = overlay_controller->GetOverlayNativeWindow();
   VerifyOverlayEnabledState(overlay_window, /*overlay_enabled_state=*/true);
@@ -5245,7 +5245,7 @@ TEST_F(ProjectorCaptureModeIntegrationTests,
   for (const auto& test_case : kTestCases) {
     SCOPED_TRACE(test_case.scope_trace);
     // Enable annotation.
-    projector_controller->OnMarkerPressed();
+    projector_controller->EnableAnnotatorTool();
 
     // Verify shelf is invisible right now.
     EXPECT_FALSE(shelf->IsVisible());
@@ -5269,7 +5269,7 @@ TEST_F(ProjectorCaptureModeIntegrationTests,
     EXPECT_TRUE(shelf->IsVisible());
 
     // Disable annotation.
-    projector_controller->OnMarkerPressed();
+    projector_controller->ResetTools();
     // Move mouse to the outside of the shelf activation area, and wait for the
     // animation to hide shelf to finish.
     event_generator->MoveMouseTo(display_center);

diff --git a/ash/constants/ash_pref_names.cc b/ash/constants/ash_pref_names.cc
@@ -936,6 +936,11 @@ const char kDeskTemplatesEnabled[] = "ash.desk_templates_enabled";
 // predefined Desks templates configured by policy administrators.
 const char kPreconfiguredDeskTemplates[] = "ash.preconfigured_desk_templates";
 
+// An unsigned integer pref which contains the last used marker color for
+// Projector.
+const char kProjectorAnnotatorLastUsedMarkerColor[] =
+    "ash.projector.annotator_last_used_marker_color";
+
 // A boolean pref that tracks whether the user has enabled Projector creation
 // flow during onboarding.
 const char kProjectorCreationFlowEnabled[] =

diff --git a/ash/constants/ash_pref_names.h b/ash/constants/ash_pref_names.h
@@ -425,6 +425,9 @@ extern const char kFastPairEnabled[];
 COMPONENT_EXPORT(ASH_CONSTANTS) extern const char kDeskTemplatesEnabled[];
 COMPONENT_EXPORT(ASH_CONSTANTS) extern const char kPreconfiguredDeskTemplates[];
 
+COMPONENT_EXPORT(ASH_CONSTANTS)
+extern const char kProjectorAnnotatorLastUsedMarkerColor[];
+
 COMPONENT_EXPORT(ASH_CONSTANTS)
 extern const char kProjectorCreationFlowEnabled[];
 

diff --git a/ash/projector/projector_annotation_tray.cc b/ash/projector/projector_annotation_tray.cc
@@ -5,6 +5,7 @@
 #include "ash/projector/projector_annotation_tray.h"
 
 #include "ash/constants/ash_features.h"
+#include "ash/constants/ash_pref_names.h"
 #include "ash/projector/projector_controller_impl.h"
 #include "ash/projector/ui/projector_color_button.h"
 #include "ash/public/cpp/projector/annotator_tool.h"
@@ -19,6 +20,7 @@
 #include "ash/system/tray/tray_container.h"
 #include "ash/system/tray/tray_popup_utils.h"
 #include "ash/system/tray/tray_utils.h"
+#include "components/prefs/pref_service.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/compositor/layer.h"
 #include "ui/gfx/paint_vector_icon.h"
@@ -101,6 +103,8 @@ ProjectorAnnotationTray::ProjectorAnnotationTray(Shelf* shelf)
   image_view_->SetVerticalAlignment(views::ImageView::Alignment::kCenter);
   image_view_->SetPreferredSize(gfx::Size(kTrayItemSize, kTrayItemSize));
   ResetTray();
+
+  session_observer_.Observe(Shell::Get()->session_controller());
 }
 
 ProjectorAnnotationTray::~ProjectorAnnotationTray() = default;
@@ -233,9 +237,20 @@ void ProjectorAnnotationTray::OnThemeChanged() {
   UpdateIcon();
 }
 
+void ProjectorAnnotationTray::OnActiveUserPrefServiceChanged(
+    PrefService* pref_service) {
+  const uint64_t color =
+      pref_service->GetUint64(prefs::kProjectorAnnotatorLastUsedMarkerColor);
+  current_pen_color_ = !color ? kProjectorDefaultPenColor : color;
+}
+
 void ProjectorAnnotationTray::HideAnnotationTray() {
   SetVisiblePreferred(false);
   UpdateIcon();
+  PrefService* pref_service =
+      Shell::Get()->session_controller()->GetActivePrefService();
+  pref_service->SetUint64(prefs::kProjectorAnnotatorLastUsedMarkerColor,
+                          current_pen_color_);
   ResetTray();
 }
 
@@ -253,7 +268,7 @@ void ProjectorAnnotationTray::OnCanvasInitializationFailed() {
 
 void ProjectorAnnotationTray::ToggleAnnotator() {
   if (GetCurrentTool() == kToolNone) {
-    EnableAnnotatorTool();
+    EnableAnnotatorWithPenColor();
   } else {
     DeactivateActiveTool();
   }
@@ -263,10 +278,13 @@ void ProjectorAnnotationTray::ToggleAnnotator() {
   UpdateIcon();
 }
 
-void ProjectorAnnotationTray::EnableAnnotatorTool() {
+void ProjectorAnnotationTray::EnableAnnotatorWithPenColor() {
   auto* controller = Shell::Get()->projector_controller();
   DCHECK(controller);
-  controller->OnMarkerPressed();
+  AnnotatorTool tool;
+  tool.color = current_pen_color_;
+  controller->SetAnnotatorTool(tool);
+  controller->EnableAnnotatorTool();
 }
 
 void ProjectorAnnotationTray::DeactivateActiveTool() {
@@ -286,12 +304,8 @@ void ProjectorAnnotationTray::UpdateIcon() {
 }
 
 void ProjectorAnnotationTray::OnPenColorPressed(SkColor color) {
-  auto* projector_controller = ProjectorControllerImpl::Get();
-  DCHECK(projector_controller);
-  AnnotatorTool tool;
-  tool.color = color;
-  projector_controller->SetAnnotatorTool(tool);
   current_pen_color_ = color;
+  EnableAnnotatorWithPenColor();
   CloseBubble();
   UpdateIcon();
 }
@@ -312,8 +326,6 @@ int ProjectorAnnotationTray::GetAccessibleNameForColor(SkColor color) {
 }
 
 void ProjectorAnnotationTray::ResetTray() {
-  // Set current pen to default pen color
-  current_pen_color_ = kProjectorMagentaPenColor;
   // Disable the tray icon. It is enabled once the ink canvas is initialized.
   SetEnabled(false);
 }

diff --git a/ash/projector/projector_annotation_tray.h b/ash/projector/projector_annotation_tray.h
@@ -5,8 +5,11 @@
 #ifndef ASH_PROJECTOR_PROJECTOR_ANNOTATION_TRAY_H_
 #define ASH_PROJECTOR_PROJECTOR_ANNOTATION_TRAY_H_
 
+#include "ash/public/cpp/session/session_observer.h"
+#include "ash/session/session_controller_impl.h"
 #include "ash/system/tray/tray_background_view.h"
 #include "ash/system/tray/view_click_listener.h"
+#include "base/scoped_observation.h"
 
 namespace ash {
 
@@ -18,10 +21,12 @@ constexpr SkColor kProjectorMagentaPenColor = SkColorSetRGB(0xFF, 0x00, 0xE5);
 constexpr SkColor kProjectorRedPenColor = SkColorSetRGB(0xE9, 0x42, 0x35);
 constexpr SkColor kProjectorYellowPenColor = SkColorSetRGB(0xFB, 0xF1, 0x04);
 constexpr SkColor kProjectorBluePenColor = SkColorSetRGB(0x42, 0x85, 0xF4);
+constexpr SkColor kProjectorDefaultPenColor = kProjectorMagentaPenColor;
 
 // Status area tray which allows you to access the annotation tools for
 // Projector.
-class ProjectorAnnotationTray : public TrayBackgroundView {
+class ProjectorAnnotationTray : public TrayBackgroundView,
+                                public SessionObserver {
  public:
   explicit ProjectorAnnotationTray(Shelf* shelf);
   ProjectorAnnotationTray(const ProjectorAnnotationTray&) = delete;
@@ -42,12 +47,15 @@ class ProjectorAnnotationTray : public TrayBackgroundView {
   void OnGestureEvent(ui::GestureEvent* event) override;
   void OnThemeChanged() override;
 
+  // SessionObserver:
+  void OnActiveUserPrefServiceChanged(PrefService* pref_service) override;
+
   void HideAnnotationTray();
   void OnCanvasInitializationFailed();
 
  private:
   void ToggleAnnotator();
-  void EnableAnnotatorTool();
+  void EnableAnnotatorWithPenColor();
   // Deactivates any annotation tool that is currently enabled and updates the
   // UI.
   void DeactivateActiveTool();
@@ -74,7 +82,10 @@ class ProjectorAnnotationTray : public TrayBackgroundView {
   std::unique_ptr<TrayBubbleWrapper> bubble_;
 
   // The last selected pen color.
-  SkColor current_pen_color_;
+  SkColor current_pen_color_ = kProjectorDefaultPenColor;
+
+  base::ScopedObservation<SessionControllerImpl, SessionObserver>
+      session_observer_{this};
 };
 
 }  // namespace ash

diff --git a/ash/projector/projector_controller_impl.cc b/ash/projector/projector_controller_impl.cc
@@ -6,6 +6,7 @@
 
 #include "ash/capture_mode/capture_mode_controller.h"
 #include "ash/capture_mode/capture_mode_metrics.h"
+#include "ash/constants/ash_pref_names.h"
 #include "ash/projector/projector_metadata_controller.h"
 #include "ash/projector/projector_metrics.h"
 #include "ash/projector/projector_ui_controller.h"
@@ -24,6 +25,8 @@
 #include "base/task/current_thread.h"
 #include "base/task/thread_pool.h"
 #include "base/time/time.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+#include "components/prefs/pref_registry_simple.h"
 #include "media/mojo/mojom/speech_recognition_service.mojom.h"
 #include "ui/gfx/image/image.h"
 
@@ -114,6 +117,14 @@ ProjectorControllerImpl* ProjectorControllerImpl::Get() {
   return static_cast<ProjectorControllerImpl*>(ProjectorController::Get());
 }
 
+// static
+void ProjectorControllerImpl::RegisterProfilePrefs(
+    PrefRegistrySimple* registry) {
+  registry->RegisterUint64Pref(
+      prefs::kProjectorAnnotatorLastUsedMarkerColor, 0u,
+      user_prefs::PrefRegistrySyncable::SYNCABLE_OS_PREF);
+}
+
 void ProjectorControllerImpl::StartProjectorSession(
     const std::string& storage_dir) {
   DCHECK_EQ(GetNewScreencastPrecondition().state,
@@ -367,9 +378,9 @@ void ProjectorControllerImpl::OnRecordingStartAborted() {
   RecordCreationFlowMetrics(ProjectorCreationFlow::kRecordingAborted);
 }
 
-void ProjectorControllerImpl::OnMarkerPressed() {
+void ProjectorControllerImpl::EnableAnnotatorTool() {
   DCHECK(ui_controller_);
-  ui_controller_->OnMarkerPressed();
+  ui_controller_->EnableAnnotatorTool();
 }
 
 void ProjectorControllerImpl::SetAnnotatorTool(const AnnotatorTool& tool) {

diff --git a/ash/projector/projector_controller_impl.h b/ash/projector/projector_controller_impl.h
@@ -14,6 +14,8 @@
 #include "ash/public/cpp/projector/projector_controller.h"
 #include "third_party/skia/include/core/SkColor.h"
 
+class PrefRegistrySimple;
+
 namespace aura {
 class Window;
 }  // namespace aura
@@ -61,6 +63,7 @@ class ASH_EXPORT ProjectorControllerImpl
   ~ProjectorControllerImpl() override;
 
   static ProjectorControllerImpl* Get();
+  static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
   // ProjectorController:
   void StartProjectorSession(const std::string& storage_dir) override;
@@ -114,8 +117,8 @@ class ASH_EXPORT ProjectorControllerImpl
   // cancellation, an error, or a DLP/HDCP restriction.
   void OnRecordingStartAborted();
 
-  // Invoked when marker button is pressed.
-  void OnMarkerPressed();
+  // Enables the annotator tool.
+  void EnableAnnotatorTool();
   // Sets the annotator tool.
   void SetAnnotatorTool(const AnnotatorTool& tool);
   // Reset and disable the the annotator tools.

diff --git a/ash/projector/projector_controller_unittest.cc b/ash/projector/projector_controller_unittest.cc
@@ -229,10 +229,10 @@ TEST_F(ProjectorControllerTest, OnSpeechRecognitionAvailabilityChanged) {
   EXPECT_FALSE(controller_->IsEligible());
 }
 
-TEST_F(ProjectorControllerTest, OnMarkerPressed) {
+TEST_F(ProjectorControllerTest, EnableAnnotatorTool) {
   // Verify that |OnMarkerPressed| in |ProjectorUiController| is called.
-  EXPECT_CALL(*mock_ui_controller_, OnMarkerPressed());
-  controller_->OnMarkerPressed();
+  EXPECT_CALL(*mock_ui_controller_, EnableAnnotatorTool());
+  controller_->EnableAnnotatorTool();
 }
 
 TEST_F(ProjectorControllerTest, SetAnnotatorTool) {

diff --git a/ash/projector/projector_ui_controller.cc b/ash/projector/projector_ui_controller.cc
@@ -165,17 +165,15 @@ void ProjectorUiController::CloseToolbar() {
   current_root_ = nullptr;
 }
 
-void ProjectorUiController::OnMarkerPressed() {
-  ToggleAnnotator();
-  annotator_enabled_ = !annotator_enabled_;
-  RecordToolbarMetrics(ProjectorToolbar::kMarkerTool);
-}
-
-void ProjectorUiController::SetAnnotatorTool(const AnnotatorTool& tool) {
+void ProjectorUiController::EnableAnnotatorTool() {
   if (!annotator_enabled_) {
     ToggleAnnotator();
     annotator_enabled_ = !annotator_enabled_;
+    RecordToolbarMetrics(ProjectorToolbar::kMarkerTool);
   }
+}
+
+void ProjectorUiController::SetAnnotatorTool(const AnnotatorTool& tool) {
   ash::ProjectorAnnotatorController::Get()->SetTool(tool);
   RecordMarkerColorMetrics(GetMarkerColorForMetrics(tool.color));
 }

diff --git a/ash/projector/projector_ui_controller.h b/ash/projector/projector_ui_controller.h
@@ -42,7 +42,7 @@ class ASH_EXPORT ProjectorUiController : public ProjectorSessionObserver {
   // Close Projector toolbar. Virtual for testing.
   virtual void CloseToolbar();
   // Invoked when marker button is pressed. Virtual for testing.
-  virtual void OnMarkerPressed();
+  virtual void EnableAnnotatorTool();
   // Sets the annotator tool.
   virtual void SetAnnotatorTool(const AnnotatorTool& tool);
   // Resets and disables the annotator tools and clears the canvas.