Add versioning to PluginFinder metadata.

Add versioning to PluginFinder metadata.

TBR=jochen@chromium.org
BUG=170097
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181967

[cevans, 2013-02-05 21:06:51]

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/plugins/plu...
File chrome/browser/plugins/plugin_finder.cc (right):

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_finder.cc:155: scoped_ptr<DictionaryValue>
plugin_list(ComputePluginList());
I think I'd have found a comment here useful, along the lines of "here we're
loading the compiled-in plug-in metadata list. it may be replaced shortly by a
newer downloaded version via class blah"

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_finder.cc:157:
ReinitializePlugins(plugin_list.Pass());
In the common case (ENABLE_PLUGIN_INSTALLATION), ReinitializePlugins() has
already been called?

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_finder.cc:167: base::DictionaryValue* result =
LoadPluginList();
I'm kind of confused -- we just called LoadPluginList() above (conditional to
some ifdef), so why are we doing the work of loading & parsing it again here?

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/resources/p...
File chrome/browser/resources/plugin_metadata/plugins_win.json (right):

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/resources/p...
chrome/browser/resources/plugin_metadata/plugins_win.json:314: "name": "DivX
Plus Web Player",
Can we split metadata updates into a separate CL? It'd be clean to keep this CL
just for the versioning mechanism.
(As an aside, I think we'd want to keep a match on the old name otherwise old
plug-ins will suddenly be up to date)

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/web_resourc...
File chrome/browser/web_resource/plugins_resource_service.cc (right):

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/web_resourc...
chrome/browser/web_resource/plugins_resource_service.cc:65: const
base::DictionaryValue* metadata =
Nit: hold on to it with const ref not ptr, if it cannot be NULL?

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/web_resourc...
chrome/browser/web_resource/plugins_resource_service.cc:68:
make_scoped_ptr(metadata->DeepCopy()));
If however, metadata _could_ be NULL, you have a crash.

[Bernhard Bauer, 2013-02-06 20:08:24]

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/plugins/plu...
File chrome/browser/plugins/plugin_finder.cc (right):

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_finder.cc:155: scoped_ptr<DictionaryValue>
plugin_list(ComputePluginList());
On 2013/02/05 21:06:52, cevans wrote:
> I think I'd have found a comment here useful, along the lines of "here we're
> loading the compiled-in plug-in metadata list. it may be replaced shortly by a
> newer downloaded version via class blah"

Done.

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_finder.cc:167: base::DictionaryValue* result =
LoadPluginList();
On 2013/02/05 21:06:52, cevans wrote:
> I'm kind of confused -- we just called LoadPluginList() above (conditional to
> some ifdef), so why are we doing the work of loading & parsing it again here?

Yes, that's what I meant by "merging it with upstream might have done some
stupid things to it" ;-) My brain wasn't really working when I uploaded it. I've
rewritten it quite a bit now though.

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/resources/p...
File chrome/browser/resources/plugin_metadata/plugins_win.json (right):

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/resources/p...
chrome/browser/resources/plugin_metadata/plugins_win.json:314: "name": "DivX
Plus Web Player",
On 2013/02/05 21:06:52, cevans wrote:
> Can we split metadata updates into a separate CL? It'd be clean to keep this
CL
> just for the versioning mechanism.
> (As an aside, I think we'd want to keep a match on the old name otherwise old
> plug-ins will suddenly be up to date)

Fair enough. Done.

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/web_resourc...
File chrome/browser/web_resource/plugins_resource_service.cc (right):

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/web_resourc...
chrome/browser/web_resource/plugins_resource_service.cc:65: const
base::DictionaryValue* metadata =
On 2013/02/05 21:06:52, cevans wrote:
> Nit: hold on to it with const ref not ptr, if it cannot be NULL?

Hm. The interface returns a const ptr. What's the advantage in using a const
ref? It doesn't actually verify that it's not NULL, AFAIK.

https://codereview.chromium.org/11348046/diff/9001/chrome/browser/web_resourc...
chrome/browser/web_resource/plugins_resource_service.cc:68:
make_scoped_ptr(metadata->DeepCopy()));
On 2013/02/05 21:06:52, cevans wrote:
> If however, metadata _could_ be NULL, you have a crash.

It can't; GetDictionary() returns an empty dictionary by default.

[Chris Evans, 2013-02-08 19:25:23]

LGTM

