[2] Adding a method to PluginFinder to get a PluginInstaller by WebPluginInfo.

Adding a method to PluginFinder to get a PluginInstaller by WebPluginInfo.

It also updates plugin metadata files. 
This is the 2nd CL in the series for deleting PluginGroup and moving hardcoded plugin group definitions to .json config files.


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

[ibraaaa, 2012-08-22 14:44:13]

This is how PluginFinder's asynchronous interface should look like. Please take
a look, waiting for comments.

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

https://chromiumcodereview.appspot.com/10881002/diff/2001/chrome/browser/plug...
chrome/browser/plugin_finder.cc:171: const webkit::WebPluginInfo& plugin) {
We can use a hash_map<plugin name, PluginInstaller*> to cache found installers
here. This may make search faster. However, we would need to save a 2 copies of
each PluginInstaller. What is your take on this?

[Bernhard Bauer, 2012-08-22 15:11:30]

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

https://chromiumcodereview.appspot.com/10881002/diff/2001/chrome/browser/plug...
chrome/browser/plugin_finder.cc:171: const webkit::WebPluginInfo& plugin) {
On 2012/08/22 14:44:13, ibraaaa wrote:
> We can use a hash_map<plugin name, PluginInstaller*> to cache found installers
> here. This may make search faster. However, we would need to save a 2 copies
of
> each PluginInstaller. What is your take on this?

Hm, if the PluginInstaller is owned by |installers_| (i.e. the map using
identifier as a key), we could simply use weak pointers in the second map?

https://chromiumcodereview.appspot.com/10881002/diff/2001/chrome/browser/plug...
chrome/browser/plugin_finder.cc:202: std::string
PluginFinder::GetIdentifier(const webkit::WebPluginInfo& plugin) {
Are you planning to call these methods from other classes? If not, you could put
them into an anonymous namespace.

https://chromiumcodereview.appspot.com/10881002/diff/2001/chrome/browser/plug...
File chrome/browser/plugin_installer.h (right):

https://chromiumcodereview.appspot.com/10881002/diff/2001/chrome/browser/plug...
chrome/browser/plugin_installer.h:70: void set_group_name(const string16&
group_name) { group_name_ = group_name; }
We don't really need a separate "group name" anymore. We can just use one name
which is the predefined one where we have it, and the one from the plug-in where
we don't.

https://chromiumcodereview.appspot.com/10881002/diff/2001/chrome/browser/plug...
chrome/browser/plugin_installer.h:73: const string16& group_name_matcher() const
{ return group_name_matcher_; }
It might be nice to just have a public method that tells us whether a plug-in
name matches the group matcher. Then the substring matching that we do would be
just an implementation detail.

[ibraaaa, 2012-08-22 17:04:53]

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

https://chromiumcodereview.appspot.com/10881002/diff/2001/chrome/browser/plug...
chrome/browser/plugin_finder.cc:171: const webkit::WebPluginInfo& plugin) {
I think just making the second map have a pointer to the same object owned by
|installers_| would work fine also provided that we don't destruct both maps in
the destructor since they both point to the same thing. What do you think?

On the other hand, I liked the weak pointer thing, it was nice to read through,
knowledge++ :)

On 2012/08/22 15:11:30, Bernhard Bauer wrote:
> On 2012/08/22 14:44:13, ibraaaa wrote:
> > We can use a hash_map<plugin name, PluginInstaller*> to cache found
installers
> > here. This may make search faster. However, we would need to save a 2 copies
> of
> > each PluginInstaller. What is your take on this?
> 
> Hm, if the PluginInstaller is owned by |installers_| (i.e. the map using
> identifier as a key), we could simply use weak pointers in the second map?

[ibraaaa, 2012-08-22 18:29:25]

PTAL

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

https://chromiumcodereview.appspot.com/10881002/diff/2001/chrome/browser/plug...
chrome/browser/plugin_finder.cc:202: std::string
PluginFinder::GetIdentifier(const webkit::WebPluginInfo& plugin) {
Done, I was not planning to use them in any other class, that is why they were
declared private. I am only curious about what is the difference between
declaring the helper methods as private static vs. in an anonymous namespace? Is
it only you want to decrease the implementation info exposed in the .h file by
hiding them in the .cc file?

On 2012/08/22 15:11:30, Bernhard Bauer wrote:
> Are you planning to call these methods from other classes? If not, you could
put
> them into an anonymous namespace.

https://chromiumcodereview.appspot.com/10881002/diff/2001/chrome/browser/plug...
File chrome/browser/plugin_installer.h (right):

https://chromiumcodereview.appspot.com/10881002/diff/2001/chrome/browser/plug...
chrome/browser/plugin_installer.h:70: void set_group_name(const string16&
group_name) { group_name_ = group_name; }
That is what I am doing. If you checked how the group_name field is populated in
PluginFinder#GetPluginInstaller you will notice that the logic you mentioned
above is the one I used to populate this field. The reason for having the setter
function is because at the PluginInstaller construction time in
PluginFinder#CreateInstaller the plug-in(WebPluginInfo) is not available so I
would need to modify it later when the user asks for the group name (previously
calling GetGroupName and now calling GetPluginInstaller). The only reason for
having the field is to make PluginInstaller class complete, on the other hand we
can remove it and the caller can always deduce the group name by the plug-in
name + file path same way we do to set group_name before returning
PluginInstaller.

On 2012/08/22 15:11:30, Bernhard Bauer wrote:
> We don't really need a separate "group name" anymore. We can just use one name
> which is the predefined one where we have it, and the one from the plug-in
where
> we don't.

https://chromiumcodereview.appspot.com/10881002/diff/2001/chrome/browser/plug...
chrome/browser/plugin_installer.h:73: const string16& group_name_matcher() const
{ return group_name_matcher_; }
LIKE, I mean +1 :)

On 2012/08/22 15:11:30, Bernhard Bauer wrote:
> It might be nice to just have a public method that tells us whether a plug-in
> name matches the group matcher. Then the substring matching that we do would
be
> just an implementation detail.

[Bernhard Bauer, 2012-08-22 19:45:57]

http://codereview.chromium.org/10881002/diff/2001/chrome/browser/plugin_finde...
File chrome/browser/plugin_finder.cc (right):

http://codereview.chromium.org/10881002/diff/2001/chrome/browser/plugin_finde...
chrome/browser/plugin_finder.cc:171: const webkit::WebPluginInfo& plugin) {
On 2012/08/22 17:04:54, ibraaaa wrote:
> I think just making the second map have a pointer to the same object owned by
> |installers_| would work fine also provided that we don't destruct both maps
in
> the destructor since they both point to the same thing. What do you think?

Yes, exactly. Conceptually, we can say that |installers_| owns the
PluginInstaller objects, so we destroy those during destruction, whereas the
other map only has raw pointers, so we don't need to do anything.

> On the other hand, I liked the weak pointer thing, it was nice to read
through,
> knowledge++ :)
> 
> On 2012/08/22 15:11:30, Bernhard Bauer wrote:
> > On 2012/08/22 14:44:13, ibraaaa wrote:
> > > We can use a hash_map<plugin name, PluginInstaller*> to cache found
> installers
> > > here. This may make search faster. However, we would need to save a 2
copies
> > of
> > > each PluginInstaller. What is your take on this?
> > 
> > Hm, if the PluginInstaller is owned by |installers_| (i.e. the map using
> > identifier as a key), we could simply use weak pointers in the second map?
>

