Description: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run

chrome/browser/first_run/try_chrome_dialog_view.cc

 // Copyright (c) 2012 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/first_run/try_chrome_dialog_view.h"
 
 #include <shellapi.h>
 
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/strings/string16.h"
 #include "chrome/browser/process_singleton.h"
+#include "chrome/browser/ui/views/harmony/chrome_layout_provider.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/grit/theme_resources.h"
 #include "chrome/install_static/install_util.h"
 #include "chrome/installer/util/user_experiment.h"
 #include "components/strings/grit/components_strings.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_tree_host.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/image/image.h"
 #include "ui/resources/grit/ui_resources.h"
 #include "ui/views/background.h"
 #include "ui/views/controls/button/checkbox.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/controls/button/radio_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/link.h"
 #include "ui/views/controls/separator.h"
 #include "ui/views/layout/grid_layout.h"
-#include "ui/views/layout/layout_constants.h"
 #include "ui/views/layout/layout_provider.h"
 #include "ui/views/widget/widget.h"
 
 namespace {
 
 const wchar_t kHelpCenterUrl[] =
     L"https://support.google.com/chrome/answer/150752";
 
 enum ButtonTags {
   BT_NONE,
@@ skipping 49 matching lines @@
   popup_->Init(params);
 
   views::View* root_view = popup_->GetRootView();
   // The window color is a tiny bit off-white.
   root_view->set_background(
       views::Background::CreateSolidBackground(0xfc, 0xfc, 0xfc));
 
   views::GridLayout* layout = views::GridLayout::CreatePanel(root_view);
   views::ColumnSet* columns;
 
-  // First row: [icon][pad][text][pad][button].
-  columns = layout->AddColumnSet(0);
+  ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
+  const int related_spacing =
+      provider->GetDistanceMetric(views::DISTANCE_RELATED_CONTROL_HORIZONTAL);
+  const int unrelated_spacing =
+      provider->GetDistanceMetric(DISTANCE_UNRELATED_CONTROL_HORIZONTAL);
+  const int button_spacing =
+      provider->GetDistanceMetric(views::DISTANCE_RELATED_BUTTON_HORIZONTAL);
+
+      // First row: [icon][pad][text][pad][button].
+      columns = layout->AddColumnSet(0);
   columns->AddColumn(views::GridLayout::LEADING, views::GridLayout::LEADING, 0,
                      views::GridLayout::FIXED, icon_size.width(),
                      icon_size.height());
-  columns->AddPaddingColumn(0, views::kRelatedControlHorizontalSpacing);
+  columns->AddPaddingColumn(0, related_spacing);

[Peter Kasting, 2017/04/17 19:36:18]

Seems like this should be using RELATED_LABEL, given the comment above.

[ananta, 2017/04/26 02:37:29]

Done.

   columns->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
                      views::GridLayout::USE_PREF, 0, 0);
-  columns->AddPaddingColumn(0, views::kUnrelatedControlHorizontalSpacing);
+  columns->AddPaddingColumn(0, unrelated_spacing);
   columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::FILL, 1,
                      views::GridLayout::USE_PREF, 0, 0);
 
   // Optional second row: [pad][pad][radio 1].
   columns = layout->AddColumnSet(1);
   columns->AddPaddingColumn(0, icon_size.width());
-  columns->AddPaddingColumn(0, views::kRelatedControlHorizontalSpacing);
+  columns->AddPaddingColumn(0, related_spacing);
   columns->AddColumn(views::GridLayout::LEADING, views::GridLayout::FILL, 1,
                      views::GridLayout::USE_PREF, 0, 0);
 
   // Third row: [pad][pad][radio 2].
   columns = layout->AddColumnSet(2);
   columns->AddPaddingColumn(0, icon_size.width());
-  columns->AddPaddingColumn(0, views::kRelatedControlHorizontalSpacing);
+  columns->AddPaddingColumn(0, related_spacing);
   columns->AddColumn(views::GridLayout::LEADING, views::GridLayout::FILL, 1,
                      views::GridLayout::USE_PREF, 0, 0);
 
   // Fourth row: [pad][pad][button][pad][button].
   columns = layout->AddColumnSet(3);
   columns->AddPaddingColumn(0, icon_size.width());

[Peter Kasting, 2017/04/17 19:36:18]

It looks to me as if this was supposed to have another padding column in here,
and is missing it.  What does this dialog look like?

This suggests that instead of adding two padding columns, we should have
computed one "icon + padding" width above and then used it for all these padding
places below.

