Capture Mode: Add feedback button to capture bar

This CL adds a feedback button for users to submit feedback regarding
the screen capture feature. This button will only exist for M-88 dog
food, and will be removed in M-89.

By default, the template is pre-populated with "#ScreenCapture" to allow
us to search easily in Listnr/.

Included is also some minor refactoring to make capture mode buttons
more generic, as well as adding an optional |description_template|
parameter to the chrome OpenFeedbackDialog method.

Test: manual, added minor test
Change-Id: Ie04c30664744187d363765c43b60b955febf0d0a
Fixed: 1142948
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2508402
Commit-Queue: Richard Chui <richui@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823442}

ash/BUILD.gn

@@ -264,8 +264,8 @@ component("ash") {
     "capture_mode/capture_label_view.h",
     "capture_mode/capture_mode_bar_view.cc",
     "capture_mode/capture_mode_bar_view.h",
-    "capture_mode/capture_mode_close_button.cc",
-    "capture_mode/capture_mode_close_button.h",
+    "capture_mode/capture_mode_button.cc",
+    "capture_mode/capture_mode_button.h",
     "capture_mode/capture_mode_constants.h",
     "capture_mode/capture_mode_controller.cc",
     "capture_mode/capture_mode_controller.h",

ash/capture_mode/capture_mode_bar_view.cc

@@ -6,11 +6,12 @@
 
 #include <memory>
 
-#include "ash/capture_mode/capture_mode_close_button.h"
+#include "ash/capture_mode/capture_mode_button.h"
 #include "ash/capture_mode/capture_mode_constants.h"
 #include "ash/capture_mode/capture_mode_controller.h"
 #include "ash/capture_mode/capture_mode_source_view.h"
 #include "ash/capture_mode/capture_mode_type_view.h"
+#include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/style/ash_color_provider.h"
 #include "base/bind.h"
 #include "ui/aura/window.h"
@@ -24,7 +25,9 @@ namespace ash {
 
 namespace {
 
-constexpr gfx::Size kBarSize{328, 64};
+// TODO(crbug.com/1144254): Change this back to {328, 64} when removing the
+// feedback button.
+constexpr gfx::Size kBarSize{392, 64};
 
 constexpr gfx::Insets kBarPadding{/*vertical=*/14, /*horizontal=*/16};
 
@@ -40,14 +43,20 @@ constexpr int kDistanceFromScreenBottom = 56;
 }  // namespace
 
 CaptureModeBarView::CaptureModeBarView()
-    : capture_type_view_(AddChildView(std::make_unique<CaptureModeTypeView>())),
+    : feedback_button_(AddChildView(std::make_unique<CaptureModeButton>(
+          base::BindRepeating(&CaptureModeBarView::OnFeedbackButtonPressed,
+                              base::Unretained(this)),
+          kCaptureModeFeedbackIcon))),
+      separator_0_(AddChildView(std::make_unique<views::Separator>())),
+      capture_type_view_(AddChildView(std::make_unique<CaptureModeTypeView>())),
       separator_1_(AddChildView(std::make_unique<views::Separator>())),
       capture_source_view_(
           AddChildView(std::make_unique<CaptureModeSourceView>())),
       separator_2_(AddChildView(std::make_unique<views::Separator>())),
-      close_button_(AddChildView(std::make_unique<CaptureModeCloseButton>(
-          base::BindRepeating(&CaptureModeBarView::OnButtonPressed,
-                              base::Unretained(this))))) {
+      close_button_(AddChildView(std::make_unique<CaptureModeButton>(
+          base::BindRepeating(&CaptureModeBarView::OnCloseButtonPressed,
+                              base::Unretained(this)),
+          kCloseButtonIcon))) {
   SetPaintToLayer();
   auto* color_provider = AshColorProvider::Get();
   SkColor background_color = color_provider->GetBaseLayerColor(
@@ -66,6 +75,8 @@ CaptureModeBarView::CaptureModeBarView()
 
   const SkColor separator_color = color_provider->GetContentLayerColor(
       AshColorProvider::ContentLayerType::kSeparatorColor);
+  separator_0_->SetColor(separator_color);
+  separator_0_->SetPreferredHeight(kSeparatorHeight);
   separator_1_->SetColor(separator_color);
   separator_1_->SetPreferredHeight(kSeparatorHeight);
   separator_2_->SetColor(separator_color);
@@ -85,6 +96,12 @@ gfx::Rect CaptureModeBarView::GetBounds(aura::Window* root) {
   return bounds;
 }
 
+void CaptureModeBarView::OnFeedbackButtonPressed() {
+  auto* controller = CaptureModeController::Get();
+  controller->OpenFeedbackDialog();
+  controller->Stop();
+}
+
 void CaptureModeBarView::OnCaptureSourceChanged(CaptureModeSource new_source) {
   capture_source_view_->OnCaptureSourceChanged(new_source);
 }
@@ -94,7 +111,7 @@ void CaptureModeBarView::OnCaptureTypeChanged(CaptureModeType new_type) {
   capture_source_view_->OnCaptureTypeChanged(new_type);
 }
 
-void CaptureModeBarView::OnButtonPressed() {
+void CaptureModeBarView::OnCloseButtonPressed() {
   CaptureModeController::Get()->Stop();
 }
 

ash/capture_mode/capture_mode_bar_view.h

@@ -16,7 +16,7 @@ class Separator;
 
 namespace ash {
 
-class CaptureModeCloseButton;
+class CaptureModeButton;
 class CaptureModeSourceView;
 class CaptureModeTypeView;
 
@@ -33,7 +33,7 @@ class CaptureModeTypeView;
 //   |  +----------------+  |  ^                 ^  |  ^      |
 //   +--^----------------------|-----------------|-----|------+
 //   ^  |                      +-----------------+     |
-//   |  |                      |                       CaptureModeCloseButton
+//   |  |                      |                       CaptureModeButton
 //   |  |                      CaptureModeSourceView
 //   |  CaptureModeTypeView
 //   |
@@ -52,7 +52,6 @@ class ASH_EXPORT CaptureModeBarView : public views::View {
   CaptureModeSourceView* capture_source_view() const {
     return capture_source_view_;
   }
-  CaptureModeCloseButton* close_button() const { return close_button_; }
 
   // Gets the ideal bounds in screen coordinates of the bar of widget on the
   // given |root| window.
@@ -62,15 +61,23 @@ class ASH_EXPORT CaptureModeBarView : public views::View {
   void OnCaptureSourceChanged(CaptureModeSource new_source);
   void OnCaptureTypeChanged(CaptureModeType new_type);
 
+  CaptureModeButton* feedback_button_for_testing() const {
+    return feedback_button_;
+  }
+  CaptureModeButton* close_button_for_testing() const { return close_button_; }
+
  private:
-  void OnButtonPressed();
+  void OnFeedbackButtonPressed();
+  void OnCloseButtonPressed();
 
   // Owned by the views hierarchy.
+  CaptureModeButton* feedback_button_;
+  views::Separator* separator_0_;
   CaptureModeTypeView* capture_type_view_;
   views::Separator* separator_1_;
   CaptureModeSourceView* capture_source_view_;
   views::Separator* separator_2_;
-  CaptureModeCloseButton* close_button_;
+  CaptureModeButton* close_button_;
 };
 
 }  // namespace ash

ash/capture_mode/capture_mode_close_button.cc → ash/capture_mode/capture_mode_button.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "ash/capture_mode/capture_mode_close_button.h"
+#include "ash/capture_mode/capture_mode_button.h"
 
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/style/ash_color_provider.h"
@@ -13,8 +13,8 @@
 
 namespace ash {
 
-CaptureModeCloseButton::CaptureModeCloseButton(
-    views::Button::PressedCallback callback)
+CaptureModeButton::CaptureModeButton(views::Button::PressedCallback callback,
+                                     const gfx::VectorIcon& icon)
     : ViewWithInkDrop(callback) {
   SetPreferredSize(capture_mode::kButtonSize);
   SetBorder(views::CreateEmptyBorder(capture_mode::kButtonPadding));
@@ -22,7 +22,7 @@ CaptureModeCloseButton::CaptureModeCloseButton(
   const SkColor normal_color = color_provider->GetContentLayerColor(
       AshColorProvider::ContentLayerType::kButtonIconColor);
   SetImage(views::Button::STATE_NORMAL,
-           gfx::CreateVectorIcon(kCloseButtonIcon, normal_color));
+           gfx::CreateVectorIcon(icon, normal_color));
   SetImageHorizontalAlignment(ALIGN_CENTER);
   SetImageVerticalAlignment(ALIGN_MIDDLE);
   GetViewAccessibility().OverrideIsLeaf(true);
@@ -40,7 +40,7 @@ CaptureModeCloseButton::CaptureModeCloseButton(
                                              capture_mode::kButtonPadding);
 }
 
-BEGIN_METADATA(CaptureModeCloseButton, ViewWithInkDrop<views::ImageButton>)
+BEGIN_METADATA(CaptureModeButton, ViewWithInkDrop<views::ImageButton>)
 END_METADATA
 
 }  // namespace ash

ash/capture_mode/capture_mode_close_button.h → ash/capture_mode/capture_mode_button.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef ASH_CAPTURE_MODE_CAPTURE_MODE_CLOSE_BUTTON_H_
-#define ASH_CAPTURE_MODE_CAPTURE_MODE_CLOSE_BUTTON_H_
+#ifndef ASH_CAPTURE_MODE_CAPTURE_MODE_BUTTON_H_
+#define ASH_CAPTURE_MODE_CAPTURE_MODE_BUTTON_H_
 
 #include "ash/ash_export.h"
 #include "ash/capture_mode/view_with_ink_drop.h"
@@ -11,20 +11,25 @@
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/metadata/metadata_header_macros.h"
 
+namespace gfx {
+struct VectorIcon;
+}  // namespace gfx
+
 namespace ash {
 
-// A view that shows a close button which is part of the CaptureBarView.
-class ASH_EXPORT CaptureModeCloseButton
+// A view that shows a button which is part of the CaptureBarView.
+class ASH_EXPORT CaptureModeButton
     : public ViewWithInkDrop<views::ImageButton> {
  public:
-  METADATA_HEADER(CaptureModeCloseButton);
+  METADATA_HEADER(CaptureModeButton);
 
-  explicit CaptureModeCloseButton(views::Button::PressedCallback callback);
-  CaptureModeCloseButton(const CaptureModeCloseButton&) = delete;
-  CaptureModeCloseButton& operator=(const CaptureModeCloseButton&) = delete;
-  ~CaptureModeCloseButton() override = default;
+  CaptureModeButton(views::Button::PressedCallback callback,
+                    const gfx::VectorIcon& icon);
+  CaptureModeButton(const CaptureModeButton&) = delete;
+  CaptureModeButton& operator=(const CaptureModeButton&) = delete;
+  ~CaptureModeButton() override = default;
 };
 
 }  // namespace ash
 
-#endif  // ASH_CAPTURE_MODE_CAPTURE_MODE_CLOSE_BUTTON_H_
+#endif  // ASH_CAPTURE_MODE_CAPTURE_MODE_BUTTON_H_

ash/capture_mode/capture_mode_controller.cc

@@ -352,6 +352,10 @@ void CaptureModeController::EndVideoRecording() {
                            weak_ptr_factory_.GetWeakPtr()));
 }
 
+void CaptureModeController::OpenFeedbackDialog() {
+  delegate_->OpenFeedbackDialog();
+}
+
 bool CaptureModeController::IsCaptureAllowed() const {
   const base::Optional<CaptureParams> capture_params = GetCaptureParams();
   if (!capture_params)

ash/capture_mode/capture_mode_controller.h

@@ -76,6 +76,9 @@ class ASH_EXPORT CaptureModeController {
 
   void EndVideoRecording();
 
+  // Called when the feedback button on the capture bar is pressed.
+  void OpenFeedbackDialog();
+
  private:
   // Returns true if doing a screen capture is currently allowed, false
   // otherwise.

ash/capture_mode/capture_mode_unittests.cc

@@ -5,7 +5,7 @@
 #include <memory>
 
 #include "ash/capture_mode/capture_mode_bar_view.h"
-#include "ash/capture_mode/capture_mode_close_button.h"
+#include "ash/capture_mode/capture_mode_button.h"
 #include "ash/capture_mode/capture_mode_controller.h"
 #include "ash/capture_mode/capture_mode_session.h"
 #include "ash/capture_mode/capture_mode_source_view.h"
@@ -160,10 +160,16 @@ class CaptureModeTest : public AshTestBase {
         ->window_toggle_button();
   }
 
-  CaptureModeCloseButton* GetCloseButton() const {
+  CaptureModeButton* GetFeedbackButton() const {
     auto* controller = CaptureModeController::Get();
     DCHECK(controller->IsActive());
-    return GetCaptureModeBarView()->close_button();
+    return GetCaptureModeBarView()->feedback_button_for_testing();
+  }
+
+  CaptureModeButton* GetCloseButton() const {
+    auto* controller = CaptureModeController::Get();
+    DCHECK(controller->IsActive());
+    return GetCaptureModeBarView()->close_button_for_testing();
   }
 
   aura::Window* GetDimensionsLabelWindow() const {
@@ -263,6 +269,15 @@ TEST_F(CaptureModeTest, StartWithMostRecentTypeAndSource) {
   EXPECT_FALSE(controller->IsActive());
 }
 
+TEST_F(CaptureModeTest, FeedbackButtonExits) {
+  auto* controller = CaptureModeController::Get();
+  controller->Start(CaptureModeEntryType::kQuickSettings);
+  EXPECT_TRUE(controller->IsActive());
+
+  ClickOnView(GetFeedbackButton(), GetEventGenerator());
+  EXPECT_FALSE(controller->IsActive());
+}
+
 TEST_F(CaptureModeTest, ChangeTypeAndSourceFromUI) {
   auto* controller = CaptureModeController::Get();
   controller->Start(CaptureModeEntryType::kQuickSettings);

ash/capture_mode/test_capture_mode_delegate.cc

@@ -40,4 +40,6 @@ void TestCaptureModeDelegate::StartObservingRestrictedContent(
 
 void TestCaptureModeDelegate::StopObservingRestrictedContent() {}
 
+void TestCaptureModeDelegate::OpenFeedbackDialog() {}
+
 }  // namespace ash

ash/capture_mode/test_capture_mode_delegate.h

@@ -31,6 +31,7 @@ class TestCaptureModeDelegate : public CaptureModeDelegate {
       const gfx::Rect& bounds,
       base::OnceClosure stop_callback) override;
   void StopObservingRestrictedContent() override;
+  void OpenFeedbackDialog() override;
 };
 
 }  // namespace ash

ash/public/cpp/capture_mode_delegate.h

@@ -66,6 +66,9 @@ class ASH_PUBLIC_EXPORT CaptureModeDelegate {
 
   // Called when the running video capture is stopped.
   virtual void StopObservingRestrictedContent() = 0;
+
+  // Called when the feedback button is pressed.
+  virtual void OpenFeedbackDialog() = 0;
 };
 
 }  // namespace ash

ash/resources/vector_icons/BUILD.gn

@@ -30,6 +30,7 @@ aggregate_vector_icons("ash_vector_icons") {
     "battery.icon",
     "capture_mode.icon",
     "capture_mode_circle_stop.icon",
+    "capture_mode_feedback.icon",
     "capture_mode_fullscreen.icon",
     "capture_mode_image.icon",
     "capture_mode_region.icon",

ash/resources/vector_icons/capture_mode_feedback.icon

+// Copyright 2020 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.
+
+CANVAS_DIMENSIONS, 20,
+MOVE_TO, 9, 5,
+R_H_LINE_TO, 2,
+R_V_LINE_TO, 4,
+H_LINE_TO, 9,
+V_LINE_TO, 5,
+CLOSE,
+MOVE_TO, 9, 10,
+R_H_LINE_TO, 2,
+R_V_LINE_TO, 2,
+H_LINE_TO, 9,
+R_V_LINE_TO, -2,
+CLOSE,
+MOVE_TO, 3.67f, 2,
+H_LINE_TO, 16,
+R_CUBIC_TO, 0.92f, 0, 2, 0.58f, 2, 1.5f,
+R_V_LINE_TO, 10,
+R_CUBIC_TO, 0, 0.92f, -1.08f, 1.5f, -2, 1.5f,
+H_LINE_TO, 5,
+R_LINE_TO, -3, 2.5f,
+R_LINE_TO, 0.01f, -13.83f,
+CUBIC_TO, 2.01f, 2.75f, 2.75f, 2, 3.67f, 2,
+CLOSE,
+MOVE_TO, 4, 13,
+R_H_LINE_TO, 12,
+V_LINE_TO, 4,
+H_LINE_TO, 4,
+R_V_LINE_TO, 9,
+CLOSE

chrome/browser/ui/ash/chrome_capture_mode_delegate.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/ash/screenshot_area.h"
+#include "chrome/browser/ui/browser_commands.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/login/login_state/login_state.h"
 #include "components/prefs/pref_service.h"
@@ -104,3 +105,9 @@ void ChromeCaptureModeDelegate::StartObservingRestrictedContent(
 void ChromeCaptureModeDelegate::StopObservingRestrictedContent() {
   policy::DlpContentManager::Get()->OnVideoCaptureStopped();
 }
+
+void ChromeCaptureModeDelegate::OpenFeedbackDialog() {
+  chrome::OpenFeedbackDialog(/*browser=*/nullptr,
+                             chrome::kFeedbackSourceCaptureMode,
+                             /*description_template=*/"#ScreenCapture\n\n");
+}

chrome/browser/ui/ash/chrome_capture_mode_delegate.h

@@ -32,6 +32,7 @@ class ChromeCaptureModeDelegate : public ash::CaptureModeDelegate {
       const gfx::Rect& bounds,
       base::OnceClosure stop_callback) override;
   void StopObservingRestrictedContent() override;
+  void OpenFeedbackDialog() override;
 };
 
 #endif  // CHROME_BROWSER_UI_ASH_CHROME_CAPTURE_MODE_DELEGATE_H_

chrome/browser/ui/browser_commands.cc

@@ -1375,12 +1375,14 @@ void OpenTaskManager(Browser* browser) {
 #endif
 }
 
-void OpenFeedbackDialog(Browser* browser, FeedbackSource source) {
+void OpenFeedbackDialog(Browser* browser,
+                        FeedbackSource source,
+                        const std::string& description_template) {
   base::RecordAction(UserMetricsAction("Feedback"));
-  chrome::ShowFeedbackPage(
-      browser, source, std::string() /* description_template */,
-      std::string() /* description_placeholder_text */,
-      std::string() /* category_tag */, std::string() /* extra_diagnostics */);
+  chrome::ShowFeedbackPage(browser, source, description_template,
+                           std::string() /* description_placeholder_text */,
+                           std::string() /* category_tag */,
+                           std::string() /* extra_diagnostics */);
 }
 
 void ToggleBookmarkBar(Browser* browser) {

chrome/browser/ui/browser_commands.h

@@ -186,7 +186,10 @@ void FocusPreviousPane(Browser* browser);
 void ToggleDevToolsWindow(Browser* browser, DevToolsToggleAction action);
 bool CanOpenTaskManager();
 void OpenTaskManager(Browser* browser);
-void OpenFeedbackDialog(Browser* browser, FeedbackSource source);
+void OpenFeedbackDialog(
+    Browser* browser,
+    FeedbackSource source,
+    const std::string& description_template = std::string());
 void ToggleBookmarkBar(Browser* browser);
 void ToggleShowFullURLs(Browser* browser);
 void ShowAppMenu(Browser* browser);

chrome/browser/ui/chrome_pages.h

@@ -67,6 +67,7 @@ enum FeedbackSource {
   kFeedbackSourceNetworkHealthPage,
   kFeedbackSourceTabSearch,
   kFeedbackSourceCameraApp,
+  kFeedbackSourceCaptureMode,
 
   // Must be last.
   kFeedbackSourceCount,

tools/metrics/histograms/enums.xml

@@ -30243,6 +30243,14 @@ Called by update_feature_policy_enum.py.-->
   <int value="6" label="SadTab Page"/>
   <int value="7" label="Supervised User Interstitial"/>
   <int value="8" label="Assistant"/>
+  <int value="9" label="Desktop Tab Groups"/>
+  <int value="10" label="Media App"/>
+  <int value="11" label="Help App"/>
+  <int value="12" label="Kaleidoscope"/>
+  <int value="13" label="Network Health Page"/>
+  <int value="14" label="Tab Search"/>
+  <int value="15" label="Camera App"/>
+  <int value="16" label="Capture Mode"/>
 </enum>
 
 <enum name="FeedControlsActions">