Couple of nits and one very simple question. Good to land after taking care of
them or not as you see fit :)
Hopefully we can get it in the next dev channel, not sure when that is.

https://codereview.chromium.org/11348046/diff/13003/chrome/browser/plugins/pl...
File chrome/browser/plugins/plugin_finder.cc (right):

https://codereview.chromium.org/11348046/diff/13003/chrome/browser/plugins/pl...
chrome/browser/plugins/plugin_finder.cc:158: scoped_ptr<DictionaryValue>
plugin_list(LoadBuiltInPluginList());
Would it be possible to DCHECK(plugin_list.get()) here?
It'd give more immediate feedback if we typo the JSON file.

https://codereview.chromium.org/11348046/diff/13003/chrome/browser/plugins/pl...
File chrome/browser/plugins/plugin_finder_unittest.cc (right):

https://codereview.chromium.org/11348046/diff/13003/chrome/browser/plugins/pl...
chrome/browser/plugins/plugin_finder_unittest.cc:21: if
(plugin_list->Remove("x-version", &version))
Nit: Now that we are versioning our files, I think it's ok to EXPECT that the
version is always there.

https://codereview.chromium.org/11348046/diff/13003/chrome/browser/web_resour...
File chrome/browser/web_resource/plugins_resource_service.cc (right):

https://codereview.chromium.org/11348046/diff/13003/chrome/browser/web_resour...
chrome/browser/web_resource/plugins_resource_service.cc:67:
prefs_->GetDictionary(prefs::kPluginsMetadata);
Do you need to check for NULL here? Will it return NULL if we don't yet have any
locally downloaded metadata?

[Bernhard Bauer, 2013-02-11 09:58:19]

https://codereview.chromium.org/11348046/diff/13003/chrome/browser/plugins/pl...
File chrome/browser/plugins/plugin_finder.cc (right):

https://codereview.chromium.org/11348046/diff/13003/chrome/browser/plugins/pl...
chrome/browser/plugins/plugin_finder.cc:158: scoped_ptr<DictionaryValue>
plugin_list(LoadBuiltInPluginList());
On 2013/02/08 19:25:23, Chris Evans wrote:
> Would it be possible to DCHECK(plugin_list.get()) here?
> It'd give more immediate feedback if we typo the JSON file.

Done (although we have unit tests for that).

https://codereview.chromium.org/11348046/diff/13003/chrome/browser/plugins/pl...
File chrome/browser/plugins/plugin_finder_unittest.cc (right):

https://codereview.chromium.org/11348046/diff/13003/chrome/browser/plugins/pl...
chrome/browser/plugins/plugin_finder_unittest.cc:21: if
(plugin_list->Remove("x-version", &version))
On 2013/02/08 19:25:23, Chris Evans wrote:
> Nit: Now that we are versioning our files, I think it's ok to EXPECT that the
> version is always there.

Done.

https://codereview.chromium.org/11348046/diff/13003/chrome/browser/web_resour...
File chrome/browser/web_resource/plugins_resource_service.cc (right):

https://codereview.chromium.org/11348046/diff/13003/chrome/browser/web_resour...
chrome/browser/web_resource/plugins_resource_service.cc:67:
prefs_->GetDictionary(prefs::kPluginsMetadata);
On 2013/02/08 19:25:23, Chris Evans wrote:
> Do you need to check for NULL here? Will it return NULL if we don't yet have
any
> locally downloaded metadata?

No, prefs always return a default value, which is an empty dictionary here.

[commit-bot: I haz the power, 2013-02-11 09:58:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/11348046/10019

[commit-bot: I haz the power, 2013-02-11 09:58:48]

Presubmit check for 11348046-10019 failed and returned exit status 1.

INFO:root:Found 9 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/web_resource

Presubmit checks took 1.8s to calculate.

[Bernhard Bauer, 2013-02-11 10:41:30]

Jochen: OWNERS TBR for file moves and trivial method renames
(browser_process_impl.cc).

[commit-bot: I haz the power, 2013-02-11 10:41:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/11348046/21014

[jochen (gone - plz use gerrit), 2013-02-11 10:46:46]

lgtm

[commit-bot: I haz the power, 2013-02-11 11:44:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/11348046/9026

[commit-bot: I haz the power, 2013-02-11 20:13:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/11348046/12004

[commit-bot: I haz the power, 2013-02-12 18:07:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/11348046/12004

[commit-bot: I haz the power, 2013-02-12 19:16:33]

Change committed as 181967