Broke out layout metric information from ViewsDelegate to LayoutProvider

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

Index: chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc
diff --git a/chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc b/chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a73d46a0b32dad2802387b816b637488600217c1
--- /dev/null
+++ b/chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc
@@ -0,0 +1,143 @@
+// Copyright 2017 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/chrome_views_layout_delegate.h"
+
+#include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "chrome/browser/ui/views/harmony/chrome_typography.h"
+#include "chrome/browser/ui/views/harmony/harmony_views_layout_delegate.h"
+#include "ui/base/material_design/material_design_controller.h"
+#include "ui/views/layout/layout_constants.h"
+
+namespace {
+

[Peter Kasting, 2017/04/04 02:08:52]

Nit: Remove blank line

+ChromeViewsLayoutDelegate* layout_delegate_ = nullptr;

[Peter Kasting, 2017/04/04 02:08:53]

Nit: I'd name this |g_layout_delegate| since it's more like a global than a
member

+}
+
+ChromeViewsLayoutDelegate::ChromeViewsLayoutDelegate() {
+  layout_delegate_ = this;
+}
+
+ChromeViewsLayoutDelegate::~ChromeViewsLayoutDelegate() {
+  if (this == layout_delegate_)
+    layout_delegate_ = nullptr;
+}
+
+// static
+ChromeViewsLayoutDelegate* ChromeViewsLayoutDelegate::Get() {
+  return layout_delegate_;
+}
+
+// static
+std::unique_ptr<views::ViewsLayoutDelegate>
+ChromeViewsLayoutDelegate::CreateLayoutDelegate() {
+  return ui::MaterialDesignController::IsSecondaryUiMaterial()
+             ? base::MakeUnique<HarmonyViewsLayoutDelegate>()
+             : base::MakeUnique<ChromeViewsLayoutDelegate>();
+}
+
+gfx::Insets ChromeViewsLayoutDelegate::GetInsetsMetric(
+    ChromeInsetsMetric metric) const {
+  switch (metric) {
+    case ChromeInsetsMetric::DIALOG_BUTTON:
+    case ChromeInsetsMetric::DIALOG_FRAME_VIEW:
+    case ChromeInsetsMetric::BUBBLE_DIALOG:
+    case ChromeInsetsMetric::VECTOR_IMAGE_BUTTON:
+      return views::ViewsLayoutDelegate::GetInsetsMetric(
+          static_cast<views::InsetsMetric>(metric));

[Peter Kasting, 2017/04/04 02:08:52]

Nit: A blank line below this top group (in each of these two functions) might
make it more obvious where the break is between the "forwarding part" of the
switch and the part that handles constants specific to the chrome side.

+    case ChromeInsetsMetric::PANEL_LAYOUT:
+      return gfx::Insets(views::kPanelHorizMargin,

[Peter Kasting, 2017/04/04 02:08:53]

Nit: Shouldn't this be kPanelVertMargin?

[kylix_rd, 2017/04/04 20:28:23]

Maybe... however, this is copied from layout_utils::CreatePanelLayout(), which
directly constructed a gfx::Insets() with PANEL_CONTENT_MARGIN. In the original
LayoutDelegate, that enum value returned views::kPanelHorizMargin from
GetMetric().

I added PANEL_LAYOUT as a new insets metric and simply used the original values
for the non-Harmony mode. Maybe PANEL_CONTENT_MARGIN was wrong?

[Peter Kasting, 2017/04/05 19:30:10]

kPanelVertMargin and kPanelHorizMargin had the same value, so the goal of
PANEL_CONTENT_MARGIN was to provide a single value to collapse the two together,
to eliminate the separate concept.

So this suggestion (to use kPanelVertMargin) would have no functional effect,
but would be more semantically correct if you're using the underlying separate
constants instead of the "new" joint metric.

+                         views::kButtonHEdgeMarginNew);

[Peter Kasting, 2017/04/04 02:08:53]

Nit: Seems like these should use GetDistanceMetric()

+  }
+  NOTREACHED();
+  return gfx::Insets();
+}
+
+int ChromeViewsLayoutDelegate::GetDistanceMetric(
+    ChromeDistanceMetric metric) const {
+  switch (metric) {
+    case ChromeDistanceMetric::BUTTON_HORIZONTAL_PADDING:
+    case ChromeDistanceMetric::CLOSE_BUTTON_MARGIN:
+    case ChromeDistanceMetric::DIALOG_BUTTON_MINIMUM_WIDTH:
+    case ChromeDistanceMetric::RELATED_BUTTON_HORIZONTAL:
+    case ChromeDistanceMetric::RELATED_CONTROL_HORIZONTAL:
+    case ChromeDistanceMetric::RELATED_CONTROL_VERTICAL:
+      return views::ViewsLayoutDelegate::GetDistanceMetric(
+          static_cast<views::DistanceMetric>(metric));
+    case ChromeDistanceMetric::RELATED_CONTROL_HORIZONTAL_SMALL:
+      return views::kRelatedControlSmallHorizontalSpacing;
+    case ChromeDistanceMetric::RELATED_CONTROL_VERTICAL_SMALL:
+      return views::kRelatedControlSmallVerticalSpacing;
+    case ChromeDistanceMetric::DIALOG_BUTTON_MARGIN:
+      return views::kButtonHEdgeMarginNew;
+    case ChromeDistanceMetric::DIALOG_BUTTON_TOP:
+      return 0;
+    case ChromeDistanceMetric::BUTTON_MAX_LINKABLE_WIDTH:
+      return 0;
+    case ChromeDistanceMetric::BUTTON_MINIMUM_WIDTH:
+      return views::kMinimumButtonWidth;

[Peter Kasting, 2017/04/04 02:08:53]

I think after this patch this constant is only present in this file.  If so,
this CL or a followup one should go ahead and move this and any similarly-unique
constants into this function/file.

[kylix_rd, 2017/04/04 20:28:23]

Eventually, all the views::kXXXX constants will be made private and moved to the
appropriate xxx_layout_delegate.cc file(s). This can be done at that point.

+    case ChromeDistanceMetric::RELATED_LABEL_HORIZONTAL:
+      return views::kItemLabelSpacing;
+    case ChromeDistanceMetric::SUBSECTION_HORIZONTAL_INDENT:
+      return views::kCheckboxIndent;
+    case ChromeDistanceMetric::PANEL_CONTENT_MARGIN:
+      return views::kPanelHorizMargin;
+    case ChromeDistanceMetric::UNRELATED_CONTROL_HORIZONTAL:
+      return views::kUnrelatedControlHorizontalSpacing;
+    case ChromeDistanceMetric::UNRELATED_CONTROL_HORIZONTAL_LARGE:
+      return views::kUnrelatedControlLargeHorizontalSpacing;
+    case ChromeDistanceMetric::UNRELATED_CONTROL_VERTICAL:
+      return views::kUnrelatedControlVerticalSpacing;
+    case ChromeDistanceMetric::UNRELATED_CONTROL_VERTICAL_LARGE:
+      return views::kUnrelatedControlLargeVerticalSpacing;
+  }
+  NOTREACHED();
+  return 0;
+}
+
+const views::TypographyProvider&
+ChromeViewsLayoutDelegate::GetTypographyProvider() const {
+  // This is not a data member because then HarmonyLayoutDelegate would inherit
+  // it, even when it provides its own.
+  CR_DEFINE_STATIC_LOCAL(LegacyTypographyProvider, legacy_provider, ());
+  return legacy_provider;
+}
+
+views::GridLayout::Alignment
+ChromeViewsLayoutDelegate::GetControlLabelGridAlignment() const {
+  return views::GridLayout::TRAILING;
+}
+
+bool ChromeViewsLayoutDelegate::UseExtraDialogPadding() const {
+  return true;
+}
+
+bool ChromeViewsLayoutDelegate::ShouldShowWindowIcon() const {
+  return true;
+}
+
+bool ChromeViewsLayoutDelegate::IsHarmonyMode() const {
+  return false;
+}
+
+int ChromeViewsLayoutDelegate::GetDialogPreferredWidth(
+    DialogWidth width) const {
+  return 0;
+}
+
+int ChromeViewsLayoutDelegate::GetDefaultDistanceMetric(
+    views::DistanceMetric metric) const {
+  return views::ViewsLayoutDelegate::GetDistanceMetric(metric);
+}
+
+gfx::Insets ChromeViewsLayoutDelegate::GetInsetsMetric(
+    views::InsetsMetric metric) const {
+  return GetInsetsMetric(static_cast<ChromeInsetsMetric>(metric));
+}
+
+int ChromeViewsLayoutDelegate::GetDistanceMetric(
+    views::DistanceMetric metric) const {
+  return GetDistanceMetric(static_cast<ChromeDistanceMetric>(metric));
+}