(Views only) Create a separate class for the contents of the Website Settings UI that is displayed …

chrome/browser/ui/views/website_settings/website_settings_popup_view.cc

Index: chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
diff --git a/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc b/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
index aed4c2192ddef24f70d9385eb53e248fac04525f..1e74d8240b8a59ac964bc64532320249be9f2ffe 100644
--- a/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
+++ b/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
@@ -134,6 +134,21 @@ class PopupHeaderView : public views::View {
   DISALLOW_COPY_AND_ASSIGN(PopupHeaderView);
 };
 
+// Website Settings are not supported for internal Chrome pages. Instead of the
+// |WebsiteSettingsPopupView|, the |InternalPageInfoPopupView| is
+// displayed.
+class InternalPageInfoPopupView : public views::BubbleDelegateView {
+ public:
+  explicit InternalPageInfoPopupView(views::View* anchor_view);
+  virtual ~InternalPageInfoPopupView();
+
+ private:
+  // views::BubbleDelegate implementations.
+  virtual gfx::Rect GetAnchorRect() OVERRIDE;
+
+  DISALLOW_COPY_AND_ASSIGN(InternalPageInfoPopupView);
+};
+
 ////////////////////////////////////////////////////////////////////////////////
 // Popup Header
 ////////////////////////////////////////////////////////////////////////////////
@@ -207,9 +222,46 @@ void PopupHeaderView::SetIdentityStatus(const string16& status,
   status_->SetEnabledColor(text_color);
 }
 
-///////////////////////////////////////////////////////////////////////////////
+////////////////////////////////////////////////////////////////////////////////
+// InternalPageInfoPopupView
+////////////////////////////////////////////////////////////////////////////////
+
+InternalPageInfoPopupView::InternalPageInfoPopupView(views::View* anchor_view)
+    : BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_LEFT) {
+  const int kSpacing = 4;
+  SetLayoutManager(new views::BoxLayout(views::BoxLayout::kHorizontal, kSpacing,
+                                        kSpacing, kSpacing));
+  views::ImageView* icon_view = new views::ImageView();
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  icon_view->SetImage(rb.GetImageSkiaNamed(IDR_PRODUCT_LOGO_26));
+  AddChildView(icon_view);
+
+  string16 text = l10n_util::GetStringUTF16(IDS_PAGE_INFO_INTERNAL_PAGE);

[tfarina, 2012/08/09 16:37:40]

could you avoid this temporary variable and fold it below? Looks like you did
created the temp var for readability ;)

[markusheintz_, 2012/08/09 21:14:44]

Done.

+  views::Label* label = new views::Label(text);
+  label->SetMultiLine(true);
+  label->SetAllowCharacterBreak(true);
+  label->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
+  AddChildView(label);
+
+  views::BubbleDelegateView::CreateBubble(this);
+  Show();
+  SizeToContents();
+}
+
+InternalPageInfoPopupView::~InternalPageInfoPopupView() {
+}
+
+gfx::Rect InternalPageInfoPopupView::GetAnchorRect() {
+  // Compensate for some built-in padding in the icon. This will make the arrow
+  // point to the middle of the icon.
+  gfx::Rect anchor(BubbleDelegateView::GetAnchorRect());
+  anchor.Inset(0, anchor_view() ? kLocationIconBottomMargin : 0);
+  return anchor;
+}
+
+////////////////////////////////////////////////////////////////////////////////
 // WebsiteSettingsPopupView
-///////////////////////////////////////////////////////////////////////////////
+////////////////////////////////////////////////////////////////////////////////
 
 WebsiteSettingsPopupView::~WebsiteSettingsPopupView() {
 }
@@ -220,7 +272,10 @@ void WebsiteSettingsPopupView::ShowPopup(views::View* anchor_view,
                                          TabContents* tab_contents,
                                          const GURL& url,
                                          const content::SSLStatus& ssl) {
-  new WebsiteSettingsPopupView(anchor_view, profile, tab_contents, url, ssl);
+  if (InternalChromePage(url))
+    new InternalPageInfoPopupView(anchor_view);
+  else
+    new WebsiteSettingsPopupView(anchor_view, profile, tab_contents, url, ssl);

[tfarina, 2012/08/09 16:37:40]

Looking now, and seeing that this inherits from BubbleDelegateView, would be
more appropriate to name it WebsiteSettingsBubbleView? For me popups are the
irritating windows that we usually block. Same thing for
InternalPageInfoPopupView (may be InternalWebsiteSettingsBubbleView or
something). 

[markusheintz_, 2012/08/09 21:14:44]

I see that inheriting from BubbleDelegetView suggest to name that
WebsiteSettings thingy bubble.

But :) :

