Merge PluginGroups for Adobe Reader

Merge PluginGroups for Adobe Reader

This patch introduces general support in PluginGroup for multiple version ranges with one minimum secure version each, to be used e.g. for multiple major versions currently supported by their manufacturer.

BUG=65207
TEST=test_shell_tests: PluginGroupTest*

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

[Jakob Kummerow, 2010-12-08 12:55:05]

Please review.

panayiotis: I didn't update the version ranges for RealPlayer because I wasn't
sure which versions to put there -- the versions mentioned in your comment seem
to me to be pretty dated.

[Bernhard Bauer, 2010-12-08 13:36:30]

http://codereview.chromium.org/5621006/diff/1/chrome/browser/ui/cocoa/table_m...
File chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm (right):

http://codereview.chromium.org/5621006/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm:21: namespace
plugin_test_internal {
Is that necessary for it to compile? Usually we don't put the test classes
themselves into a namespace (just additional classes used by tests).

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_grou...
File webkit/glue/plugins/plugin_group.cc (right):

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:193: const VersionRange* range =
&version_ranges_[i];
I think you could just use a const VersionRange& here.

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:199: }
I think we could even get rid of this rule. If someone wants to define a plugin
group matching all versions, they can just define no version ranges.

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:213: }
You could move these checks together: 

if (range->low->CompareTo(*plugin_version) <= 0 &&
    (range->high.get() == NULL) || range->high->CompareTo(*plugin_version) > 0))

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:365:
version_range.high->CompareTo(plugin_version) > 0)) {
Could you reuse part of the logic in Match here?

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_group.h
File webkit/glue/plugins/plugin_group.h (right):

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.h:167: static bool IsPluginOutdated(const
Version& plugin_version,
Can you add a comment explaining the semantics of this method? Especially what
happens if |plugin_version| is not contained in |version_range| at all.

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_list.cc
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_list...
webkit/glue/plugins/plugin_list.cc:98: 1,
"http://www.apple.com/quicktime/download/" },
Please use arraysize() instead of hard-coding it.

[Jakob Kummerow, 2010-12-09 08:58:17]

Addressed your comments, please review again.

http://codereview.chromium.org/5621006/diff/1/chrome/browser/ui/cocoa/table_m...
File chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm (right):

http://codereview.chromium.org/5621006/diff/1/chrome/browser/ui/cocoa/table_m...
chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm:21: namespace
plugin_test_internal {
On 2010/12/08 13:36:30, Bernhard Bauer wrote:
> Is that necessary for it to compile? Usually we don't put the test classes
> themselves into a namespace (just additional classes used by tests).

Done.
This was meant to be cleanup work, removed it again after offline discussion.

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_grou...
File webkit/glue/plugins/plugin_group.cc (right):

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:193: const VersionRange* range =
&version_ranges_[i];
On 2010/12/08 13:36:30, Bernhard Bauer wrote:
> I think you could just use a const VersionRange& here.

Done.

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:199: }
On 2010/12/08 13:36:30, Bernhard Bauer wrote:
> I think we could even get rid of this rule. If someone wants to define a
plugin
> group matching all versions, they can just define no version ranges.

No, we can't, because they might want to define a minimum version. See
plugin_list.cc for existing examples.

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:213: }
On 2010/12/08 13:36:30, Bernhard Bauer wrote:
> You could move these checks together: 
> 
> if (range->low->CompareTo(*plugin_version) <= 0 &&
>     (range->high.get() == NULL) || range->high->CompareTo(*plugin_version) >
0))

Done.

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:365:
version_range.high->CompareTo(plugin_version) > 0)) {
On 2010/12/08 13:36:30, Bernhard Bauer wrote:
> Could you reuse part of the logic in Match here?

Done (by refactoring shared code out into a separate method).

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_group.h
File webkit/glue/plugins/plugin_group.h (right):

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.h:167: static bool IsPluginOutdated(const
Version& plugin_version,
On 2010/12/08 13:36:30, Bernhard Bauer wrote:
> Can you add a comment explaining the semantics of this method? Especially what
> happens if |plugin_version| is not contained in |version_range| at all.

Done.

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_list.cc
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/5621006/diff/1/webkit/glue/plugins/plugin_list...
webkit/glue/plugins/plugin_list.cc:98: 1,
"http://www.apple.com/quicktime/download/" },
On 2010/12/08 13:36:30, Bernhard Bauer wrote:
> Please use arraysize() instead of hard-coding it.

Done.

[Bernhard Bauer, 2010-12-09 09:20:54]

LGTM.

[panayiotis, 2010-12-10 00:48:16]

Did a quick pass and looked great to me, just like I was imagining it. 
Regarding RealPlayer: We need to talk with them and figure out exactly what
version are vulnerable. Last I checked, I could not figure out because they
exposed different versions for XP and Vista ... I'll try to find a contact. 

On 2010/12/08 12:55:05, jkummerow wrote:
> Please review.
> 
> panayiotis: I didn't update the version ranges for RealPlayer because I wasn't
> sure which versions to put there -- the versions mentioned in your comment
seem
> to me to be pretty dated.