views: make ColorChooserView neither a WidgetDelegate nor a View.

This CL enacts the "class splitting refactor"
(//docs/ui/views/class_splitting.md) on ColorChooserView, breaking it
into:

* A controller class called ColorChooser, whose lifetime is fully
  controlled by the client, and which is safe to call at any point in
  the lifecycle of the matching Widget/Views/etc
* Two helper objects (instances of WidgetDelegate and View respectively)
  whose lifetimes are fully controlled by Views

This CL serves as a prototype for how the class splitting refactor looks
when applied to production code.

Instead, have it use only the public API on WidgetDelegate.

Bug: 1075649
Change-Id: I84655a9c204a9f5f88767ac792ce1abc846feda0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622995
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#846358}

chrome/browser/ui/views/color_chooser_aura.cc

@@ -9,33 +9,30 @@
 #include "content/public/browser/web_contents.h"
 #include "ui/views/color_chooser/color_chooser_view.h"
 #include "ui/views/widget/widget.h"
+#include "ui/views/widget/widget_delegate.h"
 
 ColorChooserAura::ColorChooserAura(content::WebContents* web_contents,
                                    SkColor initial_color)
     : web_contents_(web_contents) {
-  view_ = new views::ColorChooserView(this, initial_color);
-  widget_ = views::Widget::CreateWindowWithParent(
-      view_, web_contents->GetTopLevelNativeWindow());
-  widget_->Show();
+  chooser_ = std::make_unique<views::ColorChooser>(this, initial_color);
+  chooser_widget_ = base::WrapUnique(views::Widget::CreateWindowWithParent(
+      chooser_->MakeWidgetDelegate(), web_contents->GetTopLevelNativeWindow()));
+  chooser_widget_->Show();
 }
 
+ColorChooserAura::~ColorChooserAura() = default;
+
 void ColorChooserAura::OnColorChosen(SkColor color) {
   if (web_contents_)
     web_contents_->DidChooseColorInColorChooser(color);
 }
 
 void ColorChooserAura::OnColorChooserDialogClosed() {
-  view_ = nullptr;
-  widget_ = nullptr;
   DidEndColorChooser();
 }
 
 void ColorChooserAura::End() {
-  if (widget_) {
-    view_->set_listener(nullptr);
-    widget_->Close();
-    view_ = nullptr;
-    widget_ = nullptr;
+  if (chooser_widget_) {
     // DidEndColorChooser will invoke Browser::DidEndColorChooser, which deletes
     // this. Take care of the call order.
     DidEndColorChooser();
@@ -48,8 +45,7 @@ void ColorChooserAura::DidEndColorChooser() {
 }
 
 void ColorChooserAura::SetSelectedColor(SkColor color) {
-  if (view_)
-    view_->OnColorChanged(color);
+  chooser_->OnColorChanged(color);
 }
 
 // static

chrome/browser/ui/views/color_chooser_aura.h

@@ -9,14 +9,14 @@
 #include "base/macros.h"
 #include "content/public/browser/color_chooser.h"
 #include "ui/views/color_chooser/color_chooser_listener.h"
+#include "ui/views/widget/unique_widget_ptr.h"
 
 namespace content {
 class WebContents;
 }
 
 namespace views {
-class ColorChooserView;
-class Widget;
+class ColorChooser;
 }
 
 // TODO(mukai): rename this as -Ash and move to c/b/ui/ash after Linux-aura
@@ -29,6 +29,7 @@ class ColorChooserAura : public content::ColorChooser,
 
  private:
   ColorChooserAura(content::WebContents* web_contents, SkColor initial_color);
+  ~ColorChooserAura() override;
 
   // content::ColorChooser overrides:
   void End() override;
@@ -40,13 +41,8 @@ class ColorChooserAura : public content::ColorChooser,
 
   void DidEndColorChooser();
 
-  // The actual view of the color chooser.  No ownership because its parent
-  // view will take care of its lifetime.
-  views::ColorChooserView* view_;
-
-  // The widget for the color chooser.  No ownership because it's released
-  // automatically when closed.
-  views::Widget* widget_;
+  std::unique_ptr<views::ColorChooser> chooser_;
+  views::UniqueWidgetPtr chooser_widget_;
 
   // The web contents invoking the color chooser.  No ownership because it will
   // outlive this class.

ui/views/color_chooser/color_chooser_unittest.cc

@@ -22,6 +22,7 @@
 #include "ui/views/color_chooser/color_chooser_view.h"
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/test/views_test_base.h"
+#include "ui/views/widget/widget_delegate.h"
 #include "ui/views/widget/widget_utils.h"
 
 namespace {
@@ -42,11 +43,18 @@ class ColorChooserTest : public views::ViewsTestBase {
 
   void SetUp() override {
     ViewsTestBase::SetUp();
-    chooser_ =
-        std::make_unique<views::ColorChooserView>(&listener_, SK_ColorGREEN);
-    chooser_->SetBounds(0, 0, 400, 300);
+    chooser_ = std::make_unique<views::ColorChooser>(&listener_, SK_ColorGREEN);
+
+    // Icky: we can't use our own WidgetDelegate for CreateTestWidget, but we
+    // want to follow the production code path here regardless, so we create our
+    // own delegate, pull the contents view out of it, and stick it into the
+    // test widget. In production Views would handle that step itself.
+    auto delegate = chooser_->MakeWidgetDelegate();
+    auto* view = delegate->TransferOwnershipOfContentsView();
+
+    view->SetBounds(0, 0, 400, 300);
     widget_ = CreateTestWidget(views::Widget::InitParams::TYPE_WINDOW);
-    widget_->GetContentsView()->AddChildView(chooser());
+    widget_->GetContentsView()->AddChildView(std::move(view));
     generator_ = std::make_unique<ui::test::EventGenerator>(
         views::GetRootWindow(widget_.get()), widget_->GetNativeWindow());
     generator_->set_assume_window_at_origin(false);
@@ -58,7 +66,7 @@ class ColorChooserTest : public views::ViewsTestBase {
     ViewsTestBase::TearDown();
   }
 
-  views::ColorChooserView* chooser() { return chooser_.get(); }
+  views::ColorChooser* chooser() { return chooser_.get(); }
   ui::test::EventGenerator* generator() { return generator_.get(); }
 
   void ExpectExactHSV(float h, float s, float v) const {
@@ -115,7 +123,7 @@ class ColorChooserTest : public views::ViewsTestBase {
 
  private:
   TestChooserListener listener_;
-  std::unique_ptr<views::ColorChooserView> chooser_;
+  std::unique_ptr<views::ColorChooser> chooser_;
   std::unique_ptr<views::Widget> widget_;
   std::unique_ptr<ui::test::EventGenerator> generator_;
 };

ui/views/color_chooser/color_chooser_view.h

@@ -5,86 +5,102 @@
 #ifndef UI_VIEWS_COLOR_CHOOSER_COLOR_CHOOSER_VIEW_H_
 #define UI_VIEWS_COLOR_CHOOSER_COLOR_CHOOSER_VIEW_H_
 
+#include <memory>
+
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "third_party/skia/include/core/SkScalar.h"
 #include "ui/views/controls/textfield/textfield_controller.h"
+#include "ui/views/view.h"
+#include "ui/views/view_tracker.h"
 #include "ui/views/views_export.h"
-#include "ui/views/widget/widget_delegate.h"
 
 namespace views {
 
 class ColorChooserListener;
 class Textfield;
+class WidgetDelegate;
+
+class HueView;
+class SaturationValueView;
+class SelectedColorPatchView;
 
-// ColorChooserView provides the UI to choose a color by mouse and/or keyboard.
+// ColorChooser provides the UI to choose a color by mouse and/or keyboard.
 // It is typically used for <input type="color">.  Currently the user can
 // choose a color by dragging over the bar for hue and the area for saturation
 // and value.
-class VIEWS_EXPORT ColorChooserView : public WidgetDelegateView,
-                                      public TextfieldController {
+//
+// All public methods on ColorChooser are safe to call before, during, or after
+// the existence of the corresponding Widget/Views/etc.
+class VIEWS_EXPORT ColorChooser : public TextfieldController,
+                                  public base::SupportsWeakPtr<ColorChooser> {
  public:
-  ColorChooserView(ColorChooserListener* listener, SkColor initial_color);
-  ~ColorChooserView() override;
-
-  // Called when its color value is changed in the web contents.
-  void OnColorChanged(SkColor color);
+  ColorChooser(ColorChooserListener* listener, SkColor initial_color);
+  ~ColorChooser() override;
 
-  // Called when the user chooses a hue from the UI.
-  void OnHueChosen(SkScalar hue);
+  // Construct the WidgetDelegate that should be used to show the actual dialog
+  // for this ColorChooser. It is only safe to call this once per ColorChooser
+  // instance.
+  std::unique_ptr<WidgetDelegate> MakeWidgetDelegate();
 
-  // Called when the user chooses saturation/value from the UI.
-  void OnSaturationValueChosen(SkScalar saturation, SkScalar value);
+  SkColor GetColor() const;
+  SkScalar hue() const { return hsv_[0]; }
+  SkScalar saturation() const { return hsv_[1]; }
+  SkScalar value() const { return hsv_[2]; }
 
-  float hue() const { return hsv_[0]; }
-  float saturation() const { return hsv_[1]; }
-  float value() const { return hsv_[2]; }
-  void set_listener(ColorChooserListener* listener) { listener_ = listener; }
+  bool IsViewAttached() const;
 
-  View* hue_view_for_testing();
-  View* saturation_value_view_for_testing();
-  Textfield* textfield_for_testing();
-  View* selected_color_patch_for_testing();
+  // Called when its color value is changed in the web contents.
+  void OnColorChanged(SkColor color);
 
-  // TextfieldController overrides:
+  // TextfieldController overrides, public for testing:
   void ContentsChanged(Textfield* sender,
                        const base::string16& new_contents) override;
   bool HandleKeyEvent(Textfield* sender,
                       const ui::KeyEvent& key_event) override;
 
+  View* hue_view_for_testing();
+  View* saturation_value_view_for_testing();
+  Textfield* textfield_for_testing();
+  View* selected_color_patch_for_testing();
+
  private:
-  class HueView;
-  class SaturationValueView;
-  class SelectedColorPatchView;
+  std::unique_ptr<View> BuildView();
 
-  // WidgetDelegate overrides:
-  bool CanMinimize() const override;
-  View* GetInitiallyFocusedView() override;
-  void WindowClosing() override;
+  void SetColor(SkColor color);
+  void SetHue(SkScalar hue);
+  void SetSaturationValue(SkScalar saturation, SkScalar value);
+
+  void OnViewClosing();
+
+  // Called when the user chooses a hue from the UI.
+  void OnHueChosen(SkScalar hue);
+
+  // Called when the user chooses saturation/value from the UI.
+  void OnSaturationValueChosen(SkScalar saturation, SkScalar value);
 
   // The current color in HSV coordinate.
   SkScalar hsv_[3];
 
-  // The pointer to the current color chooser for callbacks.  It doesn't take
-  // ownership on |listener_| so the user of this class should take care of
-  // its lifetime.  See chrome/browser/ui/browser.cc for example.
   ColorChooserListener* listener_;
+  ViewTracker tracker_;
 
   // Child views. These are owned as part of the normal views hierarchy.
   // The view of hue chooser.
-  HueView* hue_;
+  HueView* hue_ = nullptr;
 
   // The view of saturation/value choosing area.
-  SaturationValueView* saturation_value_;
-
-  // The textfield to write the color explicitly.
-  Textfield* textfield_;
+  SaturationValueView* saturation_value_ = nullptr;
 
   // The rectangle to denote the selected color.
-  SelectedColorPatchView* selected_color_patch_;
+  SelectedColorPatchView* selected_color_patch_ = nullptr;
+
+  // The textfield to write the color explicitly.
+  Textfield* textfield_ = nullptr;
 
-  DISALLOW_COPY_AND_ASSIGN(ColorChooserView);
+  SkColor initial_color_;
 };
 
 }  // namespace views

ui/views/view_tracker.h

@@ -21,6 +21,7 @@ class VIEWS_EXPORT ViewTracker : public ViewObserver {
 
   void SetView(View* view);
   View* view() { return view_; }
+  const View* view() const { return view_; }
 
   // ViewObserver:
   void OnViewIsDeleting(View* observed_view) override;