[ananta, 2017/04/26 02:37:29]

Thanks done.

   columns->AddColumn(views::GridLayout::LEADING, views::GridLayout::FILL, 0,
                      views::GridLayout::USE_PREF, 0, 0);
-  columns->AddPaddingColumn(0, views::LayoutProvider::Get()->GetDistanceMetric(
-                                   views::DISTANCE_RELATED_BUTTON_HORIZONTAL));
+  columns->AddPaddingColumn(0, button_spacing);
   columns->AddColumn(views::GridLayout::LEADING, views::GridLayout::FILL, 0,
                      views::GridLayout::USE_PREF, 0, 0);
 
   // Fifth row: [pad][pad][link].
   columns = layout->AddColumnSet(4);
   columns->AddPaddingColumn(0, icon_size.width());
-  columns->AddPaddingColumn(0, views::kRelatedControlHorizontalSpacing);
+  columns->AddPaddingColumn(0, related_spacing);
   columns->AddColumn(views::GridLayout::LEADING, views::GridLayout::FILL, 1,
                      views::GridLayout::USE_PREF, 0, 0);
 
   // Optional fourth row: [button].
   columns = layout->AddColumnSet(5);
   columns->AddColumn(views::GridLayout::CENTER, views::GridLayout::FILL, 1,
                      views::GridLayout::USE_PREF, 0, 0);
 
   // Optional fourth row: [divider]
   columns = layout->AddColumnSet(6);
   columns->AddColumn(views::GridLayout::CENTER, views::GridLayout::FILL, 1,
                      views::GridLayout::USE_PREF, 0, 0);
 
   // Optional fifth row [checkbox][pad][button]
   columns = layout->AddColumnSet(7);
   columns->AddColumn(views::GridLayout::LEADING, views::GridLayout::FILL, 0,
                      views::GridLayout::USE_PREF, 0, 0);
-  columns->AddPaddingColumn(0, views::kUnrelatedControlHorizontalSpacing);
+  columns->AddPaddingColumn(0, unrelated_spacing);
   columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::FILL, 1,
                      views::GridLayout::USE_PREF, 0, 0);
 
   // First row.
   layout->StartRow(0, 0);
   layout->AddView(icon);
 
   // Find out what experiment we are conducting.
   installer::ExperimentDetails experiment;
   if (!install_static::SupportsRetentionExperiments() ||
@@ skipping 49 matching lines @@
     layout->AddView(kill_chrome_);
   }
 
   views::LabelButton* accept_button =
       views::MdTextButton::CreateSecondaryUiButton(
           this, l10n_util::GetStringUTF16(IDS_OK));
   accept_button->set_tag(BT_OK_BUTTON);
 
   views::Separator* separator = NULL;
   if (experiment.flags & installer::kToastUiMakeDefault) {
     // In this flavor we have some veritical space, then a separator line

[Peter Kasting, 2017/04/17 19:36:18]

Nit: While here: correct "veritical" misspelling

[ananta, 2017/04/26 02:37:29]

Done.

     // and the 'make default' checkbox and the OK button on the same row.
-    layout->AddPaddingRow(0, views::kUnrelatedControlVerticalSpacing);
+    layout->AddPaddingRow(0, unrelated_spacing);

[Peter Kasting, 2017/04/17 19:36:18]

This is a behavior change; you went from a vertical constant to a horizontal
one, and the two didn't have the same values.  (2 places)

[ananta, 2017/04/26 02:37:29]

Done.

     layout->StartRow(0, 6);
     separator = new views::Separator();
     layout->AddView(separator);
-    layout->AddPaddingRow(0, views::kUnrelatedControlVerticalSpacing);
+    layout->AddPaddingRow(0, unrelated_spacing);
 
     layout->StartRow(0, 7);
     make_default_ = new views::Checkbox(
         l10n_util::GetStringUTF16(IDS_TRY_TOAST_SET_DEFAULT));
     make_default_->SetChecked(true);
     layout->AddView(make_default_);
     layout->AddView(accept_button);
   } else {
     // On this other flavor there is no checkbox, the OK button and possibly
     // the cancel button are in the same row.
@@ skipping 110 matching lines @@
         result_ = TRY_CHROME_AS_DEFAULT;
   }
 
   popup_->Close();
   base::MessageLoop::current()->QuitWhenIdle();
 }
 
 void TryChromeDialogView::LinkClicked(views::Link* source, int event_flags) {
   ::ShellExecuteW(NULL, L"open", kHelpCenterUrl, NULL, NULL, SW_SHOW);
 }