Add the names of plugins to the blocked plugin bubble.

Add the names of plugins to the blocked plugin bubble.

BUG=414399
TEST=browser_tests --gtest_filter=ChromeRenderViewTest.*
TEST=unit_tests --gtest_filter=ContentSettingBubbleModelTest.Plugins

Committed: https://crrev.com/303f76bcace7e8c827e21c76ed435b05beeffb69
Cr-Commit-Position: refs/heads/master@{#307570}

[Will Harris, 2014-12-04 00:51:37]

wfh@chromium.org changed reviewers:
+ jochen@chromium.org

[Will Harris, 2014-12-04 00:53:40]

I've put an example of the UI in the bug.

[Lei Zhang, 2014-12-05 22:18:04]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2014-12-05 22:18:06]

Generally look ok to me, but someone from
chrome/browser/ui/content_settings/OWNERS probably should take a look too.

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (left):

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:701:
IPC_MESSAGE_HANDLER(ChromeViewHostMsg_ContentBlocked, OnContentBlocked)
Is OnContentBlocked() still used?

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:241: const
base::string16 kComma = base::string16(base::ASCIIToUTF16(", "));
You don't need to call base::string16().

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:244:
base::hash_set<base::string16>::const_iterator it =
You can probably just make this a C++11 Range-Based For Loops.

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:248: it++;
++it;

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.h:189: const
base::string16 blocked_plugin_names() const;
Non-trivial methods should be NamedLikeThis().

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/ui/conten...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/ui/conten...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:112:
TabSpecificContentSettings::FromWebContents(web_contents())
If you save the return value, you can then call its methods on just 1 line.