1) Some people think bubble is confusing because of the notification bubbles and
other small bubbles that are used to notify users about ...things :). I agree
with that :)
2) We also have an ExtensionPopup (which is also a bubble delegate - if I
remember correctly ) and we call it popup in the UI  and in the Extension
documentation. 
3) I tried to find a name that everybody likes. At one point in time I just gave
up on this. I call it a popup and try to use that term consistently. :-)
4) Also renaming it now would be such a pain as there are also GTK and cocoa
implementations. :(

 }
 
 WebsiteSettingsPopupView::WebsiteSettingsPopupView(
@@ -241,47 +296,6 @@ WebsiteSettingsPopupView::WebsiteSettingsPopupView(
       cert_id_(0),
       connection_info_content_(NULL),
       page_info_content_(NULL) {
-  if (InternalChromePage(url)) {
-    views::GridLayout* layout = new views::GridLayout(this);
-    SetLayoutManager(layout);
-    views::ColumnSet* column_set = layout->AddColumnSet(0);
-    column_set->AddColumn(views::GridLayout::FILL,
-                          views::GridLayout::FILL,
-                          0,  // Resize weight.
-                          views::GridLayout::USE_PREF,
-                          0,
-                          0);
-    column_set->AddPaddingColumn(0, kIconMarginLeft);
-    column_set->AddColumn(views::GridLayout::FILL,
-                          views::GridLayout::FILL,
-                          1,
-                          views::GridLayout::USE_PREF,
-                          0,
-                          0);
-
-    layout->StartRow(1, 0);
-
-    ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-    const gfx::Image& icon = rb.GetNativeImageNamed(IDR_PRODUCT_LOGO_26);
-    views::ImageView* icon_view = new views::ImageView();
-    icon_view->SetImage(icon.ToImageSkia());
-    layout->AddView(icon_view, 1, 1, views::GridLayout::LEADING,
-                    views::GridLayout::LEADING);
-
-    string16 text = l10n_util::GetStringUTF16(
-        IDS_PAGE_INFO_INTERNAL_PAGE);
-    views::Label* label = new views::Label(text);
-    label->SetMultiLine(true);
-    label->SetAllowCharacterBreak(true);
-    label->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
-    layout->AddView(label, 1, 1, views::GridLayout::LEADING,
-                    views::GridLayout::CENTER);
-
-    views::BubbleDelegateView::CreateBubble(this);
-    Show();
-    SizeToContents();
-  } else {
-    // Non internal chrome page.
     views::GridLayout* layout = new views::GridLayout(this);
     SetLayoutManager(layout);
     const int content_column = 0;
@@ -328,14 +342,11 @@ WebsiteSettingsPopupView::WebsiteSettingsPopupView(
                                          url,
                                          ssl,
                                          content::CertStore::GetInstance()));
-  }
 }
 
 void WebsiteSettingsPopupView::OnPermissionChanged(
     PermissionSelectorView* permission_selector) {
   DCHECK(permission_selector);
-  // It's not necessary to check that the |presenter_| is not NULL since for
-  // internal chrome pages OnPermissionChanged can't be called.
   presenter_->OnSitePermissionChanged(
       permission_selector->GetPermissionType(),
       permission_selector->GetSelectedSetting());
@@ -350,8 +361,7 @@ gfx::Rect WebsiteSettingsPopupView::GetAnchorRect() {
 }
 
 void WebsiteSettingsPopupView::OnWidgetClosing(views::Widget* widget) {
-  if (presenter_.get())
-    presenter_->OnUIClosing();
+  presenter_->OnUIClosing();

[tfarina, 2012/08/09 16:37:40]

Is the comment about |presenter_| being NULL still true, in another form, will
be possible for |presenter_| to be NULL after this refactoring? If not, can you
update it?

[markusheintz_, 2012/08/09 21:14:44]

Good catch. Fixed.

 }
 
 void WebsiteSettingsPopupView::ButtonPressed(