harmony: convert device chooser

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

 // Copyright 2016 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/extensions/chooser_dialog_view.h"
 
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/chooser_controller/chooser_controller.h"
 #include "chrome/browser/extensions/api/chrome_device_permissions_prompt.h"
 #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) {
   // ------------------------------------
   // | Chooser dialog title             |
   // | -------------------------------- |
   // | | option 0                     | |
   // | | option 1                     | |
   // | | option 2                     | |
   // | |                              | |
   // | |                              | |
   // | |                              | |
   // | -------------------------------- |
   // |           [ Connect ] [ Cancel ] |
   // |----------------------------------|
   // | Get help                         |
   // ------------------------------------
 
   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() {}
 
 base::string16 ChooserDialogView::GetWindowTitle() const {
   return device_chooser_content_view_->GetWindowTitle();
 }
 
 bool ChooserDialogView::ShouldShowCloseButton() const {
   return false;
@@ skipping 12 matching lines @@
   return device_chooser_content_view_->IsDialogButtonEnabled(button);
 }
 
 views::View* ChooserDialogView::CreateFootnoteView() {
   return device_chooser_content_view_->footnote_link();
 }
 
 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;
 }
 
 views::NonClientFrameView* ChooserDialogView::CreateNonClientFrameView(
     views::Widget* widget) {
   // 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() {
   device_chooser_content_view_->Accept();
   return true;
 }
 
 bool ChooserDialogView::Cancel() {
   device_chooser_content_view_->Cancel();
   return true;
@@ skipping 41 matching lines @@
 void ChromeDevicePermissionsPrompt::ShowDialogViews() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   std::unique_ptr<ChooserController> chooser_controller(
       new DevicePermissionsDialogController(web_contents()->GetMainFrame(),
                                             prompt()));
 
   constrained_window::ShowWebModalDialogViews(
       new ChooserDialogView(std::move(chooser_controller)), web_contents());
 }