Harmonize the find in page dialog.

chrome/browser/ui/views/find_bar_view.cc

Index: chrome/browser/ui/views/find_bar_view.cc
diff --git a/chrome/browser/ui/views/find_bar_view.cc b/chrome/browser/ui/views/find_bar_view.cc
index 49627d798f76cf6dbe42eead2924251379554333..d0a262eb76fdce616f7f4f46d00681f6d16d4aa7 100644
--- a/chrome/browser/ui/views/find_bar_view.cc
+++ b/chrome/browser/ui/views/find_bar_view.cc
@@ -21,6 +21,7 @@
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/find_bar_host.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/harmony/chrome_layout_provider.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/strings/grit/components_strings.h"
 #include "ui/base/ime/input_method.h"
@@ -42,20 +43,12 @@
 #include "ui/views/controls/separator.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/painter.h"
+#include "ui/views/view_properties.h"
 #include "ui/views/view_targeter.h"
 #include "ui/views/widget/widget.h"
 
 namespace {
 
-// These layout constants are all in dp.
-// The horizontal and vertical insets for the bar.
-const int kInteriorPadding = 8;
-// Default spacing between child views.
-const int kInterChildSpacing = 4;
-// Additional spacing around the separator.
-const int kSeparatorLeftSpacing = 12 - kInterChildSpacing;
-const int kSeparatorRightSpacing = 8 - kInterChildSpacing;
-
 // The default number of average characters that the text box will be.
 const int kDefaultCharWidth = 30;
 
@@ -161,15 +154,58 @@ FindBarView::FindBarView(FindBarHost* host)
       base::MakeUnique<views::ViewTargeter>(this));
   AddChildViewAt(match_count_text_, 1);
 
-  separator_->SetBorder(views::CreateEmptyBorder(0, kSeparatorLeftSpacing, 0,
-                                                 kSeparatorRightSpacing));
+  ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
+
   AddChildViewAt(separator_, 2);
 
+  // The vector image buttons have a border of 4 px. The box layout code
+  // spaces these controls based on the distance to the border of the control
+  // instead of the actual glyph. This looks visually wrong.
+  // To workaround this we space the controls using the desired margins.

[Peter Kasting, 2017/07/15 02:48:08]

Nit: Maybe this would be clearer?:

Normally we could space objects horizontally by simply passing a constant value
to BoxLayout for between-child spacing.  But for the vector image buttons, we
want the spacing to apply between the inner "glyph" portions of the buttons,
ignoring the surrounding borders.  BoxLayout has no way to dynamically adjust
for this, so instead of using between-child spacing, we place views directly
adjacent, with horizontal margins on each view that will add up to the right
spacing amounts.

[ananta, 2017/07/15 07:04:14]

Done.

+
+  int horizontal_margin =

[Peter Kasting, 2017/07/15 02:48:08]

A couple of these assume things like the vector image button or toast insets
being the same on both sides.  If we keep these all as Insets, we can avoid this
assumption:

  const gfx::Insets horizontal_margin(
      0, provider->GetDistanceMetric(
             views::DISTANCE_RELATED_CONTROL_HORIZONTAL) / 2);
  const gfx::Insets vector_button =
      provider->GetInsetsMetric(views::INSETS_VECTOR_IMAGE_BUTTON);
  const gfx::Insets vector_button_horizontal_margin(
      0, horizontal_margin.left() - vector_button.left(),
      0, horizontal_margin.right() - vector_button.right());
  const gfx::Insets toast_control_vertical_margin(
      provider->GetDistanceMetric(DISTANCE_TOAST_CONTROL_VERTICAL), 0);
  const gfx::Insets toast_label_vertical_margin(
      provider->GetDistanceMetric(DISTANCE_TOAST_LABEL_VERTICAL), 0);

  find_previous_button_->SetProperty(
      views::kMarginsKey,
      new gfx::Insets(toast_control_vertical_margin +
                      vector_button_horizontal_margin));
  ...

[ananta, 2017/07/15 07:04:14]

Thanks. done.

+      provider->GetDistanceMetric(views::DISTANCE_RELATED_CONTROL_HORIZONTAL) /

[Peter Kasting, 2017/07/15 02:48:08]

This should be UNRELATED rather than RELATED.  This will have no visible effect
in Harmony and should be closer to the current behavior pre-Harmony; plus it's
more semantically correct.

[ananta, 2017/07/15 07:04:14]

Done.

+      2;
+
+  int vector_button_horizontal_margin =
+      horizontal_margin -
+      provider->GetInsetsMetric(views::INSETS_VECTOR_IMAGE_BUTTON).left();
+
+  int left_right_margin =
+      provider->GetInsetsMetric(views::INSETS_TOAST).right() -
+      horizontal_margin;
+
+  const int toast_control_vertical_margin =
+      provider->GetDistanceMetric(DISTANCE_TOAST_CONTROL_VERTICAL);
+
+  const int toast_label_vertical_margin =
+      provider->GetDistanceMetric(DISTANCE_TOAST_LABEL_VERTICAL);
+
+  find_previous_button_->SetProperty(
+      views::kMarginsKey, new gfx::Insets(toast_control_vertical_margin,
+                                          vector_button_horizontal_margin));
+  find_next_button_->SetProperty(
+      views::kMarginsKey, new gfx::Insets(toast_control_vertical_margin,
+                                          vector_button_horizontal_margin));
+  close_button_->SetProperty(views::kMarginsKey,
+                             new gfx::Insets(toast_control_vertical_margin,
+                                             vector_button_horizontal_margin));
+  separator_->SetProperty(
+      views::kMarginsKey,
+      new gfx::Insets(toast_label_vertical_margin, horizontal_margin));

[Peter Kasting, 2017/07/15 02:48:08]

I think you should use the control margin rather than the label margin for this
one.  A separator is more like a vertical divider that should have button
height, rather than a | symbol or something.

[ananta, 2017/07/15 07:04:14]

Done.

+
+  find_text_->SetProperty(
+      views::kMarginsKey,
+      new gfx::Insets(toast_control_vertical_margin, horizontal_margin));
+
+  match_count_text_->SetProperty(
+      views::kMarginsKey,
+      new gfx::Insets(toast_label_vertical_margin, horizontal_margin));
+
   find_text_->SetBorder(views::NullBorder());
 
-  views::BoxLayout* manager =
-      new views::BoxLayout(views::BoxLayout::kHorizontal,
-                           gfx::Insets(kInteriorPadding), kInterChildSpacing);
+  views::BoxLayout* manager = new views::BoxLayout(
+      views::BoxLayout::kHorizontal, gfx::Insets(0, left_right_margin), 0);
   SetLayoutManager(manager);
   manager->SetFlexForView(find_text_, 1);
 }