(1) Group plugins with the same name together. (2) Show a download link for p...

(1) Group plugins with the same name together. (2) Show a download link for plugin versions with known security problems in about:plugins.

BUG=3910
TEST=Open "chrome://plugins", see that plugins are grouped, and that any vulnerable plugins are marked red. Try enabling and disabling plugin groups.

(Eventually landed by bauerb@ in another CL.)

[viettrungluu, 2010-05-07 23:51:20]

You know that the order of the PluginList is meaningful (and dictates how mime
type and file extension matching is done), right?

[mavrommatis, 2010-05-07 23:55:23]

No, that's ... good to know :). I need to keep the order within a group
then, right? Do we need to keep order across groups too?

How does the general method look like, otherwise?

On May 7, 2010 4:51 PM, <viettrungluu@chromium.org> wrote:
> You know that the order of the PluginList is meaningful (and dictates how
> mime
> type and file extension matching is done), right?
>
> http://codereview.chromium.org/1991005/show

[viettrungluu, 2010-05-08 00:07:42]

On 2010/05/07 23:55:23, mavrommatis wrote:
> No, that's ... good to know :). I need to keep the order within a group
> then, right? Do we need to keep order across groups too?

You definitely should keep the order within each group. The order across groups
is also important. Unfortunately, I don't have a good UI solution for you. (It's
even hard to tell which mime types are even superseded due to wildcard
matching.) :-(

Given the way plugins are currently loaded, I don't know if a good solution is
possible.

> How does the general method look like, otherwise?

I definitely like the idea. At a superficial level, the code basically looks
okay (apart from some Chrome-specific issues, such as wstrings being
theoretically deprecated -- use string16s instead). At a deeper level, my
feeling is that we should rework the way plugins are loaded, and specifically
rework the PluginList (in src/webkit/glue/plugins). It's not entirely clear to
me how we should split the work between the PluginList and maybe a delegate on
the more Chrome-specific side.

- Trung

> On May 7, 2010 4:51 PM, <mailto:viettrungluu@chromium.org> wrote:
> > You know that the order of the PluginList is meaningful (and dictates how
> > mime
> > type and file extension matching is done), right?
> >
> > http://codereview.chromium.org/1991005/show
>

[Paweł Hajdan Jr., 2010-05-08 07:56:10]

Drive-by with test comments.

http://codereview.chromium.org/1991005/diff/1/7
File chrome/browser/plugin_updater_unittest.cc (right):

http://codereview.chromium.org/1991005/diff/1/7#newcode119
chrome/browser/plugin_updater_unittest.cc:119: EXPECT_TRUE(def_group.get() !=
NULL);
Could you either make that an ASSERT, or add an additional NULL-check after so
that we don't crash entire unit_tests in case of failure?

http://codereview.chromium.org/1991005/diff/1/7#newcode128
chrome/browser/plugin_updater_unittest.cc:128: EXPECT_TRUE(def_group.get() !=
NULL);
Similarly here. Please also verify whether other tests in this file need similar
NULL-checks.

[jam, 2010-05-10 18:37:59]

On 2010/05/07 23:51:20, viettrungluu wrote:
> You know that the order of the PluginList is meaningful (and dictates how mime
> type and file extension matching is done), right?

I think most users won't know or care about how we pick which plugin to use for
a specific mime type/extension...  That seems like an internal detail to Chrome.

[viettrungluu, 2010-05-10 18:40:13]

On 2010/05/10 18:37:59, John Abd-El-Malek wrote:
> On 2010/05/07 23:51:20, viettrungluu wrote:
> > You know that the order of the PluginList is meaningful (and dictates how
mime
> > type and file extension matching is done), right?
> 
> I think most users won't know or care about how we pick which plugin to use
for
> a specific mime type/extension...  That seems like an internal detail to
Chrome.

I agree, but we should make sure that the plugin shown for a given mime
type/extension is in fact the one that's used!

[mavrommatis, 2010-05-15 19:26:36]

Please see the new patch.

Fixed:
- asserts in unittests
- string16 (I probably didn't get that 100% right still)
- showing plugin priority in the "details"
- showing "enable/disable" link for every plugin in "details"
- updated shockwave
- tested on Windows (thanks Noe) and added some "parsing" of the
plugin versions so they can be loaded in the Version object.
Unittested
- if we can't read the version of a plugin for some reason we don't
mark it as vulnerable (to prevent eg a new plugin push that changes
the versioning string to something we don't parse)

Thanks again for the comments and the quick review.

On Mon, May 10, 2010 at 11:40 AM,  <viettrungluu@chromium.org> wrote:
> On 2010/05/10 18:37:59, John Abd-El-Malek wrote:
>>
>> On 2010/05/07 23:51:20, viettrungluu wrote:
>> > You know that the order of the PluginList is meaningful (and dictates
>> > how
>
> mime
>>
>> > type and file extension matching is done), right?
>
>> I think most users won't know or care about how we pick which plugin to
>> use
>
> for
>>
>> a specific mime type/extension...  That seems like an internal detail to
>
> Chrome.
>
> I agree, but we should make sure that the plugin shown for a given mime
> type/extension is in fact the one that's used!
>
> http://codereview.chromium.org/1991005/show
>

[jam, 2010-05-18 22:39:22]

http://codereview.chromium.org/1991005/diff/25001/26001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/1991005/diff/25001/26001#newcode3669
chrome/app/generated_resources.grd:3669: Download Critical Security Update
Will all later versions be "critical security updates"?

http://codereview.chromium.org/1991005/diff/25001/26005
File chrome/browser/plugin_updater.h (right):

http://codereview.chromium.org/1991005/diff/25001/26005#newcode28
chrome/browser/plugin_updater.h:28: } PluginGroupDef;
nit: chrome style isn't to abbreviate words in names

http://codereview.chromium.org/1991005/diff/25001/26005#newcode40
chrome/browser/plugin_updater.h:40: bool Match(const WebPluginInfo& plugin);
const?  also it'd be nice to document all the functions

http://codereview.chromium.org/1991005/diff/25001/26005#newcode43
chrome/browser/plugin_updater.h:43: const string16 GetGroupName();
should this and other getters be const?

http://codereview.chromium.org/1991005/diff/25001/26006
File chrome/browser/plugin_updater_unittest.cc (right):

http://codereview.chromium.org/1991005/diff/25001/26006#newcode17
chrome/browser/plugin_updater_unittest.cc:17: { "Quicktime", "QuickTime
Plug-in", "", "", "7.6.6",
is there no way to avoid duplicating the data here and below with
plugin_updater.h?  what about adding a getter for this struct so you can use it
from the tests?

[mavrommatis, 2010-05-26 03:13:17]

Thanks John, all fixed.

http://codereview.chromium.org/1991005/diff/25001/26001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/1991005/diff/25001/26001#newcode3669
chrome/app/generated_resources.grd:3669: Download Critical Security Update
On 2010/05/18 22:39:23, John Abd-El-Malek wrote:
> Will all later versions be "critical security updates"?

Yes, we'll only warn on critical security problems. See for example how there's
a Silverlight v4 which just adds new features, so we track both v3 and v4 for
security updates. I think mixing general updates with security updates is
problematic because we might train users to ignore the warnings. 

We've found that to be true in our chrome extension as well.

http://codereview.chromium.org/1991005/diff/25001/26005
File chrome/browser/plugin_updater.h (right):

http://codereview.chromium.org/1991005/diff/25001/26005#newcode28
chrome/browser/plugin_updater.h:28: } PluginGroupDef;
On 2010/05/18 22:39:23, John Abd-El-Malek wrote:
> nit: chrome style isn't to abbreviate words in names

Done.

http://codereview.chromium.org/1991005/diff/25001/26005#newcode40
chrome/browser/plugin_updater.h:40: bool Match(const WebPluginInfo& plugin);
On 2010/05/18 22:39:23, John Abd-El-Malek wrote:
> const?  also it'd be nice to document all the functions

Done.

http://codereview.chromium.org/1991005/diff/25001/26005#newcode43
chrome/browser/plugin_updater.h:43: const string16 GetGroupName();
On 2010/05/18 22:39:23, John Abd-El-Malek wrote:
> should this and other getters be const?

Done.

http://codereview.chromium.org/1991005/diff/25001/26006
File chrome/browser/plugin_updater_unittest.cc (right):

http://codereview.chromium.org/1991005/diff/25001/26006#newcode17
chrome/browser/plugin_updater_unittest.cc:17: { "Quicktime", "QuickTime
Plug-in", "", "", "7.6.6",
On 2010/05/18 22:39:23, John Abd-El-Malek wrote:
> is there no way to avoid duplicating the data here and below with
> plugin_updater.h?  what about adding a getter for this struct so you can use
it
> from the tests?

Done.

[jam, 2010-05-26 16:11:31]

lgtm

[mavrommatis, 2010-05-28 21:08:19]

Thanks John. We've run the tests on windows and mac. I don't have
submit priviledges, how do we get this submitted?

Thanks.

On Wed, May 26, 2010 at 9:11 AM,  <jam@chromium.org> wrote:
> lgtm
>
> http://codereview.chromium.org/1991005/show
>