Using MIME types in addition to plugin name to differentiate between plugins.

Using MIME types in addition to plugin name to differentiate between plugins.


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

[Bernhard Bauer, 2012-10-02 14:40:13]

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_finder.cc (right):

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_finder.cc:73: if (mime_type_list_key ==
kMatchingMimeTypesListKey) {
This might be a bit simpler if you create a method that parses a ListValue of
StringValues and writes the strings to a std::vector<std::string>. Then you
fetch the list from the dictionary below and add the MIME types directly.

Alternatively, you could pass in a boolean that tells whether to look at the
list of MIME types for installing or for matching.

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_finder.cc:203: std::map<std::string,
PluginMetadata*>::const_iterator metadata_it =
You use this map type in a bunch of places. You could typedef it to make it a
bit shorter.

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_finder.cc:261: name_plugin_[matching_name] =
it->second;
You're never reading from |name_plugin_| anymore.

It seems that now we can't find a plug-in based on the name alone, so the cache
might not make sense anymore.

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_metadata.cc (right):

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_metadata.cc:82: std::bind2nd(mime_types_cmp,
matching_mime_types_[i]));
This seems… complicated. Is there no easier way to look for a string in a vector
of strings?

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_metadata.cc:84: return false;
It might be good to explain the logic here (i.e. every one of the
|matching_mime_types_| must be handled by the plug-in for it to match).

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_metadata.h (right):

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_metadata.h:61: const GURL& help_url() const {
return help_url_; }
Nit: newline

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/resources/pl...
File chrome/browser/resources/plugin_metadata/plugins_linux.json (right):

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/resources/pl...
chrome/browser/resources/plugin_metadata/plugins_linux.json:97:
"application/x-shockwave-flash"
This now depends on the fact that "lightspark" comes after "adobe-flash"
lexicographically, otherwise we would offer Lightspark when the user comes
across a Flash page. That seems a bit fragile to me. We could probably leave out
the "application/x-shockwave-flash" MIME type.

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/resources/pl...
chrome/browser/resources/plugin_metadata/plugins_linux.json:104: "version":
"12.1",
Is 12.1 the most recent version?

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/resources/pl...
chrome/browser/resources/plugin_metadata/plugins_linux.json:109: "name": "Light
Spark",
Nit: It's "Lightspark", I think.

[ibraaaa, 2012-10-02 16:14:53]

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_finder.cc (right):

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_finder.cc:73: if (mime_type_list_key ==
kMatchingMimeTypesListKey) {
Well, if I send a boolean value (actually that is what I was doing before this)
it will be doing the same thing as |mime_type_list_key| since I need this key
anyway to fetch the correct list from the dictionary, WDYT?

On 2012/10/02 14:40:13, Bernhard Bauer wrote:
> This might be a bit simpler if you create a method that parses a ListValue of
> StringValues and writes the strings to a std::vector<std::string>. Then you
> fetch the list from the dictionary below and add the MIME types directly.
> 
> Alternatively, you could pass in a boolean that tells whether to look at the
> list of MIME types for installing or for matching.

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_finder.cc:203: std::map<std::string,
PluginMetadata*>::const_iterator metadata_it =
On 2012/10/02 14:40:13, Bernhard Bauer wrote:
> You use this map type in a bunch of places. You could typedef it to make it a
> bit shorter. 

Done.

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_finder.cc:261: name_plugin_[matching_name] =
it->second;
On 2012/10/02 14:40:13, Bernhard Bauer wrote:
> You're never reading from |name_plugin_| anymore.
> 
> It seems that now we can't find a plug-in based on the name alone, so the
cache
> might not make sense anymore.

Done.

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_metadata.cc (right):

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_metadata.cc:82: std::bind2nd(mime_types_cmp,
matching_mime_types_[i]));
Actually I am looking for a string in a vector of WebPluginMimeType? If you
favor doing it the way done here:
http://codereview.chromium.org/11016005/diff/2001/chrome/browser/plugins/plug...
I can revert to it?

