[6] Moves CreateVersionFromString to plugin_utils and updates the callers.

Moves CreateVersionFromString to WebPluginInfo and updates the callers.

6th CL in the series to delete PluginGroup and move Hardcoded plugin group definitions to metadata files.

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

[Bernhard Bauer, 2012-08-21 12:30:18]

Please see the comment at
https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
first; the rest is mostly nits.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
File chrome/browser/plugin_finder.cc (right):

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_finder.cc:169: const std::map<std::string,
PluginInstaller*>& PluginFinder::
Ugh. I would either break this line after the return type (with the next line
not indented), or just extract the return type to a typedef.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_finder.cc:173: FindPluginWithIdentifier(plugin_it.key());
Can you add a comment that this triggers the lazy initialization of all the
PluginInstallers?

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
File chrome/browser/plugin_finder.h (right):

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_finder.h:37: // Returns  a map of all data loaded from:
Nit: one space too many.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_finder.h:38: //
'chrome/browser/resources/plugin_metadata/plugins_*.json' files.
I think we can just refer to the plugin metadata files here. Otherwise the
comment will inevitably become stale at some point ;-)

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_finder.h:46: friend class PluginInstaller;
Nit: Move before Singleton to keep this alphabetized.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
File chrome/browser/plugin_installer.cc (right):

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_installer.cc:258: /*static*/
Nit: The rest of the file uses // static.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_installer.cc:261: if (plugin_installer != NULL) {
Nit: Braces aren't required for single-line statements.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_installer.cc:297:
PluginFinder::GetInstance()->GetAllPluginInstallers();
Ok, this is where stuff gets complicated.

Right now, we bundle the plug-in metadata files as resources which are mapped
into memory, so they can be accessed from any thread. Eventually though, we want
to move them to physical files on disk though that can be updated independently.
That means that we can't load them from disk directly, because that would mean
having to wait on disk I/O, which we are only allowed to do on specific threads.
That is why I started out with an asynchronous interface to PluginFinder (there
is no public method on PluginFinder that synchronously returns the instance).
So, we should probably move this to PluginFinder and make callers use the
asynchronous interface.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/resourc...
File chrome/browser/resources/plugin_metadata/plugins_win.json (right):

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/resourc...
chrome/browser/resources/plugin_metadata/plugins_win.json:288: "status":
"up_to_date",
Can you change this to "requires_authorization"? I think we want to infobar
these (if they weren't before, that most likely a bug).

[ibraaaa, 2012-08-21 14:26:04]

Please, most importantly check my reply here:
https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
File chrome/browser/plugin_finder.cc (right):

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_finder.cc:169: const std::map<std::string,
PluginInstaller*>& PluginFinder::
On 2012/08/21 12:30:18, Bernhard Bauer wrote:
> Ugh. I would either break this line after the return type (with the next line
> not indented), or just extract the return type to a typedef.

Done.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_finder.cc:173: FindPluginWithIdentifier(plugin_it.key());
On 2012/08/21 12:30:18, Bernhard Bauer wrote:
> Can you add a comment that this triggers the lazy initialization of all the
> PluginInstallers?

Done.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
File chrome/browser/plugin_finder.h (right):

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_finder.h:37: // Returns  a map of all data loaded from:
On 2012/08/21 12:30:18, Bernhard Bauer wrote:
> Nit: one space too many.

Done.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_finder.h:38: //
'chrome/browser/resources/plugin_metadata/plugins_*.json' files.
On 2012/08/21 12:30:18, Bernhard Bauer wrote:
> I think we can just refer to the plugin metadata files here. Otherwise the
> comment will inevitably become stale at some point ;-)

Done.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_finder.h:46: friend class PluginInstaller;
On 2012/08/21 12:30:18, Bernhard Bauer wrote:
> Nit: Move before Singleton to keep this alphabetized.

Done.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
File chrome/browser/plugin_installer.cc (right):

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_installer.cc:258: /*static*/
On 2012/08/21 12:30:18, Bernhard Bauer wrote:
> Nit: The rest of the file uses // static.