http://codereview.chromium.org/10881002/diff/2001/chrome/browser/plugin_finde...
chrome/browser/plugin_finder.cc:202: std::string
PluginFinder::GetIdentifier(const webkit::WebPluginInfo& plugin) {
On 2012/08/22 18:29:25, ibraaaa wrote:
> Done, I was not planning to use them in any other class, that is why they were
> declared private. I am only curious about what is the difference between
> declaring the helper methods as private static vs. in an anonymous namespace?
Is
> it only you want to decrease the implementation info exposed in the .h file by
> hiding them in the .cc file?

Yes, that's the only reason.

> On 2012/08/22 15:11:30, Bernhard Bauer wrote:
> > Are you planning to call these methods from other classes? If not, you could
> put
> > them into an anonymous namespace.
>

http://codereview.chromium.org/10881002/diff/2001/chrome/browser/plugin_insta...
File chrome/browser/plugin_installer.h (right):

http://codereview.chromium.org/10881002/diff/2001/chrome/browser/plugin_insta...
chrome/browser/plugin_installer.h:70: void set_group_name(const string16&
group_name) { group_name_ = group_name; }
On 2012/08/22 18:29:25, ibraaaa wrote:
> That is what I am doing. If you checked how the group_name field is populated
in
> PluginFinder#GetPluginInstaller you will notice that the logic you mentioned
> above is the one I used to populate this field. The reason for having the
setter
> function is because at the PluginInstaller construction time in
> PluginFinder#CreateInstaller the plug-in(WebPluginInfo) is not available so I
> would need to modify it later when the user asks for the group name
(previously
> calling GetGroupName and now calling GetPluginInstaller).

What I meant was that there is no reason to have both "group name" and "plug-in
name". When we call PluginFinder::CreateInstaller, we either have the plug-in
name from the metadata, or the deduced plug-in name from the WebPluginInfo.

> The only reason for
> having the field is to make PluginInstaller class complete, on the other hand
we
> can remove it and the caller can always deduce the group name by the plug-in
> name + file path same way we do to set group_name before returning
> PluginInstaller.

Yes, except that PluginFinder can do that.

> 
> On 2012/08/22 15:11:30, Bernhard Bauer wrote:
> > We don't really need a separate "group name" anymore. We can just use one
name
> > which is the predefined one where we have it, and the one from the plug-in
> where
> > we don't.
>

[ibraaaa, 2012-08-23 08:35:55]

https://chromiumcodereview.appspot.com/10881002/diff/2001/chrome/browser/plug...
File chrome/browser/plugin_installer.h (right):

https://chromiumcodereview.appspot.com/10881002/diff/2001/chrome/browser/plug...
chrome/browser/plugin_installer.h:70: void set_group_name(const string16&
group_name) { group_name_ = group_name; }
On 2012/08/22 19:45:57, Bernhard Bauer wrote:
> On 2012/08/22 18:29:25, ibraaaa wrote:
> > That is what I am doing. If you checked how the group_name field is
populated
> in
> > PluginFinder#GetPluginInstaller you will notice that the logic you mentioned
> > above is the one I used to populate this field. The reason for having the
> setter
> > function is because at the PluginInstaller construction time in
> > PluginFinder#CreateInstaller the plug-in(WebPluginInfo) is not available so
I
> > would need to modify it later when the user asks for the group name
> (previously
> > calling GetGroupName and now calling GetPluginInstaller).
> 
> What I meant was that there is no reason to have both "group name" and
"plug-in
> name". When we call PluginFinder::CreateInstaller, we either have the plug-in
> name from the metadata, or the deduced plug-in name from the WebPluginInfo.
> 
> > The only reason for
> > having the field is to make PluginInstaller class complete, on the other
hand
> we
> > can remove it and the caller can always deduce the group name by the plug-in
> > name + file path same way we do to set group_name before returning
> > PluginInstaller.
> 
> Yes, except that PluginFinder can do that.
> 
> > 
> > On 2012/08/22 15:11:30, Bernhard Bauer wrote:
> > > We don't really need a separate "group name" anymore. We can just use one
> name
> > > which is the predefined one where we have it, and the one from the plug-in
> > where
> > > we don't.
> > 
> 

Done.

[Bernhard Bauer, 2012-08-23 09:05:07]

LGTM with nits:

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

https://chromiumcodereview.appspot.com/10881002/diff/4010/chrome/browser/plug...
chrome/browser/plugin_finder.cc:203: plugin_it.HasNext(); plugin_it.Advance()) {
Nit: I think aligning this line with `DictionaryValue...` looks a bit nicer.

https://chromiumcodereview.appspot.com/10881002/diff/4010/chrome/browser/plug...
File chrome/browser/plugin_finder.h (right):

https://chromiumcodereview.appspot.com/10881002/diff/4010/chrome/browser/plug...
chrome/browser/plugin_finder.h:16: #include "base/memory/weak_ptr.h"
Nit: Unnecessary #include.

https://chromiumcodereview.appspot.com/10881002/diff/4010/chrome/browser/plug...
File chrome/browser/plugin_installer.h (right):

https://chromiumcodereview.appspot.com/10881002/diff/4010/chrome/browser/plug...
chrome/browser/plugin_installer.h:67: bool IsPluginMatchingGroupMatcher(const
webkit::WebPluginInfo& plugin);
Nit: Just "MatchesPlugin()"?

[ibraaaa, 2012-08-23 09:58:32]

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

https://chromiumcodereview.appspot.com/10881002/diff/4010/chrome/browser/plug...
chrome/browser/plugin_finder.cc:203: plugin_it.HasNext(); plugin_it.Advance()) {
On 2012/08/23 09:05:07, Bernhard Bauer wrote:
> Nit: I think aligning this line with `DictionaryValue...` looks a bit nicer.

Done.

https://chromiumcodereview.appspot.com/10881002/diff/4010/chrome/browser/plug...
File chrome/browser/plugin_finder.h (right):

https://chromiumcodereview.appspot.com/10881002/diff/4010/chrome/browser/plug...
chrome/browser/plugin_finder.h:16: #include "base/memory/weak_ptr.h"
On 2012/08/23 09:05:07, Bernhard Bauer wrote:
> Nit: Unnecessary #include.

Done.

https://chromiumcodereview.appspot.com/10881002/diff/4010/chrome/browser/plug...
File chrome/browser/plugin_installer.h (right):

https://chromiumcodereview.appspot.com/10881002/diff/4010/chrome/browser/plug...
chrome/browser/plugin_installer.h:67: bool IsPluginMatchingGroupMatcher(const
webkit::WebPluginInfo& plugin);
On 2012/08/23 09:05:07, Bernhard Bauer wrote:
> Nit: Just "MatchesPlugin()"?

Done.

[commit-bot: I haz the power, 2012-08-23 09:58:41]

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

[commit-bot: I haz the power, 2012-08-23 09:58:44]

Failed to apply patch for
chrome/browser/resources/plugin_metadata/plugins_linux.json:
While running patch -p1 --forward --force;
patching file chrome/browser/resources/plugin_metadata/plugins_linux.json
Hunk #3 FAILED at 85.
Hunk #4 FAILED at 127.
2 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/resources/plugin_metadata/plugins_linux.json.rej

[commit-bot: I haz the power, 2012-08-23 10:01:49]

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

[commit-bot: I haz the power, 2012-08-23 10:01:55]

Failed to apply patch for
chrome/browser/resources/plugin_metadata/plugins_linux.json:
While running patch -p1 --forward --force;
patching file chrome/browser/resources/plugin_metadata/plugins_linux.json
Hunk #3 FAILED at 85.
Hunk #4 FAILED at 127.
2 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/resources/plugin_metadata/plugins_linux.json.rej

[commit-bot: I haz the power, 2012-08-23 10:23:44]

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

[commit-bot: I haz the power, 2012-08-23 10:23:50]

Failed to apply patch for
chrome/browser/resources/plugin_metadata/plugins_linux.json:
While running patch -p1 --forward --force;
patching file chrome/browser/resources/plugin_metadata/plugins_linux.json
Hunk #3 FAILED at 85.
Hunk #4 FAILED at 127.
2 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/resources/plugin_metadata/plugins_linux.json.rej

[commit-bot: I haz the power, 2012-08-23 11:42:50]

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

[commit-bot: I haz the power, 2012-08-23 11:42:58]

Presubmit check for 10881002-8003 failed and returned exit status 1.


Running presubmit commit checks ...

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

Presubmit checks took 4.5s to calculate.

[ibraaaa, 2012-08-23 11:45:00]

There was merge conflicts in the .json files, now I fixed them. However,
commit bot is asking for LGTM?

On Thu, Aug 23, 2012 at 1:42 PM, <commit-bot@chromium.org> wrote:

>
> Presubmit check for 10881002-8003 failed and returned exit status 1.
>
>
> Running presubmit commit checks ...
>
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome
>
> Presubmit checks took 4.5s to calculate.
>
>
>
>
https://chromiumcodereview.**appspot.com/10881002/<https://chromiumcodereview...
>



-- 

**
Mohamed Hammad | Software Engineer Intern. | ibraaaa@google.com | +2 (0)
100 682 4347
Google Germany GmbH
Alter Hof Lorenzistock
Dienerstrasse 12
80331 Muenchen

[ibraaaa, 2012-08-24 09:01:43]

Hi,

Can I get an owners review for this CL?

Thanks

[jam, 2012-08-27 17:20:53]

please see chrome/OWNERS

[ibraaaa, 2012-08-27 17:30:09]

Hi,

Can I get an Owners LGTM for this CL?

[James Hawkins, 2012-08-27 17:38:01]

On 2012/08/27 17:30:09, ibraaaa wrote:
> Hi,
> 
> Can I get an Owners LGTM for this CL?

Please pick one owner from the OWNERS file instead of spamming everyone in the
file.

[ibraaaa, 2012-08-27 17:45:51]

Oops, sorry for the "spamming", new here :). Thanks for the tip, can I have
the OWNERs review? please.