On 2012/10/02 14:40:13, Bernhard Bauer wrote:
> This seems… complicated. Is there no easier way to look for a string in a
vector
> of strings?

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_metadata.cc:84: return false;
On 2012/10/02 14:40:13, Bernhard Bauer wrote:
> It might be good to explain the logic here (i.e. every one of the
> |matching_mime_types_| must be handled by the plug-in for it to match).

Done.

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_metadata.h (right):

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_metadata.h:61: const GURL& help_url() const {
return help_url_; }
On 2012/10/02 14:40:13, Bernhard Bauer wrote:
> Nit: newline

Done.

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/resources/pl...
File chrome/browser/resources/plugin_metadata/plugins_linux.json (right):

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/resources/pl...
chrome/browser/resources/plugin_metadata/plugins_linux.json:97:
"application/x-shockwave-flash"
On 2012/10/02 14:40:13, Bernhard Bauer wrote:
> This now depends on the fact that "lightspark" comes after "adobe-flash"
> lexicographically, otherwise we would offer Lightspark when the user comes
> across a Flash page. That seems a bit fragile to me. We could probably leave
out
> the "application/x-shockwave-flash" MIME type.

Done.

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/resources/pl...
chrome/browser/resources/plugin_metadata/plugins_linux.json:109: "name": "Light
Spark",
On 2012/10/02 14:40:13, Bernhard Bauer wrote:
> Nit: It's "Lightspark", I think.

Done.

[ibraaaa, 2012-10-04 11:15:55]

Infobared Lightspark

[Bernhard Bauer, 2012-10-04 11:37:19]

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_finder.cc (right):

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_finder.cc:73: if (mime_type_list_key ==
kMatchingMimeTypesListKey) {
On 2012/10/02 16:14:53, ibraaaa wrote:
> Well, if I send a boolean value (actually that is what I was doing before
this)
> it will be doing the same thing as |mime_type_list_key| since I need this key
> anyway to fetch the correct list from the dictionary, WDYT?

Right, it just seems a bit more awkward to pass in a string that can only have
two values instead of a boolean.

> On 2012/10/02 14:40:13, Bernhard Bauer wrote:
> > This might be a bit simpler if you create a method that parses a ListValue
of
> > StringValues and writes the strings to a std::vector<std::string>. Then you
> > fetch the list from the dictionary below and add the MIME types directly.
> > 
> > Alternatively, you could pass in a boolean that tells whether to look at the
> > list of MIME types for installing or for matching.
>

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_metadata.cc (right):

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_metadata.cc:82: std::bind2nd(mime_types_cmp,
matching_mime_types_[i]));
On 2012/10/02 16:14:53, ibraaaa wrote:
> Actually I am looking for a string in a vector of WebPluginMimeType? If you
> favor doing it the way done here:
>
http://codereview.chromium.org/11016005/diff/2001/chrome/browser/plugins/plug...
> I can revert to it?

Hm, maybe we could simply make PluginList::SupportsType public?

> On 2012/10/02 14:40:13, Bernhard Bauer wrote:
> > This seems… complicated. Is there no easier way to look for a string in a
> vector
> > of strings?
>

http://codereview.chromium.org/11016005/diff/22004/chrome/browser/plugins/plu...
File chrome/browser/plugins/plugin_finder.cc (right):

http://codereview.chromium.org/11016005/diff/22004/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_finder.cc:268: string16 matching_name;
Ok, now you're not using |matching_name| anymore.

http://codereview.chromium.org/11016005/diff/22004/chrome/browser/plugins/plu...
File chrome/browser/plugins/plugin_metadata.cc (right):

http://codereview.chromium.org/11016005/diff/22004/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_metadata.cc:84: // must be handled by the plug-in
Nit: end the sentence with a period please.

