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

chrome/browser/ui/views/harmony/layout_delegate.cc

 // Copyright 2016 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.
 
 #include "chrome/browser/ui/views/harmony/layout_delegate.h"
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "chrome/browser/ui/views/harmony/harmony_layout_delegate.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/views/layout/layout_constants.h"
+#include "ui/views/views_delegate.h"
 
 static base::LazyInstance<LayoutDelegate> layout_delegate_ =
     LAZY_INSTANCE_INITIALIZER;
 
 // static
 LayoutDelegate* LayoutDelegate::Get() {
   return ui::MaterialDesignController::IsSecondaryUiMaterial()
              ? HarmonyLayoutDelegate::Get()
              : layout_delegate_.Pointer();
 }
 
 int LayoutDelegate::GetMetric(Metric metric) const {
   switch (metric) {
     case Metric::BUTTON_HORIZONTAL_PADDING:
-      return 0;
+      return views::ViewsDelegate::GetDefaultSpacingMetric(
+          views::SpacingMetric::BUTTON_HORIZONTAL_PADDING);
     case Metric::DIALOG_BUTTON_MARGIN:
       return views::kButtonHEdgeMarginNew;
     case Metric::DIALOG_BUTTON_MINIMUM_WIDTH:
-      return views::kDialogMinimumButtonWidth;
+      return views::ViewsDelegate::GetDefaultSpacingMetric(
+          views::SpacingMetric::DIALOG_BUTTON_MINIMUM_WIDTH);
     case Metric::DIALOG_BUTTON_TOP_SPACING:
       return 0;
     case Metric::DIALOG_CLOSE_BUTTON_MARGIN:
-      return views::kCloseButtonMargin;
+      return views::ViewsDelegate::GetDefaultSpacingMetric(
+          views::SpacingMetric::CLOSE_BUTTON_MARGIN);
     case Metric::PANEL_CONTENT_MARGIN:
       return views::kPanelHorizMargin;
     case Metric::RELATED_BUTTON_HORIZONTAL_SPACING:
-      return views::kRelatedButtonHSpacing;
+      return views::ViewsDelegate::GetDefaultSpacingMetric(
+          views::SpacingMetric::RELATED_HORIZONTAL_BUTTON);
     case Metric::RELATED_CONTROL_HORIZONTAL_SPACING:
-      return views::kRelatedControlHorizontalSpacing;
+      return views::ViewsDelegate::GetDefaultSpacingMetric(
+          views::SpacingMetric::RELATED_VERTICAL_CONTROL);
     case Metric::RELATED_CONTROL_VERTICAL_SPACING:
       return views::kRelatedControlVerticalSpacing;
     case Metric::RELATED_LABEL_HORIZONTAL_SPACING:
       return views::kItemLabelSpacing;
     case Metric::SUBSECTION_HORIZONTAL_INDENT:
       return views::kCheckboxIndent;
     case Metric::UNRELATED_CONTROL_HORIZONTAL_SPACING:
       return views::kUnrelatedControlHorizontalSpacing;
     case Metric::UNRELATED_CONTROL_HORIZONTAL_SPACING_LARGE:
       return views::kUnrelatedControlLargeHorizontalSpacing;
     case Metric::UNRELATED_CONTROL_VERTICAL_SPACING:
       return views::kUnrelatedControlVerticalSpacing;
     case Metric::UNRELATED_CONTROL_VERTICAL_SPACING_LARGE:
       return views::kUnrelatedControlLargeVerticalSpacing;
+    case Metric::ICON_TEXT_SPACING:
+      return views::ViewsDelegate::GetDefaultSpacingMetric(

[sky, 2017/02/24 18:31:13]

I would rather see this code call
ChromeViewsDelegate::Get()->GetDefaultSpacingMetric(). Where
GetDefaultSpacingMetric() is private in ChromeViewsDelegate and this class is a
friend of ChromeViewsDelegate. That way GetDefaultSpacingMetric() is not public
and can't be accidentally called.

[kylix_rd, 2017/02/24 19:04:20]

I placed the function on ViewsDelegate as a static since it may need to be
called in cases where there is no ViewsDelegate instance setup. Some of the
unit-tests fail with access violations or seg-faults (the ViewsDelegate
singleton instance is nullptr).

For instance, the LabelButton control does this... I suppose it could fall back
to a literal constant if ViewsDelegate::GetInstance() returns nullptr.

[Peter Kasting, 2017/02/24 20:41:03]

We should fix the tests so there is always a ViewsDelegate instance.  That's
much nicer than trying to check for and handle null in various cases, which is
why we've started ripping out all the null checks of this everywhere.

+          views::SpacingMetric::ICON_TO_TEXT);
   }
   NOTREACHED();
   return 0;
 }
 
 views::GridLayout::Alignment LayoutDelegate::GetControlLabelGridAlignment()
     const {
   return views::GridLayout::TRAILING;
 }
 
 bool LayoutDelegate::UseExtraDialogPadding() const {
   return true;
 }
 
 bool LayoutDelegate::IsHarmonyMode() const {
   return false;
 }
 
 int LayoutDelegate::GetDialogPreferredWidth(DialogWidth width) const {
   return 0;
 }