Put some plug-ins behind an infobar, where they have:...

Put some plug-ins behind an infobar, where they have:
- Been targeted by mass malware.
- Do not yet have a good sandboxing story.

BUG=60458
TEST=http://java.sun.com/products/plugin/1.4/demos/applets/Blink/example1.html with default plug-in settings.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72766

[Chris Evans, 2011-01-24 11:19:57]

As discussed at length recently, we're going to move forward with plug-in
security.
We decided to basically give an infobar-type approach a whirl in the dev channel
and get feedback. I've coded this up now so that it can get a workout on canary
/ dev channel as soon after the M10 branch as possible. Therefore this targets
M11 at the earliest.

[Bernhard Bauer, 2011-01-24 12:56:33]

http://codereview.chromium.org/6350010/diff/1/chrome/browser/tab_contents/tab...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6350010/diff/1/chrome/browser/tab_contents/tab...
chrome/browser/tab_contents/tab_contents.cc:380:
UserMetrics::RecordAction(UserMetricsAction(uma.c_str()));
The problem with this is if you don't use a string literal for the
UserMetricsAction, the script won't pick it up.

Also, it's probably going to be a bit harder to find out aggregated numbers for
the infobar (how often is it shown in total, etc).

http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc#...
chrome/renderer/render_view.cc:2708:
!cmd->HasSwitch(switches::kAllowOutdatedPlugins)) {
What is the interaction with blocking outdated plugins? If someone uses an old
version of Quicktime, will they get an infobar telling them to upgrade, and
after they have upgraded, this one? Right now, I'm trying to use the outdated
plugin infobar to incentivize people to upgrade their plugins ("if you want to
get rid of this infobar, upgrade your plugin"), and if you're always going to
get the infobar, that incentive goes away.

http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc#...
chrome/renderer/render_view.cc:2729: host_setting == CONTENT_SETTING_DEFAULT) {
Combined with the changes in HostContentSettingsMap, what are the precedences
now? IIUC, in order for someone to get the infobar, they need to have a default
plugin setting of "Allow" and no exceptions (of any sort) for the current
website, correct?

http://codereview.chromium.org/6350010/diff/1/webkit/plugins/npapi/plugin_gro...
File webkit/plugins/npapi/plugin_group.cc (right):

http://codereview.chromium.org/6350010/diff/1/webkit/plugins/npapi/plugin_gro...
webkit/plugins/npapi/plugin_group.cc:381: return group_name_ ==
ASCIIToUTF16(kJavaGroupName) ||
I think I'd prefer to reuse the rest of the PluginGroup infrastructure here,
i.e. add an additional bit "Requires authorization" to the PluginGroup
information. That way, we have all the metadata in one place and can reuse the
smart (sorta) plugin matching.

[Chris Evans, 2011-01-25 05:12:46]

On 2011/01/24 12:56:33, Bernhard Bauer wrote:
>
http://codereview.chromium.org/6350010/diff/1/chrome/browser/tab_contents/tab...
> File chrome/browser/tab_contents/tab_contents.cc (right):
> 
>
http://codereview.chromium.org/6350010/diff/1/chrome/browser/tab_contents/tab...
> chrome/browser/tab_contents/tab_contents.cc:380:
> UserMetrics::RecordAction(UserMetricsAction(uma.c_str()));
> The problem with this is if you don't use a string literal for the
> UserMetricsAction, the script won't pick it up.

Oh! I will fix this in the next iteration. Do you have a link to this script so
I can see what it is doing?

> 
> Also, it's probably going to be a bit harder to find out aggregated numbers
for
> the infobar (how often is it shown in total, etc).

Ok, I'll add aggregate info in the next iteration.

> 
> http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc
> File chrome/renderer/render_view.cc (right):
> 
>
http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc#...
> chrome/renderer/render_view.cc:2708:
> !cmd->HasSwitch(switches::kAllowOutdatedPlugins)) {
> What is the interaction with blocking outdated plugins? If someone uses an old
> version of Quicktime, will they get an infobar telling them to upgrade, and
> after they have upgraded, this one? Right now, I'm trying to use the outdated
> plugin infobar to incentivize people to upgrade their plugins ("if you want to
> get rid of this infobar, upgrade your plugin"), and if you're always going to
> get the infobar, that incentive goes away.

Yes, that's correct, for a small number of dangerous plug-ins. I've now added
the "Always run for this site" button and wired it in as discussed. (Changes
uploaded).

> 
>
http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc#...
> chrome/renderer/render_view.cc:2729: host_setting == CONTENT_SETTING_DEFAULT)
{
> Combined with the changes in HostContentSettingsMap, what are the precedences
> now? IIUC, in order for someone to get the infobar, they need to have a
default
> plugin setting of "Allow" and no exceptions (of any sort) for the current
> website, correct?

Correct. I believe this leaves us compatible with the full matrix of possible
settings, including forward-compatible with the upcoming per-site per-plugin
plan. This includes an easy metadata transition path.

> 
>
http://codereview.chromium.org/6350010/diff/1/webkit/plugins/npapi/plugin_gro...
> File webkit/plugins/npapi/plugin_group.cc (right):
> 
>
http://codereview.chromium.org/6350010/diff/1/webkit/plugins/npapi/plugin_gro...
> webkit/plugins/npapi/plugin_group.cc:381: return group_name_ ==
> ASCIIToUTF16(kJavaGroupName) ||
> I think I'd prefer to reuse the rest of the PluginGroup infrastructure here,
> i.e. add an additional bit "Requires authorization" to the PluginGroup
> information. That way, we have all the metadata in one place and can reuse the
> smart (sorta) plugin matching.

Nice idea. It'll give us per-OS decisions too. Done. I put the actual flag on
the version range, so we could event make decisions at the granularity of:
Reader 8, not so cool; Reader X, much better due to built-in sandbox.

[Bernhard Bauer, 2011-01-25 10:15:38]

On Tue, Jan 25, 2011 at 05:12,  <cevans@chromium.org> wrote:
> On 2011/01/24 12:56:33, Bernhard Bauer wrote:
>
>
http://codereview.chromium.org/6350010/diff/1/chrome/browser/tab_contents/tab...
>>
>> File chrome/browser/tab_contents/tab_contents.cc (right):
>
>
>
http://codereview.chromium.org/6350010/diff/1/chrome/browser/tab_contents/tab...
>>
>> chrome/browser/tab_contents/tab_contents.cc:380:
>> UserMetrics::RecordAction(UserMetricsAction(uma.c_str()));
>> The problem with this is if you don't use a string literal for the
>> UserMetricsAction, the script won't pick it up.
>
> Oh! I will fix this in the next iteration. Do you have a link to this script
> so
> I can see what it is doing?

It's chrome/tools/extract_actions.py.

>> Also, it's probably going to be a bit harder to find out aggregated
>> numbers
>
> for
>>
>> the infobar (how often is it shown in total, etc).
>
> Ok, I'll add aggregate info in the next iteration.
>
>
>>
>> http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc
>> File chrome/renderer/render_view.cc (right):
>
>
>
http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc#...
>>
>> chrome/renderer/render_view.cc:2708:
>> !cmd->HasSwitch(switches::kAllowOutdatedPlugins)) {
>> What is the interaction with blocking outdated plugins? If someone uses an
>> old
>> version of Quicktime, will they get an infobar telling them to upgrade,
>> and
>> after they have upgraded, this one? Right now, I'm trying to use the
>> outdated
>> plugin infobar to incentivize people to upgrade their plugins ("if you
>> want to
>> get rid of this infobar, upgrade your plugin"), and if you're always going
>> to
>> get the infobar, that incentive goes away.
>
> Yes, that's correct, for a small number of dangerous plug-ins. I've now
> added
> the "Always run for this site" button and wired it in as discussed. (Changes
> uploaded).

Hmm, can we still try to improve the messaging? As it stands right
now, if the user updates the plugin, restarts the browser and still
gets an infobar, they're actually punished for updating the plugin (by
having to restart), so we should at least manage expectations there.

>
http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc#...
>>
>> chrome/renderer/render_view.cc:2729: host_setting ==
>> CONTENT_SETTING_DEFAULT)
>
> {
>>
>> Combined with the changes in HostContentSettingsMap, what are the
>> precedences
>> now? IIUC, in order for someone to get the infobar, they need to have a
>
> default
>>
>> plugin setting of "Allow" and no exceptions (of any sort) for the current
>> website, correct?
>
> Correct. I believe this leaves us compatible with the full matrix of
> possible
> settings, including forward-compatible with the upcoming per-site per-plugin
> plan. This includes an easy metadata transition path.

OK… I think that should work, although maybe we could even show the
infobar for a click-to-play default (as click-to-play is not a
security feature, so having the plugin run with a single click is
actually a little bit less secure than showing an infobar).

>
http://codereview.chromium.org/6350010/diff/1/webkit/plugins/npapi/plugin_gro...
>>
>> File webkit/plugins/npapi/plugin_group.cc (right):
>
>
>
http://codereview.chromium.org/6350010/diff/1/webkit/plugins/npapi/plugin_gro...
>>
>> webkit/plugins/npapi/plugin_group.cc:381: return group_name_ ==
>> ASCIIToUTF16(kJavaGroupName) ||
>> I think I'd prefer to reuse the rest of the PluginGroup infrastructure
>> here,
>> i.e. add an additional bit "Requires authorization" to the PluginGroup
>> information. That way, we have all the metadata in one place and can reuse
>> the
>> smart (sorta) plugin matching.
>
> Nice idea. It'll give us per-OS decisions too. Done. I put the actual flag
> on
> the version range, so we could event make decisions at the granularity of:
> Reader 8, not so cool; Reader X, much better due to built-in sandbox.

Yeah, that's good.

> http://codereview.chromium.org/6350010/
>

[Chris Evans, 2011-01-26 05:02:42]

On 2011/01/25 10:15:38, Bernhard Bauer wrote:
> On Tue, Jan 25, 2011 at 05:12,  <mailto:cevans@chromium.org> wrote:
> > On 2011/01/24 12:56:33, Bernhard Bauer wrote:
> >
> >
>
http://codereview.chromium.org/6350010/diff/1/chrome/browser/tab_contents/tab...
> >>
> >> File chrome/browser/tab_contents/tab_contents.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/6350010/diff/1/chrome/browser/tab_contents/tab...
> >>
> >> chrome/browser/tab_contents/tab_contents.cc:380:
> >> UserMetrics::RecordAction(UserMetricsAction(uma.c_str()));
> >> The problem with this is if you don't use a string literal for the
> >> UserMetricsAction, the script won't pick it up.
> >
> > Oh! I will fix this in the next iteration. Do you have a link to this script
> > so
> > I can see what it is doing?
> 
> It's chrome/tools/extract_actions.py.

Thanks. Done.

> 
> >> Also, it's probably going to be a bit harder to find out aggregated
> >> numbers
> >
> > for
> >>
> >> the infobar (how often is it shown in total, etc).
> >
> > Ok, I'll add aggregate info in the next iteration.
> >
> >
> >>
> >>
http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc
> >> File chrome/renderer/render_view.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc#...
> >>
> >> chrome/renderer/render_view.cc:2708:
> >> !cmd->HasSwitch(switches::kAllowOutdatedPlugins)) {
> >> What is the interaction with blocking outdated plugins? If someone uses an
> >> old
> >> version of Quicktime, will they get an infobar telling them to upgrade,
> >> and
> >> after they have upgraded, this one? Right now, I'm trying to use the
> >> outdated
> >> plugin infobar to incentivize people to upgrade their plugins ("if you
> >> want to
> >> get rid of this infobar, upgrade your plugin"), and if you're always going
> >> to
> >> get the infobar, that incentive goes away.
> >
> > Yes, that's correct, for a small number of dangerous plug-ins. I've now
> > added
> > the "Always run for this site" button and wired it in as discussed. (Changes
> > uploaded).
> 
> Hmm, can we still try to improve the messaging? As it stands right
> now, if the user updates the plugin, restarts the browser and still
> gets an infobar, they're actually punished for updating the plugin (by
> having to restart), so we should at least manage expectations there.

Hmm - didn't I see a CL go past which does runtime refreshing of plugins by
monitoring file changes? Or maybe I'm dreaming.
At any rate, new the "Allow always" button would come in to play after the
restart, and gets rid of the infobar without any additional restart.

> 
> >
>
http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc#...
> >>
> >> chrome/renderer/render_view.cc:2729: host_setting ==
> >> CONTENT_SETTING_DEFAULT)
> >
> > {
> >>
> >> Combined with the changes in HostContentSettingsMap, what are the
> >> precedences
> >> now? IIUC, in order for someone to get the infobar, they need to have a
> >
> > default
> >>
> >> plugin setting of "Allow" and no exceptions (of any sort) for the current
> >> website, correct?
> >
> > Correct. I believe this leaves us compatible with the full matrix of
> > possible
> > settings, including forward-compatible with the upcoming per-site per-plugin
> > plan. This includes an easy metadata transition path.
> 
> OK… I think that should work, although maybe we could even show the
> infobar for a click-to-play default (as click-to-play is not a
> security feature, so having the plugin run with a single click is
> actually a little bit less secure than showing an infobar).

I thought about it a bit. And I think it's a good idea. Done.

PTAL? If we're going to experiment with this, there's no better time to land it.
It would be canary-only for a while due to M10 taking up the dev channel. M11
won't even hit dev for a while.

Cheers
Chris

> 
> >
>
http://codereview.chromium.org/6350010/diff/1/webkit/plugins/npapi/plugin_gro...
> >>
> >> File webkit/plugins/npapi/plugin_group.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/6350010/diff/1/webkit/plugins/npapi/plugin_gro...
> >>
> >> webkit/plugins/npapi/plugin_group.cc:381: return group_name_ ==
> >> ASCIIToUTF16(kJavaGroupName) ||
> >> I think I'd prefer to reuse the rest of the PluginGroup infrastructure
> >> here,
> >> i.e. add an additional bit "Requires authorization" to the PluginGroup
> >> information. That way, we have all the metadata in one place and can reuse
> >> the
> >> smart (sorta) plugin matching.
> >
> > Nice idea. It'll give us per-OS decisions too. Done. I put the actual flag
> > on
> > the version range, so we could event make decisions at the granularity of:
> > Reader 8, not so cool; Reader X, much better due to built-in sandbox.
> 
> Yeah, that's good.
> 
> > http://codereview.chromium.org/6350010/
> >

[Bernhard Bauer, 2011-01-26 12:49:25]

On 2011/01/26 05:02:42, Chris Evans wrote:
> On 2011/01/25 10:15:38, Bernhard Bauer wrote:
> > On Tue, Jan 25, 2011 at 05:12,  <mailto:cevans@chromium.org> wrote:
> > > On 2011/01/24 12:56:33, Bernhard Bauer wrote:
> > >
> > >
> >
>
http://codereview.chromium.org/6350010/diff/1/chrome/browser/tab_contents/tab...
> > >>
> > >> File chrome/browser/tab_contents/tab_contents.cc (right):
> > >
> > >
> > >
> >
>
http://codereview.chromium.org/6350010/diff/1/chrome/browser/tab_contents/tab...
> > >>
> > >> chrome/browser/tab_contents/tab_contents.cc:380:
> > >> UserMetrics::RecordAction(UserMetricsAction(uma.c_str()));
> > >> The problem with this is if you don't use a string literal for the
> > >> UserMetricsAction, the script won't pick it up.
> > >
> > > Oh! I will fix this in the next iteration. Do you have a link to this
script
> > > so
> > > I can see what it is doing?
> > 
> > It's chrome/tools/extract_actions.py.
> 
> Thanks. Done.
> 
> > 
> > >> Also, it's probably going to be a bit harder to find out aggregated
> > >> numbers
> > >
> > > for
> > >>
> > >> the infobar (how often is it shown in total, etc).
> > >
> > > Ok, I'll add aggregate info in the next iteration.
> > >
> > >
> > >>
> > >>
> http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc
> > >> File chrome/renderer/render_view.cc (right):
> > >
> > >
> > >
> >
>
http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc#...
> > >>
> > >> chrome/renderer/render_view.cc:2708:
> > >> !cmd->HasSwitch(switches::kAllowOutdatedPlugins)) {
> > >> What is the interaction with blocking outdated plugins? If someone uses
an
> > >> old
> > >> version of Quicktime, will they get an infobar telling them to upgrade,
> > >> and
> > >> after they have upgraded, this one? Right now, I'm trying to use the
> > >> outdated
> > >> plugin infobar to incentivize people to upgrade their plugins ("if you
> > >> want to
> > >> get rid of this infobar, upgrade your plugin"), and if you're always
going
> > >> to
> > >> get the infobar, that incentive goes away.
> > >
> > > Yes, that's correct, for a small number of dangerous plug-ins. I've now
> > > added
> > > the "Always run for this site" button and wired it in as discussed.
(Changes
> > > uploaded).
> > 
> > Hmm, can we still try to improve the messaging? As it stands right
> > now, if the user updates the plugin, restarts the browser and still
> > gets an infobar, they're actually punished for updating the plugin (by
> > having to restart), so we should at least manage expectations there.
> 
> Hmm - didn't I see a CL go past which does runtime refreshing of plugins by
> monitoring file changes? Or maybe I'm dreaming.

Yeah, but we don't use it on Mac yet, because FSEvents apparently creates too
many false positives (especially when AV software is involved). Also, a plugin
installer could still require the user to quit the browser before it installs
the plugin :-(

> At any rate, new the "Allow always" button would come in to play after the
> restart, and gets rid of the infobar without any additional restart.

Just looking at the "get rid of annoying infobars" perspective I like the idea
of adding an exception for a specific plugin (and version), not a site, although
maybe security-wise the latter is better.

> > 
> > >
> >
>
http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc#...
> > >>
> > >> chrome/renderer/render_view.cc:2729: host_setting ==
> > >> CONTENT_SETTING_DEFAULT)
> > >
> > > {
> > >>
> > >> Combined with the changes in HostContentSettingsMap, what are the
> > >> precedences
> > >> now? IIUC, in order for someone to get the infobar, they need to have a
> > >
> > > default
> > >>
> > >> plugin setting of "Allow" and no exceptions (of any sort) for the current
> > >> website, correct?
> > >
> > > Correct. I believe this leaves us compatible with the full matrix of
> > > possible
> > > settings, including forward-compatible with the upcoming per-site
per-plugin
> > > plan. This includes an easy metadata transition path.
> > 
> > OK… I think that should work, although maybe we could even show the
> > infobar for a click-to-play default (as click-to-play is not a
> > security feature, so having the plugin run with a single click is
> > actually a little bit less secure than showing an infobar).
> 
> I thought about it a bit. And I think it's a good idea. Done.
> 
> PTAL? If we're going to experiment with this, there's no better time to land
it.

LGTM with a nit below.

> It would be canary-only for a while due to M10 taking up the dev channel. M11
> won't even hit dev for a while.
> 
> Cheers
> Chris
> 
> > 
> > >
> >
>
http://codereview.chromium.org/6350010/diff/1/webkit/plugins/npapi/plugin_gro...
> > >>
> > >> File webkit/plugins/npapi/plugin_group.cc (right):
> > >
> > >
> > >
> >
>
http://codereview.chromium.org/6350010/diff/1/webkit/plugins/npapi/plugin_gro...
> > >>
> > >> webkit/plugins/npapi/plugin_group.cc:381: return group_name_ ==
> > >> ASCIIToUTF16(kJavaGroupName) ||
> > >> I think I'd prefer to reuse the rest of the PluginGroup infrastructure
> > >> here,
> > >> i.e. add an additional bit "Requires authorization" to the PluginGroup
> > >> information. That way, we have all the metadata in one place and can
reuse
> > >> the
> > >> smart (sorta) plugin matching.
> > >
> > > Nice idea. It'll give us per-OS decisions too. Done. I put the actual flag
> > > on
> > > the version range, so we could event make decisions at the granularity of:
> > > Reader 8, not so cool; Reader X, much better due to built-in sandbox.
> > 
> > Yeah, that's good.
> > 
> > > http://codereview.chromium.org/6350010/
> > >

http://codereview.chromium.org/6350010/diff/23001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6350010/diff/23001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:2736: // bubble.
Nit: Comment unnecessary?

[cevans, 2011-01-26 23:02:45]

On Wed, Jan 26, 2011 at 4:49 AM, <bauerb@chromium.org> wrote:

> On 2011/01/26 05:02:42, Chris Evans wrote:
>
>> On 2011/01/25 10:15:38, Bernhard Bauer wrote:
>> > On Tue, Jan 25, 2011 at 05:12,  <mailto:cevans@chromium.org> wrote:
>> > > On 2011/01/24 12:56:33, Bernhard Bauer wrote:
>> > >
>> > >
>> >
>>
>
>
>
http://codereview.chromium.org/6350010/diff/1/chrome/browser/tab_contents/tab...
>
>> > >>
>> > >> File chrome/browser/tab_contents/tab_contents.cc (right):
>> > >
>> > >
>> > >
>> >
>>
>
>
>
http://codereview.chromium.org/6350010/diff/1/chrome/browser/tab_contents/tab...
>
>> > >>
>> > >> chrome/browser/tab_contents/tab_contents.cc:380:
>> > >> UserMetrics::RecordAction(UserMetricsAction(uma.c_str()));
>> > >> The problem with this is if you don't use a string literal for the
>> > >> UserMetricsAction, the script won't pick it up.
>> > >
>> > > Oh! I will fix this in the next iteration. Do you have a link to this
>>
> script
>
>> > > so
>> > > I can see what it is doing?
>> >
>> > It's chrome/tools/extract_actions.py.
>>
>
>  Thanks. Done.
>>
>
>  >
>> > >> Also, it's probably going to be a bit harder to find out aggregated
>> > >> numbers
>> > >
>> > > for
>> > >>
>> > >> the infobar (how often is it shown in total, etc).
>> > >
>> > > Ok, I'll add aggregate info in the next iteration.
>> > >
>> > >
>> > >>
>> > >>
>>
>> http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc
>> > >> File chrome/renderer/render_view.cc (right):
>> > >
>> > >
>> > >
>> >
>>
>
>
>
http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc#...
>
>> > >>
>> > >> chrome/renderer/render_view.cc:2708:
>> > >> !cmd->HasSwitch(switches::kAllowOutdatedPlugins)) {
>> > >> What is the interaction with blocking outdated plugins? If someone
>> uses
>>
> an
>
>> > >> old
>> > >> version of Quicktime, will they get an infobar telling them to
>> upgrade,
>> > >> and
>> > >> after they have upgraded, this one? Right now, I'm trying to use the
>> > >> outdated
>> > >> plugin infobar to incentivize people to upgrade their plugins ("if
>> you
>> > >> want to
>> > >> get rid of this infobar, upgrade your plugin"), and if you're always
>>
> going
>
>> > >> to
>> > >> get the infobar, that incentive goes away.
>> > >
>> > > Yes, that's correct, for a small number of dangerous plug-ins. I've
>> now
>> > > added
>> > > the "Always run for this site" button and wired it in as discussed.
>>
> (Changes
>
>> > > uploaded).
>> >
>> > Hmm, can we still try to improve the messaging? As it stands right
>> > now, if the user updates the plugin, restarts the browser and still
>> > gets an infobar, they're actually punished for updating the plugin (by
>> > having to restart), so we should at least manage expectations there.
>>
>
>  Hmm - didn't I see a CL go past which does runtime refreshing of plugins
>> by
>> monitoring file changes? Or maybe I'm dreaming.
>>
>
> Yeah, but we don't use it on Mac yet, because FSEvents apparently creates
> too
> many false positives (especially when AV software is involved). Also, a
> plugin
> installer could still require the user to quit the browser before it
> installs
> the plugin :-(
>
>
>  At any rate, new the "Allow always" button would come in to play after the
>> restart, and gets rid of the infobar without any additional restart.
>>
>
> Just looking at the "get rid of annoying infobars" perspective I like the
> idea
> of adding an exception for a specific plugin (and version), not a site,
> although
> maybe security-wise the latter is better.
>
>
>  >
>> > >
>> >
>>
>
>
>
http://codereview.chromium.org/6350010/diff/1/chrome/renderer/render_view.cc#...
>
>> > >>
>> > >> chrome/renderer/render_view.cc:2729: host_setting ==
>> > >> CONTENT_SETTING_DEFAULT)
>> > >
>> > > {
>> > >>
>> > >> Combined with the changes in HostContentSettingsMap, what are the
>> > >> precedences
>> > >> now? IIUC, in order for someone to get the infobar, they need to have
>> a
>> > >
>> > > default
>> > >>
>> > >> plugin setting of "Allow" and no exceptions (of any sort) for the
>> current
>> > >> website, correct?
>> > >
>> > > Correct. I believe this leaves us compatible with the full matrix of
>> > > possible
>> > > settings, including forward-compatible with the upcoming per-site
>>
> per-plugin
>
>> > > plan. This includes an easy metadata transition path.
>> >
>> > OK… I think that should work, although maybe we could even show the
>> > infobar for a click-to-play default (as click-to-play is not a
>> > security feature, so having the plugin run with a single click is
>> > actually a little bit less secure than showing an infobar).
>>
>
>  I thought about it a bit. And I think it's a good idea. Done.
>>
>
>  PTAL? If we're going to experiment with this, there's no better time to
>> land
>>
> it.
>
> LGTM with a nit below.
>
>
>  It would be canary-only for a while due to M10 taking up the dev channel.
>> M11
>> won't even hit dev for a while.
>>
>
>  Cheers
>> Chris
>>
>
>  >
>> > >
>> >
>>
>
>
>
http://codereview.chromium.org/6350010/diff/1/webkit/plugins/npapi/plugin_gro...
>
>> > >>
>> > >> File webkit/plugins/npapi/plugin_group.cc (right):
>> > >
>> > >
>> > >
>> >
>>
>
>
>
http://codereview.chromium.org/6350010/diff/1/webkit/plugins/npapi/plugin_gro...
>
>> > >>
>> > >> webkit/plugins/npapi/plugin_group.cc:381: return group_name_ ==
>> > >> ASCIIToUTF16(kJavaGroupName) ||
>> > >> I think I'd prefer to reuse the rest of the PluginGroup
>> infrastructure
>> > >> here,
>> > >> i.e. add an additional bit "Requires authorization" to the
>> PluginGroup
>> > >> information. That way, we have all the metadata in one place and can
>>
> reuse
>
>> > >> the
>> > >> smart (sorta) plugin matching.
>> > >
>> > > Nice idea. It'll give us per-OS decisions too. Done. I put the actual
>> flag
>> > > on
>> > > the version range, so we could event make decisions at the granularity
>> of:
>> > > Reader 8, not so cool; Reader X, much better due to built-in sandbox.
>> >
>> > Yeah, that's good.
>> >
>> > > http://codereview.chromium.org/6350010/
>> > >
>>
>
>
>
>
>
>
http://codereview.chromium.org/6350010/diff/23001/chrome/renderer/render_view.cc
>
> File chrome/renderer/render_view.cc (right):
>
>
>
http://codereview.chromium.org/6350010/diff/23001/chrome/renderer/render_view...
> chrome/renderer/render_view.cc:2736: // bubble.
> Nit: Comment unnecessary?


Done.


>
> http://codereview.chromium.org/6350010/
>

[jam, 2011-01-27 20:41:58]

http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:251: // PluginInfoBar
--------------------------------------------------------------
TabContents is not the place to put dump all these infobar delegates!  We're
working on making it smaller and more focused on just displaying a page, and
this is taking it in the opposite direction.

Please create a new WebNavigationObserver that filters these plugin related
messages and has the info bar there.

[brettw, 2011-01-27 20:45:31]

On 2011/01/27 20:41:58, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
> File chrome/browser/tab_contents/tab_contents.cc (right):
> 
>
http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
> chrome/browser/tab_contents/tab_contents.cc:251: // PluginInfoBar
> --------------------------------------------------------------
> TabContents is not the place to put dump all these infobar delegates!  We're
> working on making it smaller and more focused on just displaying a page, and
> this is taking it in the opposite direction.
> 
> Please create a new WebNavigationObserver that filters these plugin related
> messages and has the info bar there.

BTW Please make sure you have the right reviewers for your change. In this case
there should have been somebody familiar with TabContents or the info bar code.
Somebody knowledgeable about either of those systems would likely have known how
to suggest this should have been integrated.

[jam, 2011-01-27 20:52:38]

On Thu, Jan 27, 2011 at 12:41 PM, <jam@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
>
> File chrome/browser/tab_contents/tab_contents.cc (right):
>
>
>
http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
> chrome/browser/tab_contents/tab_contents.cc:251: // PluginInfoBar
> --------------------------------------------------------------
> TabContents is not the place to put dump all these infobar delegates!
> We're working on making it smaller and more focused on just displaying a
> page, and this is taking it in the opposite direction.
>
> Please create a new WebNavigationObserver that filters these plugin
> related messages and has the info bar there.


btw there was a message sent about this to chromium-dev about this two days
ago (titled " Project Carnitas - Code Cleanup").  I don't mind going through
and cleaning up the code that was added before we started pushing for a
smaller TabContents, but I can't also move code afterwards since it should
have been done the suggested way.  Otherwise we'll never finish the cleanup.

If this isn't moved out in a day or two we'll have to revert it and it can
be committed later using the more modular way.



>
> http://codereview.chromium.org/6350010/
>

[cevans, 2011-01-27 20:59:31]

On Thu, Jan 27, 2011 at 12:48 PM, John Abd-El-Malek <jam@chromium.org>wrote:

>
>
> On Thu, Jan 27, 2011 at 12:41 PM, <jam@chromium.org> wrote:
>
>>
>>
>>
http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
>>
>> File chrome/browser/tab_contents/tab_contents.cc (right):
>>
>>
>>
http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
>> chrome/browser/tab_contents/tab_contents.cc:251: // PluginInfoBar
>> --------------------------------------------------------------
>> TabContents is not the place to put dump all these infobar delegates!
>> We're working on making it smaller and more focused on just displaying a
>> page, and this is taking it in the opposite direction.
>>
>> Please create a new WebNavigationObserver that filters these plugin
>> related messages and has the info bar there.
>
>
> btw there was a message sent about this to chromium-dev about this two days
> ago (titled " Project Carnitas - Code Cleanup").  I don't mind going through
> and cleaning up the code that was added before we started pushing for a
> smaller TabContents, but I can't also move code afterwards since it should
> have been done the suggested way.  Otherwise we'll never finish the cleanup.
>
> If this isn't moved out in a day or two we'll have to revert it and it can
> be committed later using the more modular way.
>

I refactored an existing infobar (that has been there for a while) into two
variants with a common base class. So I'm not sure a revert is necessary :)
That said, I'll file a bug right away to fix this and Berhard and I will
draw straws.


Cheers
Chris


>
>
>>
>> http://codereview.chromium.org/6350010/
>>
>
>

[brettw at google, 2011-01-27 21:07:14]

On Thu, Jan 27, 2011 at 12:59 PM, Chris Evans <cevans@google.com> wrote:
> On Thu, Jan 27, 2011 at 12:48 PM, John Abd-El-Malek <jam@chromium.org>
> wrote:
>>
>>
>> On Thu, Jan 27, 2011 at 12:41 PM, <jam@chromium.org> wrote:
>>>
>>>
>>>
http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
>>> File chrome/browser/tab_contents/tab_contents.cc (right):
>>>
>>>
>>>
http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
>>> chrome/browser/tab_contents/tab_contents.cc:251: // PluginInfoBar
>>> --------------------------------------------------------------
>>> TabContents is not the place to put dump all these infobar delegates!
>>> We're working on making it smaller and more focused on just displaying a
>>> page, and this is taking it in the opposite direction.
>>>
>>> Please create a new WebNavigationObserver that filters these plugin
>>> related messages and has the info bar there.
>>
>> btw there was a message sent about this to chromium-dev about this two
>> days ago (titled " Project Carnitas - Code Cleanup").  I don't mind going
>> through and cleaning up the code that was added before we started pushing
>> for a smaller TabContents, but I can't also move code afterwards since it
>> should have been done the suggested way.  Otherwise we'll never finish the
>> cleanup.
>> If this isn't moved out in a day or two we'll have to revert it and it can
>> be committed later using the more modular way.
>
> I refactored an existing infobar (that has been there for a while) into two
> variants with a common base class. So I'm not sure a revert is necessary :)
> That said, I'll file a bug right away to fix this and Berhard and I will
> draw straws.

Yeah, none of that old infobar crap should have been in there in the
first place :)

[jam, 2011-01-27 21:20:30]

On Thu, Jan 27, 2011 at 12:59 PM, Chris Evans <cevans@google.com> wrote:

> On Thu, Jan 27, 2011 at 12:48 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>>
>>
>> On Thu, Jan 27, 2011 at 12:41 PM, <jam@chromium.org> wrote:
>>
>>>
>>>
>>>
http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
>>>
>>> File chrome/browser/tab_contents/tab_contents.cc (right):
>>>
>>>
>>>
http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
>>> chrome/browser/tab_contents/tab_contents.cc:251: // PluginInfoBar
>>> --------------------------------------------------------------
>>> TabContents is not the place to put dump all these infobar delegates!
>>> We're working on making it smaller and more focused on just displaying a
>>> page, and this is taking it in the opposite direction.
>>>
>>> Please create a new WebNavigationObserver that filters these plugin
>>> related messages and has the info bar there.
>>
>>
>> btw there was a message sent about this to chromium-dev about this two
>> days ago (titled " Project Carnitas - Code Cleanup").  I don't mind going
>> through and cleaning up the code that was added before we started pushing
>> for a smaller TabContents, but I can't also move code afterwards since it
>> should have been done the suggested way.  Otherwise we'll never finish the
>> cleanup.
>>
>> If this isn't moved out in a day or two we'll have to revert it and it can
>> be committed later using the more modular way.
>>
>
> I refactored an existing infobar (that has been there for a while) into two
> variants with a common base class. So I'm not sure a revert is necessary :)
> That said, I'll file a bug right away to fix this and Berhard and I will
> draw straws.
>

Thanks.  Like Brett said, the old info bar didn't belong there, and this
change made 100 lines that didn't belong here into 200 lines.  Will whoever
draw the straw prioritize this real soon?


>
>
> Cheers
> Chris
>
>
>>
>>
>>>
>>> http://codereview.chromium.org/6350010/
>>>
>>
>>
>

[Bernhard Bauer, 2011-01-27 21:23:38]

I'll do it tomorrow; I've got some free cycles anyway.

Sorry for the mess.

On Thursday, January 27, 2011, John Abd-El-Malek <jam@chromium.org> wrote:
>
>
> On Thu, Jan 27, 2011 at 12:59 PM, Chris Evans <cevans@google.com> wrote:
>
> On Thu, Jan 27, 2011 at 12:48 PM, John Abd-El-Malek <jam@chromium.org> wrote:
>
>
>
>
> On Thu, Jan 27, 2011 at 12:41 PM,  <jam@chromium.org> wrote:
>
>
>
>
>
http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
>
>
>
> File chrome/browser/tab_contents/tab_contents.cc (right):
>
>
http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
> chrome/browser/tab_contents/tab_contents.cc:251: // PluginInfoBar
> --------------------------------------------------------------
> TabContents is not the place to put dump all these infobar delegates!
> We're working on making it smaller and more focused on just displaying a
> page, and this is taking it in the opposite direction.
>
> Please create a new WebNavigationObserver that filters these plugin
> related messages and has the info bar there.
> btw there was a message sent about this to chromium-dev about this two days
ago (titled " Project Carnitas - Code Cleanup").  I don't mind going through and
cleaning up the code that was added before we started pushing for a smaller
TabContents, but I can't also move code afterwards since it should have been
done the suggested way.  Otherwise we'll never finish the cleanup.
>
>
>
> If this isn't moved out in a day or two we'll have to revert it and it can be
committed later using the more modular way.
> I refactored an existing infobar (that has been there for a while) into two
variants with a common base class. So I'm not sure a revert is necessary :) That
said, I'll file a bug right away to fix this and Berhard and I will draw straws.
>
> Thanks.  Like Brett said, the old info bar didn't belong there, and this
change made 100 lines that didn't belong here into 200 lines.  Will whoever draw
the straw prioritize this real soon?
>
>
>
> CheersChris
>
>
>
>
>
>
>
> http://codereview.chromium.org/6350010/
>
>
>
>

[jam, 2011-01-27 21:31:31]

Thanks, appreciate it.

Please give it a generic plugin related name as it should handle all the
plugin messages currently being dispatched in TC, i.e.
ViewHostMsg_MissingPluginStatus/ViewHostMsg_CrashedPlugin/ViewHostMsg_BlockedOutdatedPlugin

On Thu, Jan 27, 2011 at 1:23 PM, Bernhard Bauer <bauerb@chromium.org> wrote:

> I'll do it tomorrow; I've got some free cycles anyway.
>
> Sorry for the mess.
>
> On Thursday, January 27, 2011, John Abd-El-Malek <jam@chromium.org> wrote:
> >
> >
> > On Thu, Jan 27, 2011 at 12:59 PM, Chris Evans <cevans@google.com> wrote:
> >
> > On Thu, Jan 27, 2011 at 12:48 PM, John Abd-El-Malek <jam@chromium.org>
> wrote:
> >
> >
> >
> >
> > On Thu, Jan 27, 2011 at 12:41 PM,  <jam@chromium.org> wrote:
> >
> >
> >
> >
> >
>
http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
> >
> >
> >
> > File chrome/browser/tab_contents/tab_contents.cc (right):
> >
> >
>
http://codereview.chromium.org/6350010/diff/34001/chrome/browser/tab_contents...
> > chrome/browser/tab_contents/tab_contents.cc:251: // PluginInfoBar
> > --------------------------------------------------------------
> > TabContents is not the place to put dump all these infobar delegates!
> > We're working on making it smaller and more focused on just displaying a
> > page, and this is taking it in the opposite direction.
> >
> > Please create a new WebNavigationObserver that filters these plugin
> > related messages and has the info bar there.
> > btw there was a message sent about this to chromium-dev about this two
> days ago (titled " Project Carnitas - Code Cleanup").  I don't mind going
> through and cleaning up the code that was added before we started pushing
> for a smaller TabContents, but I can't also move code afterwards since it
> should have been done the suggested way.  Otherwise we'll never finish the
> cleanup.
> >
> >
> >
> > If this isn't moved out in a day or two we'll have to revert it and it
> can be committed later using the more modular way.
> > I refactored an existing infobar (that has been there for a while) into
> two variants with a common base class. So I'm not sure a revert is necessary
> :) That said, I'll file a bug right away to fix this and Berhard and I will
> draw straws.
> >
> > Thanks.  Like Brett said, the old info bar didn't belong there, and this
> change made 100 lines that didn't belong here into 200 lines.  Will whoever
> draw the straw prioritize this real soon?
> >
> >
> >
> > CheersChris
> >
> >
> >
> >
> >
> >
> >
> > http://codereview.chromium.org/6350010/
> >
> >
> >
> >
>