[ibraaaa, 2012-10-04 12:24:12]

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_finder.cc (right):

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_finder.cc:73: if (mime_type_list_key ==
kMatchingMimeTypesListKey) {
On 2012/10/04 11:37:19, Bernhard Bauer wrote:
> On 2012/10/02 16:14:53, ibraaaa wrote:
> > Well, if I send a boolean value (actually that is what I was doing before
> this)
> > it will be doing the same thing as |mime_type_list_key| since I need this
key
> > anyway to fetch the correct list from the dictionary, WDYT?
> 
> Right, it just seems a bit more awkward to pass in a string that can only have
> two values instead of a boolean.
> 
> > On 2012/10/02 14:40:13, Bernhard Bauer wrote:
> > > This might be a bit simpler if you create a method that parses a ListValue
> of
> > > StringValues and writes the strings to a std::vector<std::string>. Then
you
> > > fetch the list from the dictionary below and add the MIME types directly.
> > > 
> > > Alternatively, you could pass in a boolean that tells whether to look at
the
> > > list of MIME types for installing or for matching.
> > 
> 

Done.

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_metadata.cc (right):

http://codereview.chromium.org/11016005/diff/9007/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_metadata.cc:82: std::bind2nd(mime_types_cmp,
matching_mime_types_[i]));
Looks much better now :)
On 2012/10/04 11:37:19, Bernhard Bauer wrote:
> On 2012/10/02 16:14:53, ibraaaa wrote:
> > Actually I am looking for a string in a vector of WebPluginMimeType? If you
> > favor doing it the way done here:
> >
>
http://codereview.chromium.org/11016005/diff/2001/chrome/browser/plugins/plug...
> > I can revert to it?
> 
> Hm, maybe we could simply make PluginList::SupportsType public?
> 
> > On 2012/10/02 14:40:13, Bernhard Bauer wrote:
> > > This seems… complicated. Is there no easier way to look for a string in a
> > vector
> > > of strings?
> > 
>

http://codereview.chromium.org/11016005/diff/22004/chrome/browser/plugins/plu...
File chrome/browser/plugins/plugin_finder.cc (right):

http://codereview.chromium.org/11016005/diff/22004/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_finder.cc:268: string16 matching_name;
On 2012/10/04 11:37:19, Bernhard Bauer wrote:
> Ok, now you're not using |matching_name| anymore.

Done.

http://codereview.chromium.org/11016005/diff/22004/chrome/browser/plugins/plu...
File chrome/browser/plugins/plugin_metadata.cc (right):

http://codereview.chromium.org/11016005/diff/22004/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_metadata.cc:84: // must be handled by the plug-in
On 2012/10/04 11:37:19, Bernhard Bauer wrote:
> Nit: end the sentence with a period please.

Done.

[Bernhard Bauer, 2012-10-04 12:31:16]

http://codereview.chromium.org/11016005/diff/9009/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_finder.cc (right):

http://codereview.chromium.org/11016005/diff/9009/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_finder.cc:62: void LoadMimeTypes(const
std::string& mime_type_list_key,
You don't really need to explicitly pass in the key here, do you? You could get
the correct key from the |matching_mime_types| flag.

[ibraaaa, 2012-10-04 13:03:42]

http://codereview.chromium.org/11016005/diff/9009/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_finder.cc (right):

http://codereview.chromium.org/11016005/diff/9009/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_finder.cc:62: void LoadMimeTypes(const
std::string& mime_type_list_key,
Well, that is what I thought but your last reply to my comment implied something
else (or I didn't understand it). Done.

Removed the constants for consistency, there are no constants for other keys.

On 2012/10/04 12:31:16, Bernhard Bauer wrote:
> You don't really need to explicitly pass in the key here, do you? You could
get
> the correct key from the |matching_mime_types| flag.

[Bernhard Bauer, 2012-10-04 13:21:33]

LGTM (but you're going to need to update the call sites to make it compile).

[commit-bot: I haz the power, 2012-10-04 16:07:00]

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

[commit-bot: I haz the power, 2012-10-04 18:55:11]

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

[commit-bot: I haz the power, 2012-10-04 19:49:12]

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

[ibraaaa, 2012-10-05 18:05:10]

Bernhard, I updated the test to match the changes in the metadata files. Please
have a look.

[Bernhard Bauer, 2012-10-05 18:23:13]

lgtm

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

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

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

Retried try job too often for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-10-08 11:45:40]

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

[commit-bot: I haz the power, 2012-10-08 15:02:22]

Change committed as 160653