(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 |WebsiteSettingsInternalPageView| is

[msw, 2012/08/08 17:27:08]

nit: add a comma: "|WebsiteSettingsPopupView|, the"

[markusheintz_, 2012/08/08 18:00:35]

Done.

+// displayed.
+class WebsiteSettingsInternalPageView : public views::BubbleDelegateView {
+ public:
+  explicit WebsiteSettingsInternalPageView(views::View* anchor_view);
+  virtual ~WebsiteSettingsInternalPageView();
+
+ private:
+  // views::BubbleDelegate implementations.
+  virtual gfx::Rect GetAnchorRect() OVERRIDE;
+
+  DISALLOW_COPY_AND_ASSIGN(WebsiteSettingsInternalPageView);
+};
+
 ////////////////////////////////////////////////////////////////////////////////
 // Popup Header
 ////////////////////////////////////////////////////////////////////////////////
 
 PopupHeaderView::PopupHeaderView(views::ButtonListener* close_button_listener)
   : name_(NULL), status_(NULL) {
   views::GridLayout* layout = new views::GridLayout(this);
   SetLayoutManager(layout);
 
   const int label_column = 0;
@@ skipping 53 matching lines @@
 void PopupHeaderView::SetIdentityName(const string16& name) {
   name_->SetText(name);
 }
 
 void PopupHeaderView::SetIdentityStatus(const string16& status,
                                     SkColor text_color) {
   status_->SetText(status);
   status_->SetEnabledColor(text_color);
 }
 
-///////////////////////////////////////////////////////////////////////////////
+////////////////////////////////////////////////////////////////////////////////
+// WebsiteSettingsInternalPageView

[msw, 2012/08/08 17:27:08]

nit: I don't love the name... but lack a great suggestion,
InternalPagePopupView?

[markusheintz_, 2012/08/08 18:00:35]

How about InternalPageInfoView?

[msw, 2012/08/08 18:34:10]

It's still a 'Popup' bubble, like WebsiteSettings, and both bubbles contain
'Info'; but InternalPageInfoView is an improvement, thanks!

[markusheintz_, 2012/08/08 22:10:02]

Well then let's use the descriptive name: InternalPageInfoPopupView. This name
reminds me somehow of the times when I used to code in Java :)

+////////////////////////////////////////////////////////////////////////////////
+
+WebsiteSettingsInternalPageView::WebsiteSettingsInternalPageView(
+    views::View* anchor_view)
+    : BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_LEFT) {
+  const int spacing = 4;

[msw, 2012/08/08 17:27:08]

nit: this should be kSpacing, i think. (perhaps same in your other review, did I
miss that?)

[markusheintz_, 2012/08/08 18:00:35]

Done. I'm not sure which other CL you mean. I'll skipped through some recent  
CLs and it looks like you caught all issues ;-). 

[msw, 2012/08/08 18:34:10]

found it and sent another nit :p
http://codereview.chromium.org/10823229/diff/4001/ui/views/controls/tabbed_pa...

[markusheintz_, 2012/08/08 22:10:02]

And I changed the code so that I don't need to address the nit :-D

+  SetLayoutManager(new views::BoxLayout(views::BoxLayout::kHorizontal, spacing,
+                                        spacing, spacing));
+  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());

[msw, 2012/08/08 17:27:08]

nit: consider the shorter version (very optional):
icon_view->SetImage(rb. GetImageSkiaNamed(IDR_PRODUCT_LOGO_26));

[markusheintz_, 2012/08/08 18:00:35]

Done.

+  AddChildView(icon_view);
+
+  string16 text = l10n_util::GetStringUTF16(
+      IDS_PAGE_INFO_INTERNAL_PAGE);

[msw, 2012/08/08 17:27:08]

nit: fits on line above.

[markusheintz_, 2012/08/08 18:00:35]

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();
+}
+
+WebsiteSettingsInternalPageView::~WebsiteSettingsInternalPageView() {
+}
+
+gfx::Rect WebsiteSettingsInternalPageView::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 WebsiteSettingsInternalPageView(anchor_view);
+  else
+    new WebsiteSettingsPopupView(anchor_view, profile, tab_contents, url, ssl);
 }
 
 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();
 }
 
 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);
 }