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

ui/views/views_delegate.h

Index: ui/views/views_delegate.h
diff --git a/ui/views/views_delegate.h b/ui/views/views_delegate.h
index 7b3960f5c7c0a5be23772d6cc4b7d72f7064d5c2..ad53b28ce6dcf4a2061091f0a2a09004b2c6fa61 100644
--- a/ui/views/views_delegate.h
+++ b/ui/views/views_delegate.h
@@ -58,6 +58,36 @@ 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.
@@ -196,37 +226,15 @@ class VIEWS_EXPORT ViewsDelegate {
   // 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 distance between a dialog's edge and the close button in the
-  // upper trailing corner.
-  virtual int GetDialogCloseButtonMargin() 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 metric according to the given enumeration element.
+  virtual gfx::Insets GetInsetsMetric(InsetsMetric metric) const;
 
-  // Returns the insets that should be applied around a dialog's frame view.
-  virtual gfx::Insets GetDialogFrameViewInsets() const;
+  // Returns the spacing metric according to the given enumeration element.
+  virtual int GetSpacingMetric(SpacingMetric metric) 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();