Refactor ViewsDelegate and MD-ify the icon-to-text spacing for checkbox and radiobutton

ui/views/views_delegate.h

 // Copyright (c) 2012 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.
 
 #ifndef UI_VIEWS_VIEWS_DELEGATE_H_
 #define UI_VIEWS_VIEWS_DELEGATE_H_
 
 #include <string>
 
 #if defined(OS_WIN)
@@ skipping 40 matching lines @@
 class Widget;
 
 #if defined(USE_AURA)
 class TouchSelectionMenuRunnerViews;
 #endif
 
 namespace internal {
 class NativeWidgetDelegate;
 }
 
+enum class InsetsMetric {
+  // The insets that should be applied around a DialogClientView. Note that
+  // the top inset is used for the distance between the buttons and the
+  // DialogClientView's content view.
+  DIALOG_BUTTON,
+  // The insets that should be applied around a dialog's frame view.
+  DIALOG_FRAME_VIEW,
+  // The margins that should be applied around a bubble dialog.

[Peter Kasting, 2017/03/01 06:32:36]

What is a "bubble dialog"?  Is that what the UX folks are trying to rename a
popover?  How is that different from a non-bubble dialog?

I mostly ask all this because "bubble" is a confusing term in our codebase and
I'm hoping to excise it, or at least prevent it spreading.

[kylix_rd, 2017/03/01 17:33:27]

Yes, I think the UX folks are using "popover" as a synonym for "bubble dialog".

AFICT, the bubbles are used for the pop-overs related to the content error/state
glyphs which appear in the OmniBox such as the Register Protocol Handler glyph,
blocked flash content, etc...
 
It's used in the various "bubble_dialog_xxxx" modules, so I kept the naming
consistent. If the goal is to excise "bubble" from the codebase, that should be
a focused, single purpose effort separate from the goals here.

[Peter Kasting, 2017/03/01 23:32:23]

That's fair.

+  BUBBLE_DIALOG,
+};
+
+enum class SpacingMetric {

[Peter Kasting, 2017/03/01 06:32:36]

Nit: I suggest naming these to closely align with the layout_constants names.

[kylix_rd, 2017/03/01 17:33:27]

Some of those constant names contain the word "SPACING" which creates a naming
redundancy. Using an enum class to provide the categorical context along with
the specific metric seems more clear and less redundant. I don't want to have a
name such as SpacingMetric::ICON_TEXT_SPACING. That looks like; "Department of
Redundancy Department"

[Peter Kasting, 2017/03/01 23:32:23]

That's fine, I didn't mean you had to copy names exactly.  Dropping SPACING or
something is OK.  (Although perhaps the problem is that this enum is
"SpacingMetric" instead of "IntegralMetric" or the like, so the fix is really to
rename the enum to just mean "some integer value").

But having e.g. RELATED_HORIZONTAL_BUTTON here versus
RELATED_BUTTON_HORIZONTAL[_SPACING] is a problem because it changes both the
alphabetization and, more importantly, the meaning.  A "horizontal button"
versus a "related button" are different things :)

+  // The distance between a dialog's edge and the close button in the upper
+  // trailing corner.
+  CLOSE_BUTTON_MARGIN,
+  // The spacing between a pair of related horizontal buttons, used for
+  // dialog layout.
+  RELATED_HORIZONTAL_BUTTON,
+  // The spacing between a pair of related vertical controls, used for
+  // dialog layout.
+  RELATED_VERTICAL_CONTROL,
+  // The spacing between the image on a label button and its associated
+  // text.
+  ICON_TO_TEXT,
+  // Returns the default minimum width of a dialog button.

[Peter Kasting, 2017/03/01 06:32:36]

Nit: No "Returns" (2 places)

[kylix_rd, 2017/03/01 17:33:27]

Done.

+  DIALOG_BUTTON_MINIMUM_WIDTH,
+  // Returns the default padding to add on each side of a button's label.
+  BUTTON_HORIZONTAL_PADDING,
+};
+
 // ViewsDelegate is an interface implemented by an object using the views
 // framework. It is used to obtain various high level application utilities
 // and perform some actions such as window placement saving.
 //
 // The embedding app must set the ViewsDelegate instance by instantiating an
 // implementation of ViewsDelegate (the constructor will set the instance).
 class VIEWS_EXPORT ViewsDelegate {
  public:
   using NativeWidgetFactory =
       base::Callback<NativeWidget*(const Widget::InitParams&,
@@ skipping 118 matching lines @@
   // not changed, |callback| is never run.
   //
   // The return value is a bitmask of AppbarAutohideEdge.
   virtual int GetAppbarAutohideEdges(HMONITOR monitor,
                                      const base::Closure& callback);
 #endif
 
   // Returns a blocking pool task runner given a TaskRunnerType.
   virtual scoped_refptr<base::TaskRunner> GetBlockingPoolTaskRunner();
 
-  // Returns the insets that should be applied around a DialogClientView. Note
-  // that the top inset is used for the distance between the buttons and the
-  // DialogClientView's content view.
-  virtual gfx::Insets GetDialogButtonInsets() const;
+  // Returns the insets metric according to the given enumeration element.
+  virtual gfx::Insets GetInsetsMetric(InsetsMetric metric) const;
 
-  // Returns the distance between a dialog's edge and the close button in the
-  // upper trailing corner.
-  virtual int GetDialogCloseButtonMargin() const;
+  // Returns the spacing metric according to the given enumeration element.
+  virtual int GetSpacingMetric(SpacingMetric metric) const;
 
-  // Returns the spacing between a pair of related horizontal buttons, used for
-  // dialog layout.
-  virtual int GetDialogRelatedButtonHorizontalSpacing() const;
-
-  // Returns the spacing between a pair of related vertical controls, used for
-  // dialog layout.
-  virtual int GetDialogRelatedControlVerticalSpacing() const;
-
-  // Returns the insets that should be applied around a dialog's frame view.
-  virtual gfx::Insets GetDialogFrameViewInsets() const;
-
-  // Returns the margins that should be applied around a bubble dialog.
-  virtual gfx::Insets GetBubbleDialogMargins() const;
-
-  // Returns the default minimum width of a button.
-  virtual int GetButtonMinimumWidth() const;
-
-  // Returns the minimum width of a dialog button.
-  virtual int GetDialogButtonMinimumWidth() const;
-
-  // Returns the default padding to add on each side of a button's label.
-  virtual int GetButtonHorizontalPadding() const;
+  // Returns the default metric in instances where a delegate hasn't been
+  // instantiated.
+  static int GetDefaultSpacingMetric(SpacingMetric metric);

[sky, 2017/02/28 20:11:52]

Now that you've made it so ViewsDelegate is always created is this still needed?

[kylix_rd, 2017/03/01 17:33:27]

Done.

 
  protected:
   ViewsDelegate();
 
  private:
   std::unique_ptr<ViewsTouchEditingControllerFactory> views_tsc_factory_;
 
 #if defined(USE_AURA)
   std::unique_ptr<TouchSelectionMenuRunnerViews> touch_selection_menu_runner_;
 #endif
 
   NativeWidgetFactory native_widget_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ViewsDelegate);
 };
 
 }  // namespace views
 
 #endif  // UI_VIEWS_VIEWS_DELEGATE_H_