Stop using layout provider for menu horizontal spaces

Menu horizontal spaces were consolidated into a single
item_horizontal_padding by crrev.com/c/1089431, which also made the
value come from the layout provider.

However, when Harmony is enabled, the horizontal padding becomes too
large (8 instead of 16). As a side effect, menus became too spacey on
Harmony (see details on the linked bug).

One alternative was to use DISTANCE_RELATED_CONTROL_HORIZONTAL_SMALL and
define the corresponding padding as 8 for Harmony, but that would
introduce a dependency from ui/views/controls to chrome/browser/ui/views,
which I don't think is desirable. In addition, while Harmony is rolling
out, making sure we don't break paddings on other browser components can
be challenging, so I added a TODO to make a general consolidation once
Harmony becomes the default.

Bug: 851843
Change-Id: Ifbecf12f434e20af8f93b590feed7eb4b2bc33de
Reviewed-on: https://chromium-review.googlesource.com/1108451Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Fabio Tirelo <ftirelo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569050}

chrome/browser/ui/app_list/app_context_menu_unittest.cc

@@ -34,7 +34,6 @@
 #include "components/keyed_service/core/keyed_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/ui_base_features.h"
-#include "ui/views/layout/layout_provider.h"
 
 namespace {
 
@@ -355,11 +354,6 @@ class AppContextMenuTest : public AppListTestBase,
   std::unique_ptr<FakeAppListModelUpdater> model_updater_;
   base::test::ScopedFeatureList scoped_feature_list_;
 
-  // The layout provider is meant to be a singleton, but it is not initialized
-  // for unit tests. Constructing one here makes it globally available, which
-  // is later used by the menu item during initialization.
-  views::LayoutProvider layout_provider_;
-
   DISALLOW_COPY_AND_ASSIGN(AppContextMenuTest);
 };
 

ui/views/controls/menu/menu_config.cc

@@ -8,7 +8,6 @@
 #include "ui/views/controls/menu/menu_controller.h"
 #include "ui/views/controls/menu/menu_image_util.h"
 #include "ui/views/controls/menu/menu_item_view.h"
-#include "ui/views/layout/layout_provider.h"
 #include "ui/views/round_rect_painter.h"
 
 namespace views {
@@ -25,11 +24,10 @@ MenuConfig::MenuConfig()
       minimum_text_item_height(0),
       minimum_container_item_height(0),
       minimum_menu_width(0),
-      item_horizontal_padding(LayoutProvider::Get()->GetDistanceMetric(
-          DistanceMetric::DISTANCE_RELATED_CONTROL_HORIZONTAL)),
-      touchable_item_horizontal_padding(
-          LayoutProvider::Get()->GetDistanceMetric(
-              DistanceMetric::DISTANCE_TOUCHABLE_RELATED_CONTROL_HORIZONTAL)),
+      // TODO(ftirelo): Paddings should come from the layout provider, once
+      //                Harmony is the default behavior.
+      item_horizontal_padding(8),
+      touchable_item_horizontal_padding(16),
       label_to_arrow_padding(8),
       arrow_to_edge_padding(5),
       touchable_icon_size(20),

ui/views/controls/menu/menu_item_view_unittest.cc

@@ -11,7 +11,6 @@
 #include "ui/compositor/canvas_painter.h"
 #include "ui/strings/grit/ui_strings.h"
 #include "ui/views/controls/menu/submenu_view.h"
-#include "ui/views/layout/layout_provider.h"
 #include "ui/views/test/menu_test_utils.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/vector_icons.h"
@@ -48,21 +47,7 @@ class TestMenuItemView : public MenuItemView {
   DISALLOW_COPY_AND_ASSIGN(TestMenuItemView);
 };
 
-class MenuItemViewUnitTest : public ::testing::Test {
- public:
-  MenuItemViewUnitTest() = default;
-  ~MenuItemViewUnitTest() override = default;
-
- private:
-  // The layout provider is meant to be a singleton, but it is not initialized
-  // for unit tests. Constructing one here makes it globally available, which
-  // is later used by the menu item during initialization.
-  LayoutProvider layout_provider_;
-
-  DISALLOW_COPY_AND_ASSIGN(MenuItemViewUnitTest);
-};
-
-TEST_F(MenuItemViewUnitTest, TestMenuItemViewWithFlexibleWidthChild) {
+TEST(MenuItemViewUnitTest, TestMenuItemViewWithFlexibleWidthChild) {
   TestMenuItemView root_menu;
   root_menu.set_owned_by_client();
 
@@ -101,7 +86,7 @@ TEST_F(MenuItemViewUnitTest, TestMenuItemViewWithFlexibleWidthChild) {
 
 // Tests that the top-level menu item with hidden children should contain the
 // "(empty)" menu item to display.
-TEST_F(MenuItemViewUnitTest, TestEmptyTopLevelWhenAllItemsAreHidden) {
+TEST(MenuItemViewUnitTest, TestEmptyTopLevelWhenAllItemsAreHidden) {
   TestMenuItemView root_menu;
   views::MenuItemView* item1 =
       root_menu.AppendMenuItemWithLabel(1, base::ASCIIToUTF16("item 1"));
@@ -132,7 +117,7 @@ TEST_F(MenuItemViewUnitTest, TestEmptyTopLevelWhenAllItemsAreHidden) {
 
 // Tests that submenu with hidden children should contain the "(empty)" menu
 // item to display.
-TEST_F(MenuItemViewUnitTest, TestEmptySubmenuWhenAllChildItemsAreHidden) {
+TEST(MenuItemViewUnitTest, TestEmptySubmenuWhenAllChildItemsAreHidden) {
   TestMenuItemView root_menu;
   MenuItemView* submenu_item =
       root_menu.AppendSubMenu(1, base::ASCIIToUTF16("My Submenu"));
@@ -163,7 +148,7 @@ TEST_F(MenuItemViewUnitTest, TestEmptySubmenuWhenAllChildItemsAreHidden) {
             empty_item->title());
 }
 
-TEST_F(MenuItemViewUnitTest, UseMnemonicOnPlatform) {
+TEST(MenuItemViewUnitTest, UseMnemonicOnPlatform) {
   TestMenuItemView root_menu;
   views::MenuItemView* item1 =
       root_menu.AppendMenuItemWithLabel(1, base::ASCIIToUTF16("&Item 1"));

ui/views/layout/layout_provider.cc

@@ -94,8 +94,6 @@ int LayoutProvider::GetDistanceMetric(int metric) const {
       return 6;
     case DistanceMetric::DISTANCE_RELATED_CONTROL_HORIZONTAL:
       return 8;
-    case DistanceMetric::DISTANCE_TOUCHABLE_RELATED_CONTROL_HORIZONTAL:
-      return 16;
     case DistanceMetric::DISTANCE_RELATED_CONTROL_VERTICAL:
       return 8;
     case DistanceMetric::DISTANCE_RELATED_LABEL_HORIZONTAL:

ui/views/layout/layout_provider.h

@@ -85,9 +85,6 @@ enum DistanceMetric {
   DISTANCE_RELATED_BUTTON_HORIZONTAL,
   // Horizontal spacing between controls that are logically related.
   DISTANCE_RELATED_CONTROL_HORIZONTAL,
-  // Horizontal spacing between controls in a touchable context that are
-  // logically related.
-  DISTANCE_TOUCHABLE_RELATED_CONTROL_HORIZONTAL,
   // The spacing between a pair of related vertical controls, used for
   // dialog layout.
   DISTANCE_RELATED_CONTROL_VERTICAL,