Adjust spacing in InternalPageInfoPopupView

chrome/browser/ui/views/website_settings/website_settings_popup_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/ui/views/website_settings/website_settings_popup_view.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <vector>
@@ skipping 155 matching lines @@
 
   // views::BubbleDialogDelegateView:
   views::NonClientFrameView* CreateNonClientFrameView(
       views::Widget* widget) override;
   void OnWidgetDestroying(views::Widget* widget) override;
   int GetDialogButtons() const override;
 
  private:
   friend class WebsiteSettingsPopupView;
 
+  // Used around icon and inside bubble border.
+  const int kSpacing = 12;

[Peter Kasting, 2017/01/06 02:27:11]

Nit: constexpr

[Evan Stade, 2017/01/06 20:20:20]

Done.

+
   DISALLOW_COPY_AND_ASSIGN(InternalPageInfoPopupView);
 };
 
 ////////////////////////////////////////////////////////////////////////////////
 // Popup Header
 ////////////////////////////////////////////////////////////////////////////////
 
 PopupHeaderView::PopupHeaderView(
     views::ButtonListener* button_listener,
     views::StyledLabelListener* styled_label_listener)
@@ skipping 161 matching lines @@
     icon = IDR_PRODUCT_LOGO_16;
   } else if (!url.SchemeIs(content::kChromeUIScheme) &&
              !url.SchemeIs(content::kChromeDevToolsScheme)) {
     NOTREACHED();
   }
 
   // Compensate for built-in vertical padding in the anchor view's image.
   set_anchor_view_insets(gfx::Insets(
       GetLayoutConstant(LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET), 0));
 
-  const int kSpacing = 16;
   SetLayoutManager(new views::BoxLayout(views::BoxLayout::kHorizontal, kSpacing,
                                         kSpacing, kSpacing));
   set_margins(gfx::Insets());
   views::ImageView* icon_view = new NonAccessibleImageView();
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   icon_view->SetImage(rb.GetImageSkiaNamed(icon));
   AddChildView(icon_view);
 
   views::Label* label = new views::Label(l10n_util::GetStringUTF16(text));
   label->SetMultiLine(true);
   label->SetAllowCharacterBreak(true);
   label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   AddChildView(label);
 
   views::BubbleDialogDelegateView::CreateBubble(this);
 }
 
 InternalPageInfoPopupView::~InternalPageInfoPopupView() {
 }
 
 views::NonClientFrameView* InternalPageInfoPopupView::CreateNonClientFrameView(
     views::Widget* widget) {
   views::BubbleFrameView* frame = static_cast<views::BubbleFrameView*>(
       BubbleDialogDelegateView::CreateNonClientFrameView(widget));
-  // 16px padding + half of icon width comes out to 24px.
+  // Padding around icon + half of icon width.
   frame->bubble_border()->set_arrow_offset(
-      24 + frame->bubble_border()->GetBorderThickness());
+      kSpacing + 8 + frame->bubble_border()->GetBorderThickness());

[Peter Kasting, 2017/01/06 02:27:11]

Is it possible to compute this without hardcoding "8"?  For example, keep a
pointer to the icon view around and use it to get the width/2.  This would also
let you get the x coordinate from the icon so you wouldn't have to ensure the
spacing amount/calculation stays in sync here, which would mean you wouldn't
have to hoist the spacing constant.

(You could do this without an icon pointer by just assuming "first child is the
icon", but that seems fragile.)

This would obviate the need for the comment above.

[Evan Stade, 2017/01/06 20:20:20]

I don't think the contents will have been laid out by this point, so the spacing
constant would still need to be here. We could still get rid of the hardcoded 8
but it's not at zero cost, and it doesn't seem to me super likely that will ever
save us from a bug.

   return frame;
 }
 
 void InternalPageInfoPopupView::OnWidgetDestroying(views::Widget* widget) {
   g_shown_popup_type = WebsiteSettingsPopupView::POPUP_NONE;
 }
 
 int InternalPageInfoPopupView::GetDialogButtons() const {
   return ui::DIALOG_BUTTON_NONE;
 }
@@ skipping 384 matching lines @@
           WebsiteSettings::WEBSITE_SETTINGS_CONNECTION_HELP_OPENED);
       break;
     case STYLED_LABEL_RESET_CERTIFICATE_DECISIONS:
       presenter_->OnRevokeSSLErrorBypassButtonPressed();
       GetWidget()->Close();
       break;
     default:
       NOTREACHED();
   }
 }