harmony: convert device chooser

chrome/browser/ui/views/extensions/chooser_dialog_view.cc

Index: chrome/browser/ui/views/extensions/chooser_dialog_view.cc
diff --git a/chrome/browser/ui/views/extensions/chooser_dialog_view.cc b/chrome/browser/ui/views/extensions/chooser_dialog_view.cc
index dffa13c118584bacee809b05478cd2443aae44e4..27017bc7303d284d2d0b623bab05fbe590cd590d 100644
--- a/chrome/browser/ui/views/extensions/chooser_dialog_view.cc
+++ b/chrome/browser/ui/views/extensions/chooser_dialog_view.cc
@@ -11,15 +11,32 @@
 #include "chrome/browser/extensions/chrome_extension_chooser_dialog.h"
 #include "chrome/browser/extensions/device_permissions_dialog_controller.h"
 #include "chrome/browser/ui/views/device_chooser_content_view.h"
+#include "chrome/browser/ui/views/harmony/layout_delegate.h"
 #include "components/constrained_window/constrained_window_views.h"
 #include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "ui/gfx/geometry/insets.h"
+#include "ui/views/background.h"
 #include "ui/views/controls/link.h"
 #include "ui/views/controls/styled_label.h"
-#include "ui/views/layout/layout_constants.h"
+#include "ui/views/layout/fill_layout.h"
 #include "ui/views/window/dialog_client_view.h"
 
+namespace {
+
+LayoutDelegate::LayoutDistanceType GetContentViewToButtonsDistanceType() {
+  // Hack: in the old layout, the buttons are related to the content view, but
+  // in the Harmony layout they are unrelated, so the layout distance type is
+  // variable.

[Peter Kasting, 2017/02/01 02:39:25]

Unless the function of the buttons have changed, they're either related or
they're not -- they're not related in pre-Harmony but unrelated in Harmony.

It seems like the issue is that we made rules about the spacing for different
relationships, but then we positioned things in ways that violate those rules. 
We shouldn't hack around this by choosing different semantic meanings to get the
result we want.

I don't have a strong opinion about what "related" should mean, but we should
decide what it means, and then apply that meaning here.  If that means changing
to "related" but we don't like how that works in Harmony, then we need to either
change the value of RELATED_CONTROL_VERTICAL_SPACING, or we need to have
constants that reflect the semantic distinction between the types of
relationships we want to have different positions.

(This is also true of the other place already in the codebase that does a
similar hack.)

+  return LayoutDelegate::Get()->IsHarmonyMode()
+             ? LayoutDelegate::LayoutDistanceType::
+                   UNRELATED_CONTROL_VERTICAL_SPACING
+             : LayoutDelegate::LayoutDistanceType::
+                   RELATED_CONTROL_VERTICAL_SPACING;
+}
+
+}  // namespace
+
 ChooserDialogView::ChooserDialogView(
     std::unique_ptr<ChooserController> chooser_controller) {
   // ------------------------------------
@@ -40,6 +57,19 @@ ChooserDialogView::ChooserDialogView(
   DCHECK(chooser_controller);
   device_chooser_content_view_ =
       new DeviceChooserContentView(this, std::move(chooser_controller));
+
+  LayoutDelegate* delegate = LayoutDelegate::Get();
+  if (delegate->IsHarmonyMode()) {

[Peter Kasting, 2017/02/01 02:39:26]

I'd like to avoid IsHarmonyMode() here.

The first question is whether the size of this dialog is properly determined
here or in DeviceChooserContentView.  I suspect the answer is "here", so the
existing default size in that class should be moved over to here.  Along the
way, the code should probably be changed so that content view has no preferred
size; rather, it is laid out as taking the full width (minus insets) of the
parent dialog, and the dialog itself has some kind of desired size.

Then we can just compute the width as "the width the layout delegate says to
use, if nonzero; otherwise, the old hardcoded constant".

+    gfx::Size size = device_chooser_content_view_->GetPreferredSize();
+    int inset_width =
+        2 *
+        delegate->GetLayoutDistance(
+            LayoutDelegate::LayoutDistanceType::PANEL_HORIZ_MARGIN);
+    size.set_width(delegate->GetDialogPreferredWidth(
+                       LayoutDelegate::DialogWidthType::MEDIUM) -
+                   2 * inset_width);

[Peter Kasting, 2017/02/01 02:39:26]

How come this is 2 * inset_width instead of just inset_width?

+    device_chooser_content_view_->SetPreferredSize(size);
+  }
 }
 
 ChooserDialogView::~ChooserDialogView() {}
@@ -72,7 +102,10 @@ views::View* ChooserDialogView::CreateFootnoteView() {
 views::ClientView* ChooserDialogView::CreateClientView(views::Widget* widget) {
   views::DialogClientView* client =
       new views::DialogClientView(widget, GetContentsView());
-  client->set_button_row_insets(gfx::Insets());
+  LayoutDelegate* delegate = LayoutDelegate::Get();
+  client->set_button_row_insets(gfx::Insets(
+      delegate->GetLayoutDistance(GetContentViewToButtonsDistanceType()), 0, 0,
+      0));
   return client;
 }
 
@@ -81,9 +114,13 @@ views::NonClientFrameView* ChooserDialogView::CreateNonClientFrameView(
   // ChooserDialogView always has a parent, so ShouldUseCustomFrame() should
   // always be true.
   DCHECK(ShouldUseCustomFrame());
+  LayoutDelegate* delegate = LayoutDelegate::Get();
   return views::DialogDelegate::CreateDialogFrameView(
-      widget, gfx::Insets(views::kPanelVertMargin, views::kPanelHorizMargin,
-                          views::kPanelVertMargin, views::kPanelHorizMargin));
+      widget,
+      gfx::Insets(delegate->GetLayoutDistance(
+                      LayoutDelegate::LayoutDistanceType::PANEL_VERT_MARGIN),
+                  delegate->GetLayoutDistance(
+                      LayoutDelegate::LayoutDistanceType::PANEL_HORIZ_MARGIN)));
 }
 
 bool ChooserDialogView::Accept() {