Done.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_installer.cc:261: if (plugin_installer != NULL) {
On 2012/08/21 12:30:18, Bernhard Bauer wrote:
> Nit: Braces aren't required for single-line statements.

Done.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_installer.cc:297:
PluginFinder::GetInstance()->GetAllPluginInstallers();
I want to confirm something, places where I need a list of all plugins are in
PluginInstaller::GetIdentifier and PluginInstaller::GetGroupName, they basically
need to search in a list of all plugins and do the name matching thing before
they return. I may be missing something here, but the asynchronous interface
won't guarantee that all plugins are loaded before GetIdentifier/GetGroupName
return so the result may not be correct. In other words,
GetIdentifier/GetGroupName should not return a result until the asynchronous
call is 100% finished.

I hope we are on the same page, if not, please correct me.

On 2012/08/21 12:30:18, Bernhard Bauer wrote:
> Ok, this is where stuff gets complicated.
> 
> Right now, we bundle the plug-in metadata files as resources which are mapped
> into memory, so they can be accessed from any thread. Eventually though, we
want
> to move them to physical files on disk though that can be updated
independently.
> That means that we can't load them from disk directly, because that would mean
> having to wait on disk I/O, which we are only allowed to do on specific
threads.
> That is why I started out with an asynchronous interface to PluginFinder
(there
> is no public method on PluginFinder that synchronously returns the instance).
> So, we should probably move this to PluginFinder and make callers use the
> asynchronous interface.

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/resourc...
File chrome/browser/resources/plugin_metadata/plugins_win.json (right):

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/resourc...
chrome/browser/resources/plugin_metadata/plugins_win.json:288: "status":
"up_to_date",
On 2012/08/21 12:30:18, Bernhard Bauer wrote:
> Can you change this to "requires_authorization"? I think we want to infobar
> these (if they weren't before, that most likely a bug).

Done.

[ibraaaa, 2012-08-21 17:27:12]

I modified code according to comments in patch set 1 in patch set 2.

In this patch set (3), I added more files to this CL, they all incorporate easy
changes which call the methods in PluginInstaller instead of using PluginService
& PluginList.

I also modified PluginInstaller by added one more method:
GetGroupNameByIdentifier

The added files are:

chrome/browser/renderer_host/plugin_info_message_filter.cc 
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm
chrome/browser/ui/gtk/content_setting_bubble_gtk.cc
chrome/browser/ui/views/content_setting_bubble_contents.cc
chrome/renderer/chrome_content_renderer_client.cc

https://chromiumcodereview.appspot.com/10823434/diff/7002/chrome/renderer/chr...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/10823434/diff/7002/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:17: #include
"chrome/browser/plugin_installer.h"
Presubmit checks won't pass because of this include, there is a '-chrome' in
include rules, do we remove it?

[Bernhard Bauer, 2012-08-21 19:14:52]

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
File chrome/browser/plugin_installer.cc (right):

https://chromiumcodereview.appspot.com/10823434/diff/1/chrome/browser/plugin_...
chrome/browser/plugin_installer.cc:297:
PluginFinder::GetInstance()->GetAllPluginInstallers();
On 2012/08/21 14:26:04, ibraaaa wrote:
> I want to confirm something, places where I need a list of all plugins are in
> PluginInstaller::GetIdentifier and PluginInstaller::GetGroupName, they
basically
> need to search in a list of all plugins and do the name matching thing before
> they return. I may be missing something here, but the asynchronous interface
> won't guarantee that all plugins are loaded before GetIdentifier/GetGroupName
> return so the result may not be correct. In other words,
> GetIdentifier/GetGroupName should not return a result until the asynchronous
> call is 100% finished.
> 
> I hope we are on the same page, if not, please correct me.

If by that you mean that we're going to need to make callers use an asynchronous
interface as well, then we're on the same page ;-) Blocking until the call to
GetIdentifier/GetGroupName has finished would defeat the purpose of making that
asynchronous in the first place.

The easiest way to achieve that would be to make these methods synchronous but
move them to PluginFinder (non-static!), so clients would have one asynchronous
call to get the current PluginFinder, and could afterward use a synchronous
interface.

***

Another thing we could do to keep the interface simple would be to have
PluginFinder always return a PluginInstaller. We add one method to PluginFinder
that finds the corresponding PluginInstaller for a given WebPluginInfo
(returning one from the metadata if available, or creating a dummy one
otherwise), then clients could ask that one for its name or identifier. WDYT?

https://chromiumcodereview.appspot.com/10823434/diff/7002/chrome/browser/plug...
File chrome/browser/plugin_finder.cc (right):

https://chromiumcodereview.appspot.com/10823434/diff/7002/chrome/browser/plug...
chrome/browser/plugin_finder.cc:169: // Note: this method triggers the lazy
initialization for all PluginInstallers.
It doesn't matter very much, but what I meant originally was that it's not
obvious why we're calling a method called FindXYZ if we're ignoring the return
value, so I wanted to add the comment right above the method call.

https://chromiumcodereview.appspot.com/10823434/diff/7002/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm
(right):

https://chromiumcodereview.appspot.com/10823434/diff/7002/chrome/browser/ui/c...
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:244:
PluginService::GetInstance()->GetPluginList()->
This method is deprecated in favor of using the asynchronous interface on
PluginService as well, but I think if you're going to use PluginFinder, you
should get by without this one.

https://chromiumcodereview.appspot.com/10823434/diff/7002/chrome/renderer/chr...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/10823434/diff/7002/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:17: #include
"chrome/browser/plugin_installer.h"
On 2012/08/21 17:27:12, ibraaaa wrote:
> Presubmit checks won't pass because of this include, there is a '-chrome' in
> include rules, do we remove it?

The issue isn't chrome, it's that you're depending on chrome/browser, which is
code that's only supposed to run in the browser process, but this is the
renderer process. 

To get the information from the browser process to the renderer process, you're
going to have to add an IPC message; see ChromeViewHostMsg_Foo for examples.

[Bernhard Bauer, 2012-08-29 16:20:15]

LGTM

[commit-bot: I haz the power, 2012-08-29 17:47:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@google.com/10823434/13001

[commit-bot: I haz the power, 2012-08-29 17:47:44]

Presubmit check for 10823434-13001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    content/utility
    webkit/plugins
    content/browser

Presubmit checks took 1.2s to calculate.

[ibraaaa, 2012-08-29 18:36:12]

Hi,

Can I have OWNERS LGTM for this CL? I need one from each of you hopefully :)

[jam, 2012-08-29 22:26:13]

https://codereview.chromium.org/10823434/diff/13001/webkit/plugins/webplugini...
File webkit/plugins/webplugininfo.h (right):

https://codereview.chromium.org/10823434/diff/13001/webkit/plugins/webplugini...
webkit/plugins/webplugininfo.h:63: static void CreateVersionFromString(const
string16& version_string,
a struct shouldn't have member functions per the style guide. it also doesn't
seem to belong on WebPluginInfo.

[ibraaaa, 2012-08-30 08:08:47]

https://codereview.chromium.org/10823434/diff/13001/webkit/plugins/webplugini...
File webkit/plugins/webplugininfo.h (right):

https://codereview.chromium.org/10823434/diff/13001/webkit/plugins/webplugini...
webkit/plugins/webplugininfo.h:63: static void CreateVersionFromString(const
string16& version_string,
It used to be in PluginGroup but we are doing this modification to remove
PluginGroup. I see your point, what do you suggest?

On 2012/08/29 22:26:13, John Abd-El-Malek wrote:
> a struct shouldn't have member functions per the style guide. it also doesn't
> seem to belong on WebPluginInfo.

[jam, 2012-08-30 18:37:06]

On 2012/08/30 08:08:47, ibraaaa wrote:
>
https://codereview.chromium.org/10823434/diff/13001/webkit/plugins/webplugini...
> File webkit/plugins/webplugininfo.h (right):
> 
>
https://codereview.chromium.org/10823434/diff/13001/webkit/plugins/webplugini...
> webkit/plugins/webplugininfo.h:63: static void CreateVersionFromString(const
> string16& version_string,
> It used to be in PluginGroup but we are doing this modification to remove
> PluginGroup. I see your point, what do you suggest?
> 
> On 2012/08/29 22:26:13, John Abd-El-Malek wrote:
> > a struct shouldn't have member functions per the style guide. it also
doesn't
> > seem to belong on WebPluginInfo.

perhaps add it as a static method to PluginList? or a new file in
webkit/plugins/npapi, mayb plugin_utils.h?

also, shouldn't this change be removing the old version in PluginGroup?

[ibraaaa, 2012-08-31 19:19:00]

PTAL

[jam, 2012-09-04 16:46:40]

https://codereview.chromium.org/10823434/diff/23001/webkit/plugins/npapi/plug...
File webkit/plugins/npapi/plugin_utils.h (right):

https://codereview.chromium.org/10823434/diff/23001/webkit/plugins/npapi/plug...
webkit/plugins/npapi/plugin_utils.h:16: WEBKIT_PLUGINS_EXPORT void
CreateVersionFromString(
nit: bring over the comment as well

https://codereview.chromium.org/10823434/diff/23001/webkit/plugins/npapi/plug...
webkit/plugins/npapi/plugin_utils.h:23: 
nit: just need one blank line, not 3

[ibraaaa, 2012-09-04 17:00:34]

PTAL, thanks.

https://codereview.chromium.org/10823434/diff/23001/webkit/plugins/npapi/plug...
File webkit/plugins/npapi/plugin_utils.h (right):

https://codereview.chromium.org/10823434/diff/23001/webkit/plugins/npapi/plug...
webkit/plugins/npapi/plugin_utils.h:16: WEBKIT_PLUGINS_EXPORT void
CreateVersionFromString(
On 2012/09/04 16:46:40, John Abd-El-Malek wrote:
> nit: bring over the comment as well

Done.

https://codereview.chromium.org/10823434/diff/23001/webkit/plugins/npapi/plug...
webkit/plugins/npapi/plugin_utils.h:23: 
On 2012/09/04 16:46:40, John Abd-El-Malek wrote:
> nit: just need one blank line, not 3

Done.

[jam, 2012-09-04 19:59:54]

lgtm

[commit-bot: I haz the power, 2012-09-04 20:01:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@google.com/10823434/28001

[commit-bot: I haz the power, 2012-09-04 20:01:38]

Presubmit check for 10823434-28001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit

Presubmit checks took 1.9s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[ibraaaa, 2012-09-04 20:08:34]

Hi James,

Can I have an OWNERS review for this CL?

Thanks

[jamesr, 2012-09-04 20:14:43]

webkit/ lgtm

[commit-bot: I haz the power, 2012-09-04 20:15:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@google.com/10823434/28001

[commit-bot: I haz the power, 2012-09-04 20:42:44]

Try job failure for 10823434-28001 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-09-04 20:44:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@google.com/10823434/22011

[commit-bot: I haz the power, 2012-09-04 23:08:59]

Change committed as 154842