On Mon, Aug 27, 2012 at 7:38 PM, <jhawkins@chromium.org> wrote:

>
> On 2012/08/27 17:30:09, ibraaaa wrote:
>
>> Hi,
>>
>
>  Can I get an Owners LGTM for this CL?
>>
>
> Please pick one owner from the OWNERS file instead of spamming everyone in
> the
> file.
>
>
http://codereview.chromium.**org/10881002/<http://codereview.chromium.org/108...
>



-- 

**
Mohamed Hammad | Software Engineer Intern. | ibraaaa@google.com | +2 (0)
100 682 4347
Google Germany GmbH
Alter Hof Lorenzistock
Dienerstrasse 12
80331 Muenchen

[James Hawkins, 2012-08-27 17:49:43]

LGTM with nits.

http://codereview.chromium.org/10881002/diff/17004/chrome/browser/plugin_find...
File chrome/browser/plugin_finder.h (right):

http://codereview.chromium.org/10881002/diff/17004/chrome/browser/plugin_find...
chrome/browser/plugin_finder.h:39: // Gets a plug-in installer using web plug-in
info.
s/web plugin-in info/|plugin|/

http://codereview.chromium.org/10881002/diff/17004/chrome/browser/plugin_find...
chrome/browser/plugin_finder.h:62: // Note: Don't free memory for its values
since it holds pointers
nit: Blank line aboe this.

