Use an InsetMetric instead of a constant for vector button padding.

chrome/browser/ui/views/harmony/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/harmony_layout_delegate.h"
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 
 static base::LazyInstance<HarmonyLayoutDelegate>::DestructorAtExit
@@ skipping 13 matching lines @@
     case Metric::BUTTON_MAX_LINKABLE_WIDTH:
       return kHarmonyLayoutUnit * 8;
     case Metric::BUTTON_MINIMUM_WIDTH:
     case Metric::DIALOG_BUTTON_MINIMUM_WIDTH:
       // Minimum label size plus padding.
       return 2 * kHarmonyLayoutUnit +
              2 * GetMetric(Metric::BUTTON_HORIZONTAL_PADDING);
     case Metric::DIALOG_BUTTON_TOP_SPACING:
       return kHarmonyLayoutUnit;
     case Metric::DIALOG_CLOSE_BUTTON_MARGIN:
-      // TODO(pkasting): The "- 4" here is a hack that matches the extra padding
-      // in vector_icon_button.cc and should be removed when that padding is.
-      return (kHarmonyLayoutUnit / 2) - 4;
+      // The amount of padding doesn't affect the button's position, so subtract
+      // it out here.

[Peter Kasting, 2017/03/29 22:39:42]

Nit: Maybe clearer:

      constexpr int kVisibleMargin = kHarmonyLayoutUnit / 2;
      // The visible margin is based on the unpadded size, but the actual margin
      // needs to account for the padding.
      return kVisibleMargin - GetMetric(Metric::VECTOR_IMAGE_BUTTON_PADDING);

[Bret, 2017/03/29 23:06:11]

Done.

+      return (kHarmonyLayoutUnit / 2) -
+             GetMetric(Metric::VECTOR_IMAGE_BUTTON_PADDING);
     case Metric::PANEL_CONTENT_MARGIN:
       return kHarmonyLayoutUnit;
     case Metric::RELATED_BUTTON_HORIZONTAL_SPACING:
       return kHarmonyLayoutUnit / 2;
     case Metric::RELATED_CONTROL_HORIZONTAL_SPACING:
       return kHarmonyLayoutUnit;
     case Metric::RELATED_CONTROL_HORIZONTAL_SPACING_SMALL:
       return kHarmonyLayoutUnit;
     case Metric::RELATED_CONTROL_VERTICAL_SPACING:
       return kHarmonyLayoutUnit / 2;
     case Metric::RELATED_CONTROL_VERTICAL_SPACING_SMALL:
       return kHarmonyLayoutUnit / 2;
     case Metric::RELATED_LABEL_HORIZONTAL_SPACING:
       return kHarmonyLayoutUnit;
     case Metric::SUBSECTION_HORIZONTAL_INDENT:
       return 0;
     case Metric::UNRELATED_CONTROL_HORIZONTAL_SPACING:
       return kHarmonyLayoutUnit;
     case Metric::UNRELATED_CONTROL_HORIZONTAL_SPACING_LARGE:
       return kHarmonyLayoutUnit;
     case Metric::UNRELATED_CONTROL_VERTICAL_SPACING:
       return kHarmonyLayoutUnit;
     case Metric::UNRELATED_CONTROL_VERTICAL_SPACING_LARGE:
       return kHarmonyLayoutUnit;
+    case Metric::VECTOR_IMAGE_BUTTON_PADDING:
+      return 4;

[Peter Kasting, 2017/03/29 22:39:42]

Nit: It seems like if we scaled up the layout unit we'd probably scale this
value up too, so maybe use kHarmonyLayoutUnit / 4?

[Bret, 2017/03/29 23:06:11]

I went back and forth on this. It's not really a layout unit value, since it
results in a button size of 24 dips, it's just that making the button any
smaller is problematic. I think if the layout unit was 24 for example we'd be
happy keeping it as 4, and if we changed the vector asset size we'd want to
fiddle with it. So I'm going to leave it as just "4".

   }
   NOTREACHED();
   return 0;
 }
 
 views::GridLayout::Alignment
 HarmonyLayoutDelegate::GetControlLabelGridAlignment() const {
   return views::GridLayout::LEADING;
 }
 
@@ skipping 14 matching lines @@
     case DialogWidth::SMALL:
       return 320;
     case DialogWidth::MEDIUM:
       return 448;
     case DialogWidth::LARGE:
       return 512;
   }
   NOTREACHED();
   return 0;
 }