(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

 // 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 "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/certificate_viewer.h"
 #include "chrome/browser/profiles/profile.h"
@@ skipping 116 matching lines @@
 
  private:
   // The label that displays the name of the site's identity.
   views::Label* name_;
   // The label that displays the status of the identity check for this site.
   views::Label* status_;
 
   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
 ////////////////////////////////////////////////////////////////////////////////
 
 PopupHeaderView::PopupHeaderView(views::ButtonListener* close_button_listener)
   : name_(NULL), status_(NULL) {

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

indent 4 spaces

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

Done.

   views::GridLayout* layout = new views::GridLayout(this);
   SetLayoutManager(layout);
 
   const int label_column = 0;
   views::ColumnSet* column_set = layout->AddColumnSet(label_column);
   column_set->AddPaddingColumn(0, kHeaderPaddingLeft);
   column_set->AddColumn(views::GridLayout::FILL,
                         views::GridLayout::FILL,
                         1,
                         views::GridLayout::USE_PREF,
@@ skipping 42 matching lines @@
 }
 
 PopupHeaderView::~PopupHeaderView() {
 }
 
 void PopupHeaderView::SetIdentityName(const string16& name) {
   name_->SetText(name);
 }
 
 void PopupHeaderView::SetIdentityStatus(const string16& status,
                                     SkColor text_color) {

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

wrong indentation here

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

Done.

   status_->SetText(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() {
 }
 
 // static
 void WebsiteSettingsPopupView::ShowPopup(views::View* anchor_view,
                                          Profile* profile,
                                          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(
     views::View* anchor_view,
     Profile* profile,
     TabContents* tab_contents,
     const GURL& url,
     const content::SSLStatus& ssl)
     : BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_LEFT),
       tab_contents_(tab_contents),
       header_(NULL),
       tabbed_pane_(NULL),
       site_data_content_(NULL),
       cookie_dialog_link_(NULL),
       permissions_content_(NULL),
       identity_info_content_(NULL),
       certificate_dialog_link_(NULL),
       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;
     views::ColumnSet* column_set = layout->AddColumnSet(content_column);
     column_set->AddColumn(views::GridLayout::FILL,
                           views::GridLayout::FILL,
                           1,
                           views::GridLayout::USE_PREF,
                           0,
                           0);
@@ skipping 26 matching lines @@
     views::BubbleDelegateView::CreateBubble(this);
     this->Show();
     SizeToContents();
 
     presenter_.reset(new WebsiteSettings(this, profile,
                                          tab_contents->content_settings(),
                                          tab_contents->infobar_tab_helper(),
                                          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());
 }
 
 gfx::Rect WebsiteSettingsPopupView::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;
 }
 
 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(
     views::Button* button,
     const views::Event& event) {
   GetWidget()->Close();
 }
 
 void WebsiteSettingsPopupView::LinkClicked(views::Link* source,
                                            int event_flags) {
@@ skipping 333 matching lines @@
 
   if (link) {
     content_layout->StartRow(1, 0);
     content_layout->AddView(link);
   }
 
   layout->AddView(content_pane, 1, 1, views::GridLayout::LEADING,
                   views::GridLayout::LEADING);
   layout->AddPaddingRow(0, kConnectionSectionPaddingBottom);
 }