http://codereview.chromium.org/10881002/diff/17004/chrome/browser/plugin_find...
chrome/browser/plugin_finder.h:62: // Note: Don't free memory for its values
since it holds pointers
nit: Try to not use pronouns (it) because they (pronouns) are ambiguous.

[commit-bot: I haz the power, 2012-08-27 18:00:24]

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

[commit-bot: I haz the power, 2012-08-28 08:43:45]

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

[commit-bot: I haz the power, 2012-08-28 10:49:09]

Try job failure for 10881002-10021 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-28 11:23:27]

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

[commit-bot: I haz the power, 2012-08-28 13:42:51]

Change committed as 153659

[ibraaaa, 2012-08-28 15:01:45]

http://codereview.chromium.org/10881002/diff/17004/chrome/browser/plugin_find...
File chrome/browser/plugin_finder.h (right):

http://codereview.chromium.org/10881002/diff/17004/chrome/browser/plugin_find...
chrome/browser/plugin_finder.h:39: // Gets a plug-in installer using web plug-in
info.
On 2012/08/27 17:49:43, James Hawkins wrote:
> s/web plugin-in info/|plugin|/

Done.

http://codereview.chromium.org/10881002/diff/17004/chrome/browser/plugin_find...
chrome/browser/plugin_finder.h:62: // Note: Don't free memory for its values
since it holds pointers
On 2012/08/27 17:49:43, James Hawkins wrote:
> nit: Blank line aboe this.

Done.

http://codereview.chromium.org/10881002/diff/17004/chrome/browser/plugin_find...
chrome/browser/plugin_finder.h:62: // Note: Don't free memory for its values
since it holds pointers
On 2012/08/27 17:49:43, James Hawkins wrote:
> nit: Blank line aboe this.

Done.