Last round of cleaning up MD layout constants.

chrome/browser/ui/layout_constants.cc

 // Copyright 2015 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/layout_constants.h"
 
 #include "base/logging.h"
 #include "build/build_config.h"
 #include "ui/base/material_design/material_design_controller.h"
 
 int GetLayoutConstant(LayoutConstant constant) {
-  const int kFindBarVerticalOffset[] = {6, 6};
-  const int kLocationBarBorderThickness[] = {1, 1};
-  const int kLocationBarBubbleFontVerticalPadding[] = {2, 4};
-  const int kLocationBarBubbleVerticalPadding[] = {3, 3};
-  const int kLocationBarBubbleAnchorVerticalInset[] = {6, 8};
-  const int kLocationBarHeight[] = {28, 32};
-  const int kLocationBarHorizontalPadding[] = {6, 6};
-  const int kLocationBarVerticalPadding[] = {1, 1};
-  const int kOmniboxFontPixelSize[] = {14, 14};
-  const int kTabFaviconTitleSpacing[] = {6, 6};
-  const int kTabHeight[] = {29, 33};
-  const int kTabPinnedContentWidth[] = {23, 23};
-  const int kTabstripNewTabButtonOverlap[] = {5, 6};
-  const int kTabstripTabOverlap[] = {16, 16};
-  const int kToolbarStandardSpacing[] = {4, 8};
-  const int kToolbarElementPadding[] = {0, 8};
-  const int kToolbarLocationBarRightPadding[] = {4, 8};
-
-  const int mode = ui::MaterialDesignController::GetMode();
+  const bool hybrid = ui::MaterialDesignController::GetMode() ==
+                      ui::MaterialDesignController::MATERIAL_HYBRID;
   switch (constant) {
     case AVATAR_ICON_PADDING:
       return 4;

[Peter Kasting, 2016/12/06 04:12:24]

Usually with these constants that don't depend on MD state, I would suggest
making them a public constexpr member of a relevant class or namespace.  This
makes the conceptual dependency of other classes on the defining class a bit
clearer in code, and it makes it a bit easier to find the constant value in
codesearch (it's a single click of a cross reference).

(My eventual hope was that we could either kill this file entirely, or rename it
and its methods to make clear that we only really need it for constants whose
values change at runtime.)

The downside of this is consistency (some constants are kFooBar, some are
GetLayoutConstant(FOO_BAR)), but we already have that downside today, and I'm
not convinced making it worse is "that bad".

Are there other reasons to prefer leaving these here?

[Evan Stade, 2016/12/08 15:34:44]

not really. Moved.

-    case FIND_BAR_TOOLBAR_OVERLAP:
-      return kFindBarVerticalOffset[mode];
     case LOCATION_BAR_BORDER_THICKNESS:
-      return kLocationBarBorderThickness[mode];
+      return 1;
     case LOCATION_BAR_BUBBLE_FONT_VERTICAL_PADDING:
-      return kLocationBarBubbleFontVerticalPadding[mode];
+      return hybrid ? 4 : 2;
     case LOCATION_BAR_BUBBLE_VERTICAL_PADDING:
-      return kLocationBarBubbleVerticalPadding[mode];
+      return 3;
     case LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET:
       if (ui::MaterialDesignController::IsSecondaryUiMaterial())
         return 1;
-      return kLocationBarBubbleAnchorVerticalInset[mode];
+      return hybrid ? 8 : 6;
     case LOCATION_BAR_HEIGHT:
-      return kLocationBarHeight[mode];
+      return hybrid ? 32 : 28;
     case LOCATION_BAR_HORIZONTAL_PADDING:
-      return kLocationBarHorizontalPadding[mode];
+      return 6;
     case LOCATION_BAR_VERTICAL_PADDING:
-      return kLocationBarVerticalPadding[mode];
-    case OMNIBOX_FONT_PIXEL_SIZE:
-      return kOmniboxFontPixelSize[mode];
+      return 1;
     case TABSTRIP_NEW_TAB_BUTTON_OVERLAP:
-      return kTabstripNewTabButtonOverlap[mode];
+      return hybrid ? 6 : 5;
     case TABSTRIP_TAB_OVERLAP:
-      return kTabstripTabOverlap[mode];
-    case TAB_FAVICON_TITLE_SPACING:
-      return kTabFaviconTitleSpacing[mode];
+      return 16;
     case TAB_HEIGHT:
-      return kTabHeight[mode];
-    case TAB_PINNED_CONTENT_WIDTH:
-      return kTabPinnedContentWidth[mode];
+      return hybrid ? 33 : 29;
     case TOOLBAR_BUTTON_PADDING:
       return 6;
     case TOOLBAR_ELEMENT_PADDING:
-      return kToolbarElementPadding[mode];
-    case TOOLBAR_LOCATION_BAR_RIGHT_PADDING:
-      return kToolbarLocationBarRightPadding[mode];
+      return hybrid ? 8 : 0;
     case TOOLBAR_STANDARD_SPACING:
-      return kToolbarStandardSpacing[mode];
+      return hybrid ? 8 : 4;
   }
   NOTREACHED();
   return 0;
 }
 
 gfx::Insets GetLayoutInsets(LayoutInset inset) {
+  const bool hybrid = ui::MaterialDesignController::GetMode() ==
+                      ui::MaterialDesignController::MATERIAL_HYBRID;
   switch (inset) {
-    case TAB: {
-      const bool hybrid = ui::MaterialDesignController::GetMode() ==
-                          ui::MaterialDesignController::MATERIAL_HYBRID;
+    case TAB:
       return gfx::Insets(1, hybrid ? 18 : 16);
-    }
   }
   NOTREACHED();
   return gfx::Insets();
 }
 
 gfx::Size GetLayoutSize(LayoutSize size) {
-  const int kNewTabButtonWidth[] = {36, 39};
-  const int kNewTabButtonHeight[] = {18, 21};
-
-  const int mode = ui::MaterialDesignController::GetMode();
+  const bool hybrid = ui::MaterialDesignController::GetMode() ==
+                      ui::MaterialDesignController::MATERIAL_HYBRID;
   switch (size) {
     case NEW_TAB_BUTTON:
-      return gfx::Size(kNewTabButtonWidth[mode], kNewTabButtonHeight[mode]);
+      return gfx::Size(hybrid ? 39 : 36, hybrid ? 21 : 18);

[Peter Kasting, 2016/12/06 04:12:24]

Nit: Slightly longer, but seems easier to read:

      return hybrid ? gfx::Size(39, 21) : gfx::Size(36, 18);

[Evan Stade, 2016/12/08 15:34:44]

Done.

   }
   NOTREACHED();
   return gfx::Size();
 }