Use same base class for popup and plugin blocking UI

chrome/browser/ui/content_settings/content_setting_bubble_model.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/content_settings/content_setting_bubble_model.h"
 
 #include "base/command_line.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ skipping 18 matching lines @@
 #include "components/content_settings/core/browser/content_settings_utils.h"
 #include "components/content_settings/core/common/content_settings.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/user_metrics.h"
 #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"
 #include "ui/resources/grit/ui_resources.h"
 
 using base::UserMetricsAction;
 using content::WebContents;
 using content_settings::SettingInfo;
 using content_settings::SettingSource;
 using content_settings::SETTING_SOURCE_USER;
@@ skipping 58 matching lines @@
   SetLearnMoreLink();
 }
 
 void ContentSettingTitleAndLinkModel::SetTitle() {
   TabSpecificContentSettings* content_settings = NULL;
   if (web_contents()) {
     content_settings =
         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},
     {CONTENT_SETTINGS_TYPE_JAVASCRIPT, IDS_BLOCKED_JAVASCRIPT_TITLE},
     {CONTENT_SETTINGS_TYPE_PLUGINS, IDS_BLOCKED_PLUGINS_TITLE},
     {CONTENT_SETTINGS_TYPE_POPUPS, IDS_BLOCKED_POPUPS_TITLE},
     {CONTENT_SETTINGS_TYPE_MIXEDSCRIPT,
         IDS_BLOCKED_DISPLAYING_INSECURE_CONTENT},
     {CONTENT_SETTINGS_TYPE_PPAPI_BROKER,
         IDS_BLOCKED_PPAPI_BROKER_TITLE},
@@ skipping 326 matching lines @@
   if (!web_contents())
     return;
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_COLLECTED_COOKIES_SHOWN,
       content::Source<TabSpecificContentSettings>(
           TabSpecificContentSettings::FromWebContents(web_contents())),
       content::NotificationService::NoDetails());
   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,
                                   Profile* profile,
                                   ContentSettingsType content_type);
 
   ~ContentSettingPluginBubbleModel() override;
 
  private:
+  void SetListItems() override;
   void OnCustomLinkClicked() override;
 };
 
 ContentSettingPluginBubbleModel::ContentSettingPluginBubbleModel(
     Delegate* delegate,
     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
   // and ran all plugins.
   set_custom_link_enabled(!setting_is_managed() &&
                           web_contents &&
                           TabSpecificContentSettings::FromWebContents(
                               web_contents)->load_plugins_link_enabled());
+  SetListItems();
 }
 
 ContentSettingPluginBubbleModel::~ContentSettingPluginBubbleModel() {
   if (settings_changed()) {
     // If the user elected to allow all plugins then run plugins at this time.
     if (selected_item() == kAllowButtonIndex)
       OnCustomLinkClicked();
   }
 }
 
+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
   // settings bubble is visible.
   if (!web_contents())
     return;
 #if defined(ENABLE_PLUGINS)
   // TODO(bauerb): We should send the identifiers of blocked plugins here.
   ChromePluginServiceFilter::GetInstance()->AuthorizeAllPlugins(
       web_contents(), true, std::string());
 #endif
   set_custom_link_enabled(false);
   TabSpecificContentSettings::FromWebContents(web_contents())->
       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,
                                  Profile* profile,
                                  ContentSettingsType content_type);
   ~ContentSettingPopupBubbleModel() override {}
 
  private:
-  void SetPopups();
-  void OnPopupClicked(int index) override;
+  void SetListItems() override;
+  void OnListItemClicked(int index) override;
 };
 
 ContentSettingPopupBubbleModel::ContentSettingPopupBubbleModel(
     Delegate* delegate,
     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();
   for (std::map<int32, GURL>::const_iterator iter = blocked_popups.begin();

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

ditto nit: use range-based-for or auto

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

Done.

        iter != blocked_popups.end();
        ++iter) {
     std::string title(iter->second.spec());
     // 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.

   }
 }
 
 // The model of the content settings bubble for media settings.
 class ContentSettingMediaStreamBubbleModel
     : public ContentSettingTitleAndLinkModel {
  public:
   ContentSettingMediaStreamBubbleModel(Delegate* delegate,
                                        WebContents* web_contents,
                                        Profile* profile);
@@ skipping 726 matching lines @@
   if (type == content::NOTIFICATION_WEB_CONTENTS_DESTROYED) {
     DCHECK_EQ(web_contents_,
               content::Source<WebContents>(source).ptr());
     web_contents_ = NULL;
   } else {
     DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type);
     DCHECK_EQ(profile_, content::Source<Profile>(source).ptr());
     profile_ = NULL;
   }
 }