https://codereview.chromium.org/777103002/diff/80001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/777103002/diff/80001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:201: void
ContentSettingsObserver::DidBlockContentType(
This method can just call the one below.

[Will Harris, 2014-12-06 01:46:33]

wfh@chromium.org changed reviewers:
+ bauerb@chromium.org

[Will Harris, 2014-12-08 00:21:12]

PTAL

+bauerb for chrome/browser/content_settings

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (left):

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:701:
IPC_MESSAGE_HANDLER(ChromeViewHostMsg_ContentBlocked, OnContentBlocked)
On 2014/12/05 22:18:05, Lei Zhang wrote:
> Is OnContentBlocked() still used?

used by the tests

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:241: const
base::string16 kComma = base::string16(base::ASCIIToUTF16(", "));
On 2014/12/05 22:18:05, Lei Zhang wrote:
> You don't need to call base::string16().

Done.

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:244:
base::hash_set<base::string16>::const_iterator it =
On 2014/12/05 22:18:05, Lei Zhang wrote:
> You can probably just make this a C++11 Range-Based For Loops.

left using iterator as it's the fastest way to check I'm on the final element
and not add the comma.

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:248: it++;
On 2014/12/05 22:18:05, Lei Zhang wrote:
> ++it;

Done.

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.h:189: const
base::string16 blocked_plugin_names() const;
On 2014/12/05 22:18:05, Lei Zhang wrote:
> Non-trivial methods should be NamedLikeThis().

Done.

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/ui/conten...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/ui/conten...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:112:
TabSpecificContentSettings::FromWebContents(web_contents())
On 2014/12/05 22:18:05, Lei Zhang wrote:
> If you save the return value, you can then call its methods on just 1 line.

Ack. restructured this and also checked for non NULL web_contents()

https://codereview.chromium.org/777103002/diff/80001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/777103002/diff/80001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:201: void
ContentSettingsObserver::DidBlockContentType(
On 2014/12/05 22:18:05, Lei Zhang wrote:
> This method can just call the one below.

Done.

[Bernhard Bauer, 2014-12-08 09:21:45]

https://codereview.chromium.org/777103002/diff/120001/chrome/browser/content_...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/777103002/diff/120001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:243:
base::string16 names;
There is JoinString (in base/strings/string_util.h) which does this.

https://codereview.chromium.org/777103002/diff/120001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:345:
blocked_plugin_names_.insert(details);
Leave out the braces for single-line statements.

https://codereview.chromium.org/777103002/diff/120001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:345:
blocked_plugin_names_.insert(details);
Is this going to insert an empty string into the set?

https://codereview.chromium.org/777103002/diff/120001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/777103002/diff/120001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:121: if
(!plugin_names.empty()) {
If |plugin_names| is indeed empty, you will call set_plugin_names() with an
empty string, which is fine, so you can just do that unconditionally.

https://codereview.chromium.org/777103002/diff/120001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/777103002/diff/120001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:209: if
(!content_blocked_[settings_type]) {
This check means you are only going to send this message for the first plugin
that is blocked. That's probably not what you want?

[Lei Zhang, 2014-12-08 20:08:19]

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (left):

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:701:
IPC_MESSAGE_HANDLER(ChromeViewHostMsg_ContentBlocked, OnContentBlocked)
On 2014/12/08 00:21:11, Will Harris wrote:
> On 2014/12/05 22:18:05, Lei Zhang wrote:
> > Is OnContentBlocked() still used?
> 
> used by the tests

If it's the case that only tests call it, it's probably time to change the tests
to use OnContentBlockedWithDetail() and delete OnContentBlocked().

[Will Harris, 2014-12-08 21:11:47]

PTAL

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (left):

https://codereview.chromium.org/777103002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:701:
IPC_MESSAGE_HANDLER(ChromeViewHostMsg_ContentBlocked, OnContentBlocked)
On 2014/12/08 20:08:19, Lei Zhang wrote:
> On 2014/12/08 00:21:11, Will Harris wrote:
> > On 2014/12/05 22:18:05, Lei Zhang wrote:
> > > Is OnContentBlocked() still used?
> > 
> > used by the tests
> 
> If it's the case that only tests call it, it's probably time to change the
tests
> to use OnContentBlockedWithDetail() and delete OnContentBlocked().

Actually, changing the default function signature of OnContentBlocked() ends up
making almost all calls to it pass extra empty string parameter, so the CL turns
out to be more complex, so I'll leave OnContentBlocked() as a convenience
method.

https://codereview.chromium.org/777103002/diff/120001/chrome/browser/content_...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/777103002/diff/120001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:243:
base::string16 names;
On 2014/12/08 09:21:44, Bernhard Bauer wrote:
> There is JoinString (in base/strings/string_util.h) which does this.

Done.

https://codereview.chromium.org/777103002/diff/120001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:345:
blocked_plugin_names_.insert(details);
On 2014/12/08 09:21:44, Bernhard Bauer wrote:
> Leave out the braces for single-line statements.

Done.

https://codereview.chromium.org/777103002/diff/120001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/777103002/diff/120001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:121: if
(!plugin_names.empty()) {
On 2014/12/08 09:21:44, Bernhard Bauer wrote:
> If |plugin_names| is indeed empty, you will call set_plugin_names() with an
> empty string, which is fine, so you can just do that unconditionally.

Done.

https://codereview.chromium.org/777103002/diff/120001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/777103002/diff/120001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:209: if
(!content_blocked_[settings_type]) {
On 2014/12/08 09:21:44, Bernhard Bauer wrote:
> This check means you are only going to send this message for the first plugin
> that is blocked. That's probably not what you want?

send this IPC multiple times if details is not empty string. 
TabSpecificContentSettings already handles this case gracefully.

[Bernhard Bauer, 2014-12-08 21:47:14]

LGTM, just one suggestion:

https://codereview.chromium.org/777103002/diff/160001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/777103002/diff/160001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:880:
observer->DidBlockContentType(content_type, plugin.name);
Could you pass |group_name| here and below instead? That one is slightly more
user-friendly (e.g. "Java" instead of "Oracle Java(tm) Runtime Plugin For
Browsers").

[Will Harris, 2014-12-09 07:27:09]

thestig PTAL

https://codereview.chromium.org/777103002/diff/160001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/777103002/diff/160001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:880:
observer->DidBlockContentType(content_type, plugin.name);
On 2014/12/08 21:47:14, Bernhard Bauer wrote:
> Could you pass |group_name| here and below instead? That one is slightly more
> user-friendly (e.g. "Java" instead of "Oracle Java(tm) Runtime Plugin For
> Browsers").

Done.

[Lei Zhang, 2014-12-09 20:42:06]

lgtm

[Will Harris, 2014-12-09 21:20:49]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2014-12-09 21:22:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777103002/180001

[commit-bot: I haz the power, 2014-12-09 22:53:15]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-12-09 22:54:57]

Patchset 10 (id:??) landed as
https://crrev.com/303f76bcace7e8c827e21c76ed435b05beeffb69
Cr-Commit-Position: refs/heads/master@{#307570}