Use same base class for popup and plugin blocking UI

chrome/browser/ui/content_settings/content_setting_bubble_model.cc

Index: chrome/browser/ui/content_settings/content_setting_bubble_model.cc
diff --git a/chrome/browser/ui/content_settings/content_setting_bubble_model.cc b/chrome/browser/ui/content_settings/content_setting_bubble_model.cc
index 8154cb2a5df90a083944b0263fa001ccc6e4467c..5a910760edcdaed791215c0bfa9563898194fa8c 100644
--- a/chrome/browser/ui/content_settings/content_setting_bubble_model.cc
+++ b/chrome/browser/ui/content_settings/content_setting_bubble_model.cc
@@ -36,6 +36,7 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "grit/components_strings.h"
+#include "grit/theme_resources.h"
 #include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -114,11 +115,6 @@ void ContentSettingTitleAndLinkModel::SetTitle() {
         TabSpecificContentSettings::FromWebContents(web_contents());
   }
 
-  if (content_type() == CONTENT_SETTINGS_TYPE_PLUGINS && content_settings &&
-      content_settings->IsContentBlocked(content_type())) {
-    set_plugin_names(content_settings->GetBlockedPluginNames());
-  }
-
   static const ContentSettingsTypeIdEntry kBlockedTitleIDs[] = {
     {CONTENT_SETTINGS_TYPE_COOKIES, IDS_BLOCKED_COOKIES_TITLE},
     {CONTENT_SETTINGS_TYPE_IMAGES, IDS_BLOCKED_IMAGES_TITLE},
@@ -465,7 +461,29 @@ void ContentSettingCookiesBubbleModel::OnCustomLinkClicked() {
   delegate()->ShowCollectedCookiesDialog(web_contents());
 }
 
-class ContentSettingPluginBubbleModel : public ContentSettingSingleRadioGroup {
+// A bubble model that can display a list of items.
+class ContentSettingItemListModel : public ContentSettingSingleRadioGroup {
+ public:
+  ContentSettingItemListModel(Delegate* delegate,
+                              WebContents* web_contents,
+                              Profile* profile,
+                              ContentSettingsType content_type)
+      : ContentSettingSingleRadioGroup(delegate,
+                                       web_contents,
+                                       profile,
+                                       content_type) {}
+  ~ContentSettingItemListModel() override {}
+
+ protected:
+  int32 item_id_from_item_index(int index) const {
+    return bubble_content().list_items[index].item_id;
+  }
+
+ private:
+  virtual void SetListItems() = 0;

[msw, 2015/03/20 22:06:33]

Shouldn't this be protected for subclasses to override? I don't actually see a
reason for this to exist, the subclasses could just inline their individual
SetListItems behavior into their constructors and avoid calling this virtual.

[meacer, 2015/03/20 23:39:42]

Done.

+};
+
+class ContentSettingPluginBubbleModel : public ContentSettingItemListModel {

[msw, 2015/03/20 22:06:33]

nit: rename PluginBubbleModel?

[meacer, 2015/03/20 23:39:42]

Done.

[msw, 2015/03/21 00:25:19]

Jeez, I should have looked closer, maybe we shouldn't rename these, to stay
consistent with the other ContentSetting*BubbleModel classes? Really, I'd like
to see them all renamed without "ContentSetting", for brevity, since they're
local to this file, but perhaps that's too much chrun for this CL?

[msw, 2015/03/23 19:21:06]

Ping, you should revert the renaming of these two classes or plan to rename all
the others in a follow up CL. Sorry, but these really ought to be consistent.

[meacer, 2015/03/23 20:23:29]

Sorry missed this comment. I named them back to ContentSetting*BubbleModel. I'll
follow up with a CL that renames all classes here.

  public:
   ContentSettingPluginBubbleModel(Delegate* delegate,
                                   WebContents* web_contents,
@@ -475,6 +493,7 @@ class ContentSettingPluginBubbleModel : public ContentSettingSingleRadioGroup {
   ~ContentSettingPluginBubbleModel() override;
 
  private:
+  void SetListItems() override;
   void OnCustomLinkClicked() override;
 };
 
@@ -483,8 +502,10 @@ ContentSettingPluginBubbleModel::ContentSettingPluginBubbleModel(
     WebContents* web_contents,
     Profile* profile,
     ContentSettingsType content_type)

[msw, 2015/03/20 22:06:33]

nit: nix the content_type arg; inline CONTENT_SETTINGS_TYPE_PLUGINS below.

[meacer, 2015/03/20 23:39:42]

Done.

-    : ContentSettingSingleRadioGroup(
-          delegate, web_contents, profile, content_type) {
+    : ContentSettingItemListModel(delegate,
+                                  web_contents,
+                                  profile,
+                                  content_type) {
   DCHECK_EQ(content_type, CONTENT_SETTINGS_TYPE_PLUGINS);
   // Disable the "Run all plugins this time" link if the setting is managed and
   // can't be controlled by the user or if the user already clicked on the link
@@ -493,6 +514,7 @@ ContentSettingPluginBubbleModel::ContentSettingPluginBubbleModel(
                           web_contents &&
                           TabSpecificContentSettings::FromWebContents(
                               web_contents)->load_plugins_link_enabled());
+  SetListItems();
 }
 
 ContentSettingPluginBubbleModel::~ContentSettingPluginBubbleModel() {
@@ -503,6 +525,25 @@ ContentSettingPluginBubbleModel::~ContentSettingPluginBubbleModel() {
   }
 }
 
+void ContentSettingPluginBubbleModel::SetListItems() {
+  if (!web_contents())
+    return;
+  TabSpecificContentSettings* content_settings =
+      TabSpecificContentSettings::FromWebContents(web_contents());
+
+  const std::vector<base::string16>& blocked_plugins =
+      content_settings->GetBlockedPluginNames();
+  for (std::vector<base::string16>::const_iterator iter =

[msw, 2015/03/20 22:06:33]

nit: use range-based-for or auto

[meacer, 2015/03/20 23:39:42]

Done.

+           blocked_plugins.begin();
+       iter != blocked_plugins.end(); ++iter) {
+    // TODO(meacer): Link to chrome://plugins#plugin_id from here.

[msw, 2015/03/23 19:21:06]

nit: should you keep this TODO?

[meacer, 2015/03/23 20:23:29]

Removed in a previous patchset.

+    ListItem plugin_item(ui::ResourceBundle::GetSharedInstance().GetImageNamed(
+                             IDR_BLOCKED_PLUGINS),
+                         base::UTF16ToUTF8(*iter), false, 0);
+    add_list_item(plugin_item);
+  }
+}
+
 void ContentSettingPluginBubbleModel::OnCustomLinkClicked() {
   content::RecordAction(UserMetricsAction("ClickToPlay_LoadAll_Bubble"));
   // Web contents can be NULL if the tab was closed while the plugins
@@ -519,7 +560,7 @@ void ContentSettingPluginBubbleModel::OnCustomLinkClicked() {
       set_load_plugins_link_enabled(false);
 }
 
-class ContentSettingPopupBubbleModel : public ContentSettingSingleRadioGroup {
+class ContentSettingPopupBubbleModel : public ContentSettingItemListModel {

[msw, 2015/03/20 22:06:33]

nit: rename PopupBubbleModel?

[meacer, 2015/03/20 23:39:42]

Done.

  public:
   ContentSettingPopupBubbleModel(Delegate* delegate,
                                  WebContents* web_contents,
@@ -528,8 +569,8 @@ class ContentSettingPopupBubbleModel : public ContentSettingSingleRadioGroup {
   ~ContentSettingPopupBubbleModel() override {}
 
  private:
-  void SetPopups();
-  void OnPopupClicked(int index) override;
+  void SetListItems() override;
+  void OnListItemClicked(int index) override;
 };
 
 ContentSettingPopupBubbleModel::ContentSettingPopupBubbleModel(
@@ -537,13 +578,14 @@ ContentSettingPopupBubbleModel::ContentSettingPopupBubbleModel(
     WebContents* web_contents,
     Profile* profile,
     ContentSettingsType content_type)

[msw, 2015/03/20 22:06:33]

nit: nix the content_type arg; inline CONTENT_SETTINGS_TYPE_POPUPS below.

[meacer, 2015/03/20 23:39:42]

Done, removed from some more classes where applicable.

-    : ContentSettingSingleRadioGroup(
-        delegate, web_contents, profile, content_type) {
-  SetPopups();
+    : ContentSettingItemListModel(delegate,
+                                  web_contents,
+                                  profile,
+                                  content_type) {
+  SetListItems();
 }
 
-
-void ContentSettingPopupBubbleModel::SetPopups() {
+void ContentSettingPopupBubbleModel::SetListItems() {
   std::map<int32, GURL> blocked_popups =
       PopupBlockerTabHelper::FromWebContents(web_contents())
           ->GetBlockedPopupRequests();
@@ -554,19 +596,17 @@ void ContentSettingPopupBubbleModel::SetPopups() {
     // The popup may not have a valid URL.
     if (title.empty())
       title = l10n_util::GetStringUTF8(IDS_TAB_LOADING_TITLE);
-    PopupItem popup_item(
-        ui::ResourceBundle::GetSharedInstance().GetImageNamed(
-            IDR_DEFAULT_FAVICON),
-        title,
-        iter->first);
-    add_popup(popup_item);
+    ListItem popup_item(ui::ResourceBundle::GetSharedInstance().GetImageNamed(
+                            IDR_DEFAULT_FAVICON),
+                        title, true, iter->first);
+    add_list_item(popup_item);
   }
 }
 
-void ContentSettingPopupBubbleModel::OnPopupClicked(int index) {
+void ContentSettingPopupBubbleModel::OnListItemClicked(int index) {
   if (web_contents()) {
-    PopupBlockerTabHelper::FromWebContents(web_contents())->
-        ShowBlockedPopup(bubble_content().popup_items[index].popup_id);
+    PopupBlockerTabHelper::FromWebContents(web_contents())
+        ->ShowBlockedPopup(item_id_from_item_index(index));

[msw, 2015/03/20 22:06:33]

This function, item_id_from_item_index(), is only needed for popups, why not
just make that a ContentSettingPopupBubbleModel function and remove the new
shared base class? I don't see the point of adding ContentSettingItemListModel.

[meacer, 2015/03/20 23:39:42]

Removed ContentSettingItemListModel.

   }
 }