Factor out PluginGroup into a separate class in common/.

Factor out PluginGroup into a separate class in common/.

Moving PluginGroup into common is necessary in order to enable the renderer process to check if a plugin is outdated. 

Also, make PluginUpdater static since it doesn't contain any state anymore.

BUG=47731
TEST=PluginGroupTest.*

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

[Bernhard Bauer, 2010-07-12 08:37:05]

Please review.

[viettrungluu, 2010-07-12 14:20:15]

Sounds like a good idea to me. Longer term, I'd also like to see more of the
PluginList implementation moved into chrome/common, leaving a bare-bones
implementation in webkit/glue (only enough for test_shell).

http://codereview.chromium.org/2955005/diff/2001/3003
File chrome/browser/plugin_updater.cc (right):

http://codereview.chromium.org/2955005/diff/2001/3003#newcode34
chrome/browser/plugin_updater.cc:34: if (group == NULL) {
!group is more idiomatic.

http://codereview.chromium.org/2955005/diff/2001/3003#newcode204
chrome/browser/plugin_updater.cc:204: if (!(*it)->IsVulnerable()) {
Braces not necessary (but you can keep them if you insist).

http://codereview.chromium.org/2955005/diff/2001/3004
File chrome/browser/plugin_updater.h (right):

http://codereview.chromium.org/2955005/diff/2001/3004#newcode22
chrome/browser/plugin_updater.h:22: class PluginUpdater {
Since it's all static now, there's not much reason for it to be a class rather
than a namespace....

http://codereview.chromium.org/2955005/diff/2001/3004#newcode31
chrome/browser/plugin_updater.h:31: static void EnablePluginFile(bool enable,
const FilePath::StringType& file_path);
80 chars.

http://codereview.chromium.org/2955005/diff/2001/3004#newcode39
chrome/browser/plugin_updater.h:39: private:
And private static methods are a bit silly too. They may as well be in an
anonymous namespace (or static functions) in the implementation.

http://codereview.chromium.org/2955005/diff/2001/3004#newcode40
chrome/browser/plugin_updater.h:40: static void
GetPluginGroups(std::vector<linked_ptr<PluginGroup> >* plugin_groups);
80 chars.

http://codereview.chromium.org/2955005/diff/2001/3007
File chrome/common/plugin_group.cc (right):

http://codereview.chromium.org/2955005/diff/2001/3007#newcode13
chrome/common/plugin_group.cc:13: /*private*/
This comment unnecessary.

[Bernhard Bauer, 2010-07-12 15:01:06]

On 2010/07/12 14:20:15, viettrungluu wrote:
> Sounds like a good idea to me. Longer term, I'd also like to see more of the
> PluginList implementation moved into chrome/common, leaving a bare-bones
> implementation in webkit/glue (only enough for test_shell).
> 
> http://codereview.chromium.org/2955005/diff/2001/3003
> File chrome/browser/plugin_updater.cc (right):
> 
> http://codereview.chromium.org/2955005/diff/2001/3003#newcode34
> chrome/browser/plugin_updater.cc:34: if (group == NULL) {
> !group is more idiomatic.
> 
> http://codereview.chromium.org/2955005/diff/2001/3003#newcode204
> chrome/browser/plugin_updater.cc:204: if (!(*it)->IsVulnerable()) {
> Braces not necessary (but you can keep them if you insist).
> 
> http://codereview.chromium.org/2955005/diff/2001/3004
> File chrome/browser/plugin_updater.h (right):
> 
> http://codereview.chromium.org/2955005/diff/2001/3004#newcode22
> chrome/browser/plugin_updater.h:22: class PluginUpdater {
> Since it's all static now, there's not much reason for it to be a class rather
> than a namespace....
> 
> http://codereview.chromium.org/2955005/diff/2001/3004#newcode31
> chrome/browser/plugin_updater.h:31: static void EnablePluginFile(bool enable,
> const FilePath::StringType& file_path);
> 80 chars.
> 
> http://codereview.chromium.org/2955005/diff/2001/3004#newcode39
> chrome/browser/plugin_updater.h:39: private:
> And private static methods are a bit silly too. They may as well be in an
> anonymous namespace (or static functions) in the implementation.
> 
> http://codereview.chromium.org/2955005/diff/2001/3004#newcode40
> chrome/browser/plugin_updater.h:40: static void
> GetPluginGroups(std::vector<linked_ptr<PluginGroup> >* plugin_groups);
> 80 chars.
> 
> http://codereview.chromium.org/2955005/diff/2001/3007
> File chrome/common/plugin_group.cc (right):
> 
> http://codereview.chromium.org/2955005/diff/2001/3007#newcode13
> chrome/common/plugin_group.cc:13: /*private*/
> This comment unnecessary.

All done, please take another look.

[viettrungluu, 2010-07-12 16:26:14]

LGTM with nits (most of which were probably there before) fixed. Just send it
through the trybots once you're done.

http://codereview.chromium.org/2955005/diff/9001/10003
File chrome/browser/plugin_updater.cc (right):

http://codereview.chromium.org/2955005/diff/9001/10003#newcode210
chrome/browser/plugin_updater.cc:210: 
No trailing blank line.

http://codereview.chromium.org/2955005/diff/9001/10004
File chrome/browser/plugin_updater.h (right):

http://codereview.chromium.org/2955005/diff/9001/10004#newcode15
chrome/browser/plugin_updater.h:15: class DictionaryValue;
Some of these don't appear to be necessary any more.

http://codereview.chromium.org/2955005/diff/9001/10007
File chrome/common/plugin_group.cc (right):

http://codereview.chromium.org/2955005/diff/9001/10007#newcode76
chrome/common/plugin_group.cc:76: const size_t
PluginGroup::GetPluginGroupDefinitionsSize() {
Does something really return "const size_t"?!

http://codereview.chromium.org/2955005/diff/9001/10007#newcode126
chrome/common/plugin_group.cc:126: if (hardcoded_plugin_groups == NULL) {
!hardcoded_plugin_groups will do.

http://codereview.chromium.org/2955005/diff/9001/10007#newcode133
chrome/common/plugin_group.cc:133: groups->push_back(linked_ptr<PluginGroup>(
This looks like it'll fit on one line.

http://codereview.chromium.org/2955005/diff/9001/10007#newcode142
chrome/common/plugin_group.cc:142: if (hardcoded_group != NULL) {
hardcoded_group will do.

http://codereview.chromium.org/2955005/diff/9001/10007#newcode154
chrome/common/plugin_group.cc:154: for (std::vector<linked_ptr<PluginGroup>
>::iterator it =
I believe plugin_groups.begin(); will fit on the previous line.

http://codereview.chromium.org/2955005/diff/9001/10007#newcode166
chrome/common/plugin_group.cc:166: return new PluginGroup(group_name_,
name_matcher_, version_range_low_str_,
You can probably put these one (logical) line methods directly in the header.

http://codereview.chromium.org/2955005/diff/9001/10007#newcode172
chrome/common/plugin_group.cc:172: return group_name_;
Especially this one.

http://codereview.chromium.org/2955005/diff/9001/10007#newcode301
chrome/common/plugin_group.cc:301: NPAPI::PluginList::Singleton()->EnablePlugin(
Fits on one line, I think.

http://codereview.chromium.org/2955005/diff/9001/10007#newcode304
chrome/common/plugin_group.cc:304:
NPAPI::PluginList::Singleton()->DisablePlugin(
Ditto.

http://codereview.chromium.org/2955005/diff/9001/10008
File chrome/common/plugin_group.h (right):

http://codereview.chromium.org/2955005/diff/9001/10008#newcode20
chrome/common/plugin_group.h:20: const char* const name;  // Name of this group.
Presumably you're just moving this from elsewhere, but it seems to have more
consts than is sane (per style guide).

http://codereview.chromium.org/2955005/diff/9001/10008#newcode47
chrome/common/plugin_group.h:47: std::vector<linked_ptr<PluginGroup> >&
plugin_groups,
Indent 2 more spaces.

http://codereview.chromium.org/2955005/diff/9001/10008#newcode67
chrome/common/plugin_group.h:67: // Returns a DictionaryValue with data to
display in the UI.
"GetData" seems a bit generic; maybe it should be "GetUIData" or something like
that?

http://codereview.chromium.org/2955005/diff/9001/10009
File chrome/common/plugin_group_unittest.cc (right):

http://codereview.chromium.org/2955005/diff/9001/10009#newcode1
chrome/common/plugin_group_unittest.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
2010

[Bernhard Bauer, 2010-07-12 17:06:41]

On 2010/07/12 16:26:14, viettrungluu wrote:
> LGTM with nits (most of which were probably there before) fixed. Just send it
> through the trybots once you're done.
 
All done, except for:

> http://codereview.chromium.org/2955005/diff/9001/10007#newcode133
> chrome/common/plugin_group.cc:133: groups->push_back(linked_ptr<PluginGroup>(
> This looks like it'll fit on one line.

Nope, one character too long.

[viettrungluu, 2010-07-13 03:27:49]

(Just reiterating my LGTM....)

And adding danno so that he's aware of his pain....

[mavrommatis, 2010-07-13 04:39:38]

LGTM 

There are some updates, if you want to include in this CL, otherwise we can make
a new one.

http://codereview.chromium.org/2955005/diff/17001/18007
File chrome/common/plugin_group.cc (right):

http://codereview.chromium.org/2955005/diff/17001/18007#newcode22
chrome/common/plugin_group.cc:22: { "Flash", "Shockwave Flash", "", "",
"10.0.45",
this was updated to 10.1.53 at some point, and then it was probably lost in the
numerous reverts. 

Same for Mac.

http://codereview.chromium.org/2955005/diff/17001/18007#newcode42
chrome/common/plugin_group.cc:42: { "Adobe Reader 9", "Adobe Acrobat", "9",
"10", "9.3.2",
9.3.3 was released last weak to fix new vulnerabilities.

http://codereview.chromium.org/2955005/diff/17001/18007#newcode44
chrome/common/plugin_group.cc:44: { "Adobe Reader 8", "Adobe Acrobat", "0", "9",
 "8.2.2",
Version 8 is also at 8.2.3 now.