Use a placeholder instead of the default plugin for missing plug-ins on Mac and Linux.

Use a placeholder instead of the default plugin for missing plug-ins on Mac and Linux.


BUG=62079
TEST=Missing plug-in placeholder looks different


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

[Bernhard Bauer, 2011-09-27 14:35:07]

Please review.

[jam, 2011-09-27 16:51:34]

http://codereview.chromium.org/7990005/diff/12001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/7990005/diff/12001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:302: if (i == 0 ||
plugin_allowed) {
it's not clear to me why you do this, i.e. why the renderer now needs to get a
list of plugins and pick one itself. why can't this be done in the browser?

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
content/browser/plugin_service.h:130: void GetMatchingPlugins(
nit: it seems that this is equivalent to GetPluginInfo but returns an array. Can
you call it GetPluginsInfo to match the existing terminology? the fewer plugin
related names we have, the easier it is for people to guess what a function/ipc
does

http://codereview.chromium.org/7990005/diff/12001/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7990005/diff/12001/content/common/view_message...
content/common/view_messages.h:1472: // Return information about a plugin for
the given URL and MIME
nit: update comment

http://codereview.chromium.org/7990005/diff/12001/content/renderer/render_view.h
File content/renderer/render_view.h (right):

http://codereview.chromium.org/7990005/diff/12001/content/renderer/render_vie...
content/renderer/render_view.h:80: struct ViewHostMsg_GetMatchingPlugins_Params;
nit: not needed

http://codereview.chromium.org/7990005/diff/12001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

http://codereview.chromium.org/7990005/diff/12001/ipc/ipc_message_utils.h#new...
ipc/ipc_message_utils.h:501: struct ParamTraits<std::vector<bool> > {
why do you need this? i.e. why isn't the traits below enough?

[Bernhard Bauer, 2011-09-27 21:21:39]

Thanks for the review! I'll address the other comments tomorrow.

http://codereview.chromium.org/7990005/diff/12001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/7990005/diff/12001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:302: if (i == 0 ||
plugin_allowed) {
On 2011/09/27 16:51:34, John Abd-El-Malek wrote:
> it's not clear to me why you do this, i.e. why the renderer now needs to get a
> list of plugins and pick one itself. why can't this be done in the browser?

I want to differentiate between the case where there is no plug-in installed for
a given MIME type (in which case we show the missing plug-in placeholder), or
there is at least one plug-in, but none of them is enabled (in which case we may
want to show a different placeholder, with information about the first one of
those plug-ins). The browser side of this is in content/ though, and I'd prefer
to have this logic in chrome/.

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
content/browser/plugin_service.h:130: void GetMatchingPlugins(
On 2011/09/27 16:51:34, John Abd-El-Malek wrote:
> nit: it seems that this is equivalent to GetPluginInfo but returns an array.
Can
> you call it GetPluginsInfo to match the existing terminology? the fewer plugin
> related names we have, the easier it is for people to guess what a
function/ipc
> does

That's why I tried to give it a more descriptive name ;) Plugin is conceptually
equal to PluginInfo, so GetPluginInfos wouldn't tell anything about the set of
PluginInfos it returns. If we'd rename GetPluginInfo to GetMatchingPlugin, we'd
have GetPlugins (all plug-ins), GetMatchingPlugins (all matching plug-ins), and
GetMatchingPlugin (first matching and allowed plug-in).

http://codereview.chromium.org/7990005/diff/12001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

http://codereview.chromium.org/7990005/diff/12001/ipc/ipc_message_utils.h#new...
ipc/ipc_message_utils.h:501: struct ParamTraits<std::vector<bool> > {
On 2011/09/27 16:51:34, John Abd-El-Malek wrote:
> why do you need this? i.e. why isn't the traits below enough?

std::vector has a specialization for bool which uses more compact storage
(storing eight bools in a byte), but it means that it can't return a pointer to
an individual item, and that's what the ParamTraits below uses.

[jam, 2011-09-29 01:03:31]

sorry for the delay, I just saw your reply. Please feel free to ping me if I
don't respond within an hour or two.

http://codereview.chromium.org/7990005/diff/12001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/7990005/diff/12001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:302: if (i == 0 ||
plugin_allowed) {
On 2011/09/27 21:21:39, Bernhard Bauer wrote:
> On 2011/09/27 16:51:34, John Abd-El-Malek wrote:
> > it's not clear to me why you do this, i.e. why the renderer now needs to get
a
> > list of plugins and pick one itself. why can't this be done in the browser?
> 
> I want to differentiate between the case where there is no plug-in installed
for
> a given MIME type (in which case we show the missing plug-in placeholder), or
> there is at least one plug-in, but none of them is enabled (in which case we
may
> want to show a different placeholder, with information about the first one of
> those plug-ins). The browser side of this is in content/ though, and I'd
prefer
> to have this logic in chrome/.

Since content's renderer side doesn't care about getting multiple plugins: why
not have a separate IPC that's used in chrome to get a list. That IPC can be
dispatched in ChromeRenderMessageFilter, and it can do the filtering and return
a boolean indicating whether there's no plugin or there's no enabled plugin.
Then we can get rid of the filtering concept from content?

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
content/browser/plugin_service.h:130: void GetMatchingPlugins(
On 2011/09/27 21:21:39, Bernhard Bauer wrote:
> On 2011/09/27 16:51:34, John Abd-El-Malek wrote:
> > nit: it seems that this is equivalent to GetPluginInfo but returns an array.
> Can
> > you call it GetPluginsInfo to match the existing terminology? the fewer
plugin
> > related names we have, the easier it is for people to guess what a
> function/ipc
> > does
> 
> That's why I tried to give it a more descriptive name ;) Plugin is
conceptually
> equal to PluginInfo, so GetPluginInfos wouldn't tell anything about the set of
> PluginInfos it returns.

You lost me here :) I'm still not clear why GetPluginsInfo wouldn't be good,
i.e. wouldn't anyone who wants one or multiple plugin always want the "matching"
ones? How is this match different than GetPluginInfo?
 
> If we'd rename GetPluginInfo to GetMatchingPlugin, we'd
> have GetPlugins (all plug-ins), GetMatchingPlugins (all matching plug-ins),
and
> GetMatchingPlugin (first matching and allowed plug-in).

http://codereview.chromium.org/7990005/diff/12001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

http://codereview.chromium.org/7990005/diff/12001/ipc/ipc_message_utils.h#new...
ipc/ipc_message_utils.h:501: struct ParamTraits<std::vector<bool> > {
On 2011/09/27 21:21:39, Bernhard Bauer wrote:
> On 2011/09/27 16:51:34, John Abd-El-Malek wrote:
> > why do you need this? i.e. why isn't the traits below enough?
> 
> std::vector has a specialization for bool which uses more compact storage
> (storing eight bools in a byte), but it means that it can't return a pointer
to
> an individual item, and that's what the ParamTraits below uses. 

what paramtraits below are you talking about?

[Robert Sesek, 2011-09-29 01:09:48]

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
content/browser/plugin_service.h:138: bool* use_stale,
This isn't going to work with the direction we're going. Plugin loading will
only happen on the FILE thread on Windows; Linux and Mac are going to be OOP
(Mac already is). Callers need to either accept stale data, or should call
GetPlugins() first to ensure it is up-to-date.

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
content/browser/plugin_service.h:140: std::vector<std::string>* mime_types,
I'm adding a function that is very similar to this in a WIP.
http://codereview.chromium.org/8071013

It's not quite ready for review, though.

[jam, 2011-09-29 01:13:49]

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
content/browser/plugin_service.h:138: bool* use_stale,
On 2011/09/29 01:09:48, rsesek wrote:
> This isn't going to work with the direction we're going. Plugin loading will
> only happen on the FILE thread on Windows; Linux and Mac are going to be OOP
> (Mac already is). Callers need to either accept stale data, or should call
> GetPlugins() first to ensure it is up-to-date.

right, I think Bernhard's change is from a revision before your change. Once he
syncs he would get a CHECK failure :) If this is just a separate IPC that chrome
sends, then the dispatcher will call PluginService::GetPlugins asynchronously
and then filter when the results come back.

[Bernhard Bauer, 2011-09-29 13:20:47]

http://codereview.chromium.org/7990005/diff/12001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/7990005/diff/12001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:302: if (i == 0 ||
plugin_allowed) {
On 2011/09/29 01:03:32, John Abd-El-Malek wrote:
> On 2011/09/27 21:21:39, Bernhard Bauer wrote:
> > On 2011/09/27 16:51:34, John Abd-El-Malek wrote:
> > > it's not clear to me why you do this, i.e. why the renderer now needs to
get
> a
> > > list of plugins and pick one itself. why can't this be done in the
browser?
> > 
> > I want to differentiate between the case where there is no plug-in installed
> for
> > a given MIME type (in which case we show the missing plug-in placeholder),
or
> > there is at least one plug-in, but none of them is enabled (in which case we
> may
> > want to show a different placeholder, with information about the first one
of
> > those plug-ins). The browser side of this is in content/ though, and I'd
> prefer
> > to have this logic in chrome/.
> 
> Since content's renderer side doesn't care about getting multiple plugins: why
> not have a separate IPC that's used in chrome to get a list. That IPC can be
> dispatched in ChromeRenderMessageFilter, and it can do the filtering and
return
> a boolean indicating whether there's no plugin or there's no enabled plugin.

Ok, that sounds good. Then we can also do additional checks (is the plug-in
out-of-date? Is it blocked? etc.) there. 

> Then we can get rid of the filtering concept from content?

I don't think that's going to be possible though. There are still more calls to
get a plug-in for a MIME type from content/ (the actual OpenChannelToPlugin
call, as well as RenderView::CreatePluginNoCheck, which is called when loading a
blocked plug-in), and we need to make sure those obey the same rules for
disabled plug-ins.

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
content/browser/plugin_service.h:130: void GetMatchingPlugins(
On 2011/09/29 01:03:32, John Abd-El-Malek wrote:
> On 2011/09/27 21:21:39, Bernhard Bauer wrote:
> > On 2011/09/27 16:51:34, John Abd-El-Malek wrote:
> > > nit: it seems that this is equivalent to GetPluginInfo but returns an
array.
> > Can
> > > you call it GetPluginsInfo to match the existing terminology? the fewer
> plugin
> > > related names we have, the easier it is for people to guess what a
> > function/ipc
> > > does
> > 
> > That's why I tried to give it a more descriptive name ;) Plugin is
> conceptually
> > equal to PluginInfo, so GetPluginInfos wouldn't tell anything about the set
of
> > PluginInfos it returns.
> 
> You lost me here :) I'm still not clear why GetPluginsInfo wouldn't be good,
> i.e. wouldn't anyone who wants one or multiple plugin always want the
"matching"
> ones? How is this match different than GetPluginInfo?

It's not, that's why I'd prefer to rename GetPluginInfo instead ;-) The only
difference in name between GetPlugins and GetPluginInfos is the "Infos" (which
doesn't convey any information by itself), but there is a big difference in the
set of plug-ins that is returned. It's not obvious that one returns *all*
plug-ins, whereas the other returns only matching plug-ins.

> > If we'd rename GetPluginInfo to GetMatchingPlugin, we'd
> > have GetPlugins (all plug-ins), GetMatchingPlugins (all matching plug-ins),
> and
> > GetMatchingPlugin (first matching and allowed plug-in).
>

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
content/browser/plugin_service.h:138: bool* use_stale,
On 2011/09/29 01:13:49, John Abd-El-Malek wrote:
> On 2011/09/29 01:09:48, rsesek wrote:
> > This isn't going to work with the direction we're going. Plugin loading will
> > only happen on the FILE thread on Windows; Linux and Mac are going to be OOP
> > (Mac already is). Callers need to either accept stale data, or should call
> > GetPlugins() first to ensure it is up-to-date.
> 
> right, I think Bernhard's change is from a revision before your change. Once
he
> syncs he would get a CHECK failure :) If this is just a separate IPC that
chrome
> sends, then the dispatcher will call PluginService::GetPlugins asynchronously
> and then filter when the results come back.

No, I did not get a CHECK :) Note that this is essentially the same as the
already existing GetPluginInfo method, it just returns all matching plug-ins
instead of the first enabled one. That's also why the signature is so similar.
If the signature of GetPluginInfo should change before this CL is landed, I'll
just update this method's signature accordingly.

http://codereview.chromium.org/7990005/diff/12001/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7990005/diff/12001/content/common/view_message...
content/common/view_messages.h:1472: // Return information about a plugin for
the given URL and MIME
On 2011/09/27 16:51:34, John Abd-El-Malek wrote:
> nit: update comment

Done.

http://codereview.chromium.org/7990005/diff/12001/content/renderer/render_view.h
File content/renderer/render_view.h (right):

http://codereview.chromium.org/7990005/diff/12001/content/renderer/render_vie...
content/renderer/render_view.h:80: struct ViewHostMsg_GetMatchingPlugins_Params;
On 2011/09/27 16:51:34, John Abd-El-Malek wrote:
> nit: not needed

Done.

http://codereview.chromium.org/7990005/diff/12001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

http://codereview.chromium.org/7990005/diff/12001/ipc/ipc_message_utils.h#new...
ipc/ipc_message_utils.h:501: struct ParamTraits<std::vector<bool> > {
On 2011/09/29 01:03:32, John Abd-El-Malek wrote:
> On 2011/09/27 21:21:39, Bernhard Bauer wrote:
> > On 2011/09/27 16:51:34, John Abd-El-Malek wrote:
> > > why do you need this? i.e. why isn't the traits below enough?
> > 
> > std::vector has a specialization for bool which uses more compact storage
> > (storing eight bools in a byte), but it means that it can't return a pointer
> to
> > an individual item, and that's what the ParamTraits below uses. 
> 
> what paramtraits below are you talking about? 

ParamTraits<std::vector<P> > takes the address of the i-th element of the vector
(line 549), and that's not possible if you have a more compact storage than one
byte per element.

[jam, 2011-09-29 16:31:09]

http://codereview.chromium.org/7990005/diff/12001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/7990005/diff/12001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:302: if (i == 0 ||
plugin_allowed) {
On 2011/09/29 13:20:47, Bernhard Bauer wrote:
> On 2011/09/29 01:03:32, John Abd-El-Malek wrote:
> > On 2011/09/27 21:21:39, Bernhard Bauer wrote:
> > > On 2011/09/27 16:51:34, John Abd-El-Malek wrote:
> > > > it's not clear to me why you do this, i.e. why the renderer now needs to
> get
> > a
> > > > list of plugins and pick one itself. why can't this be done in the
> browser?
> > > 
> > > I want to differentiate between the case where there is no plug-in
installed
> > for
> > > a given MIME type (in which case we show the missing plug-in placeholder),
> or
> > > there is at least one plug-in, but none of them is enabled (in which case
we
> > may
> > > want to show a different placeholder, with information about the first one
> of
> > > those plug-ins). The browser side of this is in content/ though, and I'd
> > prefer
> > > to have this logic in chrome/.
> > 
> > Since content's renderer side doesn't care about getting multiple plugins:
why
> > not have a separate IPC that's used in chrome to get a list. That IPC can be
> > dispatched in ChromeRenderMessageFilter, and it can do the filtering and
> return
> > a boolean indicating whether there's no plugin or there's no enabled plugin.
> 
> Ok, that sounds good. Then we can also do additional checks (is the plug-in
> out-of-date? Is it blocked? etc.) there. 

yep sounds good.

> 
> > Then we can get rid of the filtering concept from content?
> 
> I don't think that's going to be possible though. There are still more calls
to
> get a plug-in for a MIME type from content/ (the actual OpenChannelToPlugin
> call, as well as RenderView::CreatePluginNoCheck, which is called when loading
a
> blocked plug-in), and we need to make sure those obey the same rules for
> disabled plug-ins.

ah right, nm :)

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
content/browser/plugin_service.h:130: void GetMatchingPlugins(
On 2011/09/29 13:20:47, Bernhard Bauer wrote:
> On 2011/09/29 01:03:32, John Abd-El-Malek wrote:
> > On 2011/09/27 21:21:39, Bernhard Bauer wrote:
> > > On 2011/09/27 16:51:34, John Abd-El-Malek wrote:
> > > > nit: it seems that this is equivalent to GetPluginInfo but returns an
> array.
> > > Can
> > > > you call it GetPluginsInfo to match the existing terminology? the fewer
> > plugin
> > > > related names we have, the easier it is for people to guess what a
> > > function/ipc
> > > > does
> > > 
> > > That's why I tried to give it a more descriptive name ;) Plugin is
> > conceptually
> > > equal to PluginInfo, so GetPluginInfos wouldn't tell anything about the
set
> of
> > > PluginInfos it returns.
> > 
> > You lost me here :) I'm still not clear why GetPluginsInfo wouldn't be good,
> > i.e. wouldn't anyone who wants one or multiple plugin always want the
> "matching"
> > ones? How is this match different than GetPluginInfo?
> 
> It's not, that's why I'd prefer to rename GetPluginInfo instead ;-) The only
> difference in name between GetPlugins and GetPluginInfos is the "Infos" (which
> doesn't convey any information by itself), but there is a big difference in
the
> set of plug-ins that is returned. It's not obvious that one returns *all*
> plug-ins, whereas the other returns only matching plug-ins.
> 
> > > If we'd rename GetPluginInfo to GetMatchingPlugin, we'd
> > > have GetPlugins (all plug-ins), GetMatchingPlugins (all matching
plug-ins),
> > and
> > > GetMatchingPlugin (first matching and allowed plug-in).
> > 
> 

Ok I think I understand your point now, which is that GetPluginsInfo is too
similar to the existing GetPlugins (I was comparing it to GetPluginInfo)

http://codereview.chromium.org/7990005/diff/12001/content/browser/plugin_serv...
content/browser/plugin_service.h:138: bool* use_stale,
On 2011/09/29 13:20:47, Bernhard Bauer wrote:
> On 2011/09/29 01:13:49, John Abd-El-Malek wrote:
> > On 2011/09/29 01:09:48, rsesek wrote:
> > > This isn't going to work with the direction we're going. Plugin loading
will
> > > only happen on the FILE thread on Windows; Linux and Mac are going to be
OOP
> > > (Mac already is). Callers need to either accept stale data, or should call
> > > GetPlugins() first to ensure it is up-to-date.
> > 
> > right, I think Bernhard's change is from a revision before your change. Once
> he
> > syncs he would get a CHECK failure :) If this is just a separate IPC that
> chrome
> > sends, then the dispatcher will call PluginService::GetPlugins
asynchronously
> > and then filter when the results come back.
> 
> No, I did not get a CHECK :) 

hmm, probably because it was cached then.

> Note that this is essentially the same as the
> already existing GetPluginInfo method, it just returns all matching plug-ins
> instead of the first enabled one. That's also why the signature is so similar.
> If the signature of GetPluginInfo should change before this CL is landed, I'll
> just update this method's signature accordingly.

http://codereview.chromium.org/7990005/diff/12001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

http://codereview.chromium.org/7990005/diff/12001/ipc/ipc_message_utils.h#new...
ipc/ipc_message_utils.h:501: struct ParamTraits<std::vector<bool> > {
On 2011/09/29 13:20:47, Bernhard Bauer wrote:
> On 2011/09/29 01:03:32, John Abd-El-Malek wrote:
> > On 2011/09/27 21:21:39, Bernhard Bauer wrote:
> > > On 2011/09/27 16:51:34, John Abd-El-Malek wrote:
> > > > why do you need this? i.e. why isn't the traits below enough?
> > > 
> > > std::vector has a specialization for bool which uses more compact storage
> > > (storing eight bools in a byte), but it means that it can't return a
pointer
> > to
> > > an individual item, and that's what the ParamTraits below uses. 
> > 
> > what paramtraits below are you talking about? 
> 
> ParamTraits<std::vector<P> > takes the address of the i-th element of the
vector
> (line 549), and that's not possible if you have a more compact storage than
one
> byte per element.

ah, nm now i get it. I was familiar with vector's optimization for bool, but I
thought you were doing this change for something specific in your new message.
I'm very surprised that we never sent a vector of bools before! Even if you
don't end up using it in your new IPC, of course please leave this as it'll be
useful to have.

[Bernhard Bauer, 2011-09-30 16:21:28]

Okay, I added a separate message in ChromeRenderMessageFilter. PTAL?

[jam, 2011-09-30 17:01:21]

http://codereview.chromium.org/7990005/diff/28017/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/7990005/diff/28017/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_message_filter.cc:515: const
GetPluginInfoCallback& callback,
it seems unnecessary to make a function in one class be passed a callback to
another function in this class, i.e. you can just call GetPluginInfo here
directly

http://codereview.chromium.org/7990005/diff/28017/chrome/common/render_messag...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/7990005/diff/28017/chrome/common/render_messag...
chrome/common/render_messages.h:142:
IPC_STRUCT_BEGIN(ChromeViewHostMsg_GetPluginInfo_Params)
nit: we normally don't create structs unless we need > 4 parameters which is
what our IPC macros support. if you're just sending 3 out parameters, use
IPC_SYNC_MESSAGE_CONTROL4_3

http://codereview.chromium.org/7990005/diff/28017/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7990005/diff/28017/content/browser/plugin_serv...
content/browser/plugin_service.h:126: bool GetPluginInfoArray(const GURL& url,
is it really necessary to add a wrapper function around PluginList instead of
having callers call it directly?

http://codereview.chromium.org/7990005/diff/28017/content/browser/plugin_serv...
content/browser/plugin_service.h:161: void EnsurePluginsLoaded(const
base::Closure& callback);
this function doesn't seem necessary since it can already be done with existing
methods (the fewer methods there are, the better!). if someone just wants to
know that plugins are loaded, they can call GetPlugins and ignore the result.

[Bernhard Bauer, 2011-09-30 17:22:21]

http://codereview.chromium.org/7990005/diff/28017/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/7990005/diff/28017/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_message_filter.cc:515: const
GetPluginInfoCallback& callback,
On 2011/09/30 17:01:21, John Abd-El-Malek wrote:
> it seems unnecessary to make a function in one class be passed a callback to
> another function in this class, i.e. you can just call GetPluginInfo here
> directly

Ok. I guess I just like currying stuff now that I can ;-)

http://codereview.chromium.org/7990005/diff/28017/chrome/common/render_messag...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/7990005/diff/28017/chrome/common/render_messag...
chrome/common/render_messages.h:142:
IPC_STRUCT_BEGIN(ChromeViewHostMsg_GetPluginInfo_Params)
On 2011/09/30 17:01:21, John Abd-El-Malek wrote:
> nit: we normally don't create structs unless we need > 4 parameters which is
> what our IPC macros support. if you're just sending 3 out parameters, use
> IPC_SYNC_MESSAGE_CONTROL4_3

I need to forward-declare the struct, and I can't do that for an enum. It looks
to me like including one of these _messages.h files in another *header* doesn't
work (because the missing include guards screw things up), so if I want to refer
to the message parameters in a header file, I have to forward-declare them :-/

http://codereview.chromium.org/7990005/diff/28017/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7990005/diff/28017/content/browser/plugin_serv...
content/browser/plugin_service.h:126: bool GetPluginInfoArray(const GURL& url,
On 2011/09/30 17:01:21, John Abd-El-Malek wrote:
> is it really necessary to add a wrapper function around PluginList instead of
> having callers call it directly?

That's at least Robert's plan for the future. I copied this method from
http://codereview.chromium.org/8071013/.

http://codereview.chromium.org/7990005/diff/28017/content/browser/plugin_serv...
content/browser/plugin_service.h:161: void EnsurePluginsLoaded(const
base::Closure& callback);
On 2011/09/30 17:01:21, John Abd-El-Malek wrote:
> this function doesn't seem necessary since it can already be done with
existing
> methods (the fewer methods there are, the better!). if someone just wants to
> know that plugins are loaded, they can call GetPlugins and ignore the result.

I find it a bit awkward having the list of plug-ins as an additional parameter
and then ignoring it. ChromeRenderMessageFilter::GetPluginInfo already has
enough parameters as it is (and I was actually hitting the base::Callback limits
with it!).

[jam, 2011-09-30 17:40:19]

http://codereview.chromium.org/7990005/diff/28017/chrome/common/render_messag...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/7990005/diff/28017/chrome/common/render_messag...
chrome/common/render_messages.h:142:
IPC_STRUCT_BEGIN(ChromeViewHostMsg_GetPluginInfo_Params)
On 2011/09/30 17:22:21, Bernhard Bauer wrote:
> On 2011/09/30 17:01:21, John Abd-El-Malek wrote:
> > nit: we normally don't create structs unless we need > 4 parameters which is
> > what our IPC macros support. if you're just sending 3 out parameters, use
> > IPC_SYNC_MESSAGE_CONTROL4_3
> 
> I need to forward-declare the struct, and I can't do that for an enum. It
looks
> to me like including one of these _messages.h files in another *header*
doesn't
> work 

that's by design, since there's a lot of unnecessary template expansion when
they're included in headers

>(because the missing include guards screw things up), so if I want to refer
> to the message parameters in a header file, I have to forward-declare them :-/

if you make ChromeRenderMessageFilter::PluginsLoaded() do all the work (and not
have GetPluginInfo()), then you don't need to forward declare this enum in a
header. the function except for the last two lines would only be indented once
more.

http://codereview.chromium.org/7990005/diff/28017/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7990005/diff/28017/content/browser/plugin_serv...
content/browser/plugin_service.h:126: bool GetPluginInfoArray(const GURL& url,
On 2011/09/30 17:22:21, Bernhard Bauer wrote:
> On 2011/09/30 17:01:21, John Abd-El-Malek wrote:
> > is it really necessary to add a wrapper function around PluginList instead
of
> > having callers call it directly?
> 
> That's at least Robert's plan for the future. I copied this method from
> http://codereview.chromium.org/8071013/.

I see. it's a bit unfortunate to have these wrappers, but ok.

http://codereview.chromium.org/7990005/diff/28017/content/browser/plugin_serv...
content/browser/plugin_service.h:161: void EnsurePluginsLoaded(const
base::Closure& callback);
On 2011/09/30 17:22:21, Bernhard Bauer wrote:
> On 2011/09/30 17:01:21, John Abd-El-Malek wrote:
> > this function doesn't seem necessary since it can already be done with
> existing
> > methods (the fewer methods there are, the better!). if someone just wants to
> > know that plugins are loaded, they can call GetPlugins and ignore the
result.
> 
> I find it a bit awkward having the list of plug-ins as an additional parameter
> and then ignoring it.

we already do this (i.e. BufferedRersourceHandler)

> ChromeRenderMessageFilter::GetPluginInfo already has
> enough parameters as it is (and I was actually hitting the base::Callback
limits
> with it!).

we ignore parameters all the time in various methods, so that's nothing new.

For a base class like PluginService that's used by a lot of code, having the
fewest number of methods is useful, because it makes it easy for developers not
familiar with it to quickly scan all the public methods. in this case, i think
the cost of having a CRMF method take an unused parameter is low vs adding
another function to PS

[Bernhard Bauer, 2011-10-05 13:34:33]

http://codereview.chromium.org/7990005/diff/28017/chrome/common/render_messag...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/7990005/diff/28017/chrome/common/render_messag...
chrome/common/render_messages.h:142:
IPC_STRUCT_BEGIN(ChromeViewHostMsg_GetPluginInfo_Params)
On 2011/09/30 17:40:19, John Abd-El-Malek wrote:
> On 2011/09/30 17:22:21, Bernhard Bauer wrote:
> > On 2011/09/30 17:01:21, John Abd-El-Malek wrote:
> > > nit: we normally don't create structs unless we need > 4 parameters which
is
> > > what our IPC macros support. if you're just sending 3 out parameters, use
> > > IPC_SYNC_MESSAGE_CONTROL4_3
> > 
> > I need to forward-declare the struct, and I can't do that for an enum. It
> looks
> > to me like including one of these _messages.h files in another *header*
> doesn't
> > work 
> 
> that's by design, since there's a lot of unnecessary template expansion when
> they're included in headers
> 
> >(because the missing include guards screw things up), so if I want to refer
> > to the message parameters in a header file, I have to forward-declare them
:-/
> 
> if you make ChromeRenderMessageFilter::PluginsLoaded() do all the work (and
not
> have GetPluginInfo()), then you don't need to forward declare this enum in a
> header. the function except for the last two lines would only be indented once
> more.

I split up ChromeViewHostMsg_GetPluginInfo_Params into its components. I kept
ChromeViewHostMsg_GetPluginInfo_Status for the forward-declaration (having a
nice signature for CRMF::GetPluginInfo makes it easier to unit-test it, for
example, and there are other instances of wrapping an enum in a struct, like
ViewMsg_StopFinding_Params).

http://codereview.chromium.org/7990005/diff/28017/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7990005/diff/28017/content/browser/plugin_serv...
content/browser/plugin_service.h:161: void EnsurePluginsLoaded(const
base::Closure& callback);
On 2011/09/30 17:40:19, John Abd-El-Malek wrote:
> On 2011/09/30 17:22:21, Bernhard Bauer wrote:
> > On 2011/09/30 17:01:21, John Abd-El-Malek wrote:
> > > this function doesn't seem necessary since it can already be done with
> > existing
> > > methods (the fewer methods there are, the better!). if someone just wants
to
> > > know that plugins are loaded, they can call GetPlugins and ignore the
> result.
> > 
> > I find it a bit awkward having the list of plug-ins as an additional
parameter
> > and then ignoring it.
> 
> we already do this (i.e. BufferedRersourceHandler)
> 
> > ChromeRenderMessageFilter::GetPluginInfo already has
> > enough parameters as it is (and I was actually hitting the base::Callback
> limits
> > with it!).
> 
> we ignore parameters all the time in various methods, so that's nothing new.
> 
> For a base class like PluginService that's used by a lot of code, having the
> fewest number of methods is useful, because it makes it easy for developers
not
> familiar with it to quickly scan all the public methods. in this case, i think
> the cost of having a CRMF method take an unused parameter is low vs adding
> another function to PS

ok, done.

[jam, 2011-10-05 17:22:10]

lgtm

http://codereview.chromium.org/7990005/diff/37001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/7990005/diff/37001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:473: return
render_view->CreatePepperPlugin(frame, params, plugin.path,
nit: here and below, both forms of tabbing are allowed per the style guide. so
it's better that people don't change it to suit their personal preference,
otherwise there'll be a lot of hysteresis if everyone changes it back and forth

[commit-bot: I haz the power, 2011-10-05 17:26:52]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/7990005/40003

[Bernhard Bauer, 2011-10-05 17:30:25]

http://codereview.chromium.org/7990005/diff/37001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/7990005/diff/37001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:473: return
render_view->CreatePepperPlugin(frame, params, plugin.path,
On 2011/10/05 17:22:10, John Abd-El-Malek wrote:
> nit: here and below, both forms of tabbing are allowed per the style guide. so
> it's better that people don't change it to suit their personal preference,
> otherwise there'll be a lot of hysteresis if everyone changes it back and
forth

Done.

[commit-bot: I haz the power, 2011-10-05 18:55:45]

Change committed as 104144