Show a replacement plug-in for loading errors.

Show a replacement plug-in for loading errors.

This allows us to show a nicer placeholder when loading a plug-in fails then the one coming from WebKit.

BUG=123580
TEST=manual

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

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

[Bernhard Bauer, 2012-04-18 16:41:35]

Hi John + Darin, 

I want to show a custom placeholder when loading a plug-in fails. Could you take
a look at this CL and see if this is a reasonable approach? The interesting
parts are in (ppapi)?webplugin_impl.cc, which calls back through the content
client interface to get a WebViewPlugin as a replacement.

An alternative would be to do the plug-in switch in FrameLoaderClientImpl, but
that would require changing the WebKit embedder interface, and it wouldn't tell
us which plug-in actually failed to load. You can see that approach in patch set
2 (the full patch including WebKit changes is at
http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/18519/...).

Thanks!
Bernhard.

[Bernhard Bauer, 2012-04-23 20:57:09]

On 2012/04/18 16:41:35, Bernhard Bauer wrote:
> Hi John + Darin, 
> 
> I want to show a custom placeholder when loading a plug-in fails. Could you
take
> a look at this CL and see if this is a reasonable approach? The interesting
> parts are in (ppapi)?webplugin_impl.cc, which calls back through the content
> client interface to get a WebViewPlugin as a replacement.
> 
> An alternative would be to do the plug-in switch in FrameLoaderClientImpl, but
> that would require changing the WebKit embedder interface, and it wouldn't
tell
> us which plug-in actually failed to load. You can see that approach in patch
set
> 2 (the full patch including WebKit changes is at
>
http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/18519/...).
> 
> Thanks!
> Bernhard.

Ping?

[jam, 2012-04-23 22:44:08]

sorry I skipped this because of the "WIP" in the title.

has this gone through UI review? do we know how often this happens, i.e. that
it's worth adding this UI for?

[bauerb at google, 2012-04-24 13:38:06]

This does indeed happen often enough to be noticed; it's one of the Top
User Issues ("Missing plug-in"). I coordinated with Julia Tan (ConOps) and
Patrick Nepper (PM) on it.

I'll start a UI review for the current version on a separate thread.

On Tue Apr 24 00:44:08 GMT+200 2012, <jam@chromium.org> wrote:

sorry I skipped this because of the "WIP" in the title.

has this gone through UI review? do we know how often this happens, i.e.
that
it's worth adding this UI for?

http://codereview.chromium.org/10093011/<https://www.google.com/url?sa=D&q=ht...

[jam, 2012-04-24 15:14:29]

On 2012/04/24 13:38:06, please use chromium account wrote:
> This does indeed happen often enough to be noticed; it's one of the Top
> User Issues ("Missing plug-in"). I coordinated with Julia Tan (ConOps) and
> Patrick Nepper (PM) on it.
> 
> I'll start a UI review for the current version on a separate thread.

Just to double check: what currently gives the "Missing plug-in" message? Just
the plugin failing to load, or also the user not having the plugin installed?

btw the changes to the two cc files you mentioned are trivial and look fine, so
I can review once you have a final patch and after I figure out the answer to
the above :)

[Bernhard Bauer, 2012-04-24 15:28:42]

On 2012/04/24 15:14:29, John Abd-El-Malek wrote:
> On 2012/04/24 13:38:06, please use chromium account wrote:
> > This does indeed happen often enough to be noticed; it's one of the Top
> > User Issues ("Missing plug-in"). I coordinated with Julia Tan (ConOps) and
> > Patrick Nepper (PM) on it.
> > 
> > I'll start a UI review for the current version on a separate thread.
> 
> Just to double check: what currently gives the "Missing plug-in" message? Just
> the plugin failing to load, or also the user not having the plugin installed?

Yes, only the plug-in failing to load triggers it (when the plug-in is missing,
we provide our own placeholder from ChromeContentRendererClient). It does seem
to happen surprisingly often though, and we're not completely sure why (in the
past, there have been several different issues, see http://crbug.com/108228 and
http://crbug,com/123969 for example).

In any case, the current UI just looks plain ugly, because it's the standard UI
coming from Webkit :D

> btw the changes to the two cc files you mentioned are trivial and look fine,
so
> I can review once you have a final patch and after I figure out the answer to
> the above :)

[jam, 2012-04-27 16:09:58]

lgtm

[commit-bot: I haz the power, 2012-04-27 17:22:27]

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

[commit-bot: I haz the power, 2012-04-27 17:22:38]

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

Running presubmit commit checks ...

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

Presubmit checks took 2.3s to calculate.

[Bernhard Bauer, 2012-04-30 15:22:54]

Darin, could I trouble you for an OWNERS review for webkit/?

Thanks!

[darin (slow to review), 2012-04-30 18:57:02]

http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_delegate.h:58: struct DeviceRefData;
are you sure about this change?  class is sorted before struct, no?  i didn't
think we had a style rule that these should be sorted on type name instead of
first character in the row.

http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):

http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:31: using WebKit::WebPluginParams;
add using WebKit::WebPlugin here?

http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:92: LOG(ERROR) << "Couldn't
initialize replacement plug-in";
do we really need all of these LOG(ERROR) statements in this patch?  just use
DLOG(ERROR) instead?  or, are you really hoping to collect chrome_debug.log
files from end users?

[Bernhard Bauer, 2012-04-30 19:30:34]

http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/plugi...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/plugi...
webkit/plugins/ppapi/plugin_delegate.h:58: struct DeviceRefData;
On 2012/04/30 18:57:02, darin wrote:
> are you sure about this change?  class is sorted before struct, no?  i didn't
> think we had a style rule that these should be sorted on type name instead of
> first character in the row.

Up in lines 34–38 we sort by type name :)

http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right):

http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:31: using WebKit::WebPluginParams;
On 2012/04/30 18:57:02, darin wrote:
> add using WebKit::WebPlugin here?

Done.

http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:92: LOG(ERROR) << "Couldn't
initialize replacement plug-in";
On 2012/04/30 18:57:02, darin wrote:
> do we really need all of these LOG(ERROR) statements in this patch?  just use
> DLOG(ERROR) instead?  or, are you really hoping to collect chrome_debug.log
> files from end users?

Right; I do want logging if we can't load the actual plug-in, but the
replacement plug-in won't ever fail as long as it is a WebViewPlugin.

[darin (slow to review), 2012-04-30 19:43:16]

On Mon, Apr 30, 2012 at 12:30 PM, <bauerb@chromium.org> wrote:

>
> http://codereview.chromium.**org/10093011/diff/28001/**
>
webkit/plugins/ppapi/plugin_**delegate.h<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/plugin_delegate.h>
> File webkit/plugins/ppapi/plugin_**delegate.h (right):
>
> http://codereview.chromium.**org/10093011/diff/28001/**
>
webkit/plugins/ppapi/plugin_**delegate.h#newcode58<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/plugin_delegate.h#newcode58>
> webkit/plugins/ppapi/plugin_**delegate.h:58: struct DeviceRefData;
> On 2012/04/30 18:57:02, darin wrote:
>
>> are you sure about this change?  class is sorted before struct, no?  i
>>
> didn't
>
>> think we had a style rule that these should be sorted on type name
>>
> instead of
>
>> first character in the row.
>>
>
> Up in lines 34–38 we sort by type name :)


Which style is correct?  Is there any style rule for this?



>
>
> http://codereview.chromium.**org/10093011/diff/28001/**
>
webkit/plugins/ppapi/ppapi_**webplugin_impl.cc<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi_webplugin_impl.cc>
> File webkit/plugins/ppapi/ppapi_**webplugin_impl.cc (right):
>
> http://codereview.chromium.**org/10093011/diff/28001/**
>
webkit/plugins/ppapi/ppapi_**webplugin_impl.cc#newcode31<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi_webplugin_impl.cc#newcode31>
> webkit/plugins/ppapi/ppapi_**webplugin_impl.cc:31: using
> WebKit::WebPluginParams;
> On 2012/04/30 18:57:02, darin wrote:
>
>> add using WebKit::WebPlugin here?
>>
>
> Done.
>
>
> http://codereview.chromium.**org/10093011/diff/28001/**
>
webkit/plugins/ppapi/ppapi_**webplugin_impl.cc#newcode92<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi_webplugin_impl.cc#newcode92>
> webkit/plugins/ppapi/ppapi_**webplugin_impl.cc:92: LOG(ERROR) << "Couldn't
> initialize replacement plug-in";
> On 2012/04/30 18:57:02, darin wrote:
>
>> do we really need all of these LOG(ERROR) statements in this patch?
>>
> just use
>
>> DLOG(ERROR) instead?  or, are you really hoping to collect
>>
> chrome_debug.log
>
>> files from end users?
>>
>
> Right; I do want logging if we can't load the actual plug-in, but the
> replacement plug-in won't ever fail as long as it is a WebViewPlugin.
>

It just seems like there is some extraneous release-mode logging.  You
should only need to output one line in the log file if a plugin cannot be
loaded.
Ideally, it should give some indication of why the plugin could not be
loaded,
right?

I just think we should be mindful of the bloatiness of LOG statements.

-Darin



>
>
http://codereview.chromium.**org/10093011/<http://codereview.chromium.org/100...
>

[Bernhard Bauer, 2012-04-30 20:02:11]

On 2012/04/30 19:43:16, darin wrote:
> On Mon, Apr 30, 2012 at 12:30 PM, <mailto:bauerb@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.**org/10093011/diff/28001/**
> >
>
webkit/plugins/ppapi/plugin_**delegate.h<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/plugin_delegate.h>
> > File webkit/plugins/ppapi/plugin_**delegate.h (right):
> >
> > http://codereview.chromium.**org/10093011/diff/28001/**
> >
>
webkit/plugins/ppapi/plugin_**delegate.h#newcode58<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/plugin_delegate.h#newcode58>
> > webkit/plugins/ppapi/plugin_**delegate.h:58: struct DeviceRefData;
> > On 2012/04/30 18:57:02, darin wrote:
> >
> >> are you sure about this change?  class is sorted before struct, no?  i
> >>
> > didn't
> >
> >> think we had a style rule that these should be sorted on type name
> >>
> > instead of
> >
> >> first character in the row.
> >>
> >
> > Up in lines 34–38 we sort by type name :)
> 
> 
> Which style is correct?  Is there any style rule for this?

I don't know either; I just tried to make it consistent inside this file. 

If there is no style rule, I think that sorting by first character is a bit
easier to read (and easier to achieve via `sort`), so I did that now :)

> >
> >
> > http://codereview.chromium.**org/10093011/diff/28001/**
> >
>
webkit/plugins/ppapi/ppapi_**webplugin_impl.cc<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi_webplugin_impl.cc>
> > File webkit/plugins/ppapi/ppapi_**webplugin_impl.cc (right):
> >
> > http://codereview.chromium.**org/10093011/diff/28001/**
> >
>
webkit/plugins/ppapi/ppapi_**webplugin_impl.cc#newcode31<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi_webplugin_impl.cc#newcode31>
> > webkit/plugins/ppapi/ppapi_**webplugin_impl.cc:31: using
> > WebKit::WebPluginParams;
> > On 2012/04/30 18:57:02, darin wrote:
> >
> >> add using WebKit::WebPlugin here?
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.**org/10093011/diff/28001/**
> >
>
webkit/plugins/ppapi/ppapi_**webplugin_impl.cc#newcode92<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi_webplugin_impl.cc#newcode92>
> > webkit/plugins/ppapi/ppapi_**webplugin_impl.cc:92: LOG(ERROR) << "Couldn't
> > initialize replacement plug-in";
> > On 2012/04/30 18:57:02, darin wrote:
> >
> >> do we really need all of these LOG(ERROR) statements in this patch?
> >>
> > just use
> >
> >> DLOG(ERROR) instead?  or, are you really hoping to collect
> >>
> > chrome_debug.log
> >
> >> files from end users?
> >>
> >
> > Right; I do want logging if we can't load the actual plug-in, but the
> > replacement plug-in won't ever fail as long as it is a WebViewPlugin.
> >
> 
> It just seems like there is some extraneous release-mode logging.  You
> should only need to output one line in the log file if a plugin cannot be
> loaded.
> Ideally, it should give some indication of why the plugin could not be
> loaded,
> right?

That's what I'm trying to do. Apart from WebPluginImpl and ppapi::WebPluginImpl,
where I have removed them, are there any other places in this CL where I missed
extraneous LOG statements? 

> I just think we should be mindful of the bloatiness of LOG statements.

> -Darin
> 
> 
> 
> >
> >
>
http://codereview.chromium.**org/10093011/%3Chttp://codereview.chromium.org/1...>
> >

[darin (slow to review), 2012-04-30 20:10:36]

OK, LGTM!

On Mon, Apr 30, 2012 at 1:02 PM, <bauerb@chromium.org> wrote:

> On 2012/04/30 19:43:16, darin wrote:
>
>> On Mon, Apr 30, 2012 at 12:30 PM, <mailto:bauerb@chromium.org> wrote:
>>
>
>  >
>> > http://codereview.chromium.****org/10093011/diff/28001/**
>> >
>>
>
> webkit/plugins/ppapi/plugin_****delegate.h<http://codereview.**
> chromium.org/10093011/diff/**28001/webkit/plugins/ppapi/**
>
plugin_delegate.h<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/plugin_delegate.h>
> >
>
>> > File webkit/plugins/ppapi/plugin_****delegate.h (right):
>> >
>> > http://codereview.chromium.****org/10093011/diff/28001/**
>> >
>>
>
> webkit/plugins/ppapi/plugin_****delegate.h#newcode58<http://**
> codereview.chromium.org/**10093011/diff/28001/webkit/**
>
plugins/ppapi/plugin_delegate.**h#newcode58<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/plugin_delegate.h#newcode58>
> >
>
>> > webkit/plugins/ppapi/plugin_****delegate.h:58: struct DeviceRefData;
>>
>> > On 2012/04/30 18:57:02, darin wrote:
>> >
>> >> are you sure about this change?  class is sorted before struct, no?  i
>> >>
>> > didn't
>> >
>> >> think we had a style rule that these should be sorted on type name
>> >>
>> > instead of
>> >
>> >> first character in the row.
>> >>
>> >
>> > Up in lines 34–38 we sort by type name :)
>>
>
>
>  Which style is correct?  Is there any style rule for this?
>>
>
> I don't know either; I just tried to make it consistent inside this file.
>
> If there is no style rule, I think that sorting by first character is a bit
> easier to read (and easier to achieve via `sort`), so I did that now :)
>
>  >
>> >
>> > http://codereview.chromium.****org/10093011/diff/28001/**
>> >
>>
>
> webkit/plugins/ppapi/ppapi_****webplugin_impl.cc<http://**
> codereview.chromium.org/**10093011/diff/28001/webkit/**
>
plugins/ppapi/ppapi_webplugin_**impl.cc<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi_webplugin_impl.cc>
> >
>
>> > File webkit/plugins/ppapi/ppapi_****webplugin_impl.cc (right):
>> >
>> > http://codereview.chromium.****org/10093011/diff/28001/**
>> >
>>
>
> webkit/plugins/ppapi/ppapi_****webplugin_impl.cc#newcode31<ht**
> tp://codereview.chromium.org/**10093011/diff/28001/webkit/**
>
plugins/ppapi/ppapi_webplugin_**impl.cc#newcode31<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi_webplugin_impl.cc#newcode31>
> >
>
>> > webkit/plugins/ppapi/ppapi_****webplugin_impl.cc:31: using
>>
>> > WebKit::WebPluginParams;
>> > On 2012/04/30 18:57:02, darin wrote:
>> >
>> >> add using WebKit::WebPlugin here?
>> >>
>> >
>> > Done.
>> >
>> >
>> > http://codereview.chromium.****org/10093011/diff/28001/**
>> >
>>
>
> webkit/plugins/ppapi/ppapi_****webplugin_impl.cc#newcode92<ht**
> tp://codereview.chromium.org/**10093011/diff/28001/webkit/**
>
plugins/ppapi/ppapi_webplugin_**impl.cc#newcode92<http://codereview.chromium.org/10093011/diff/28001/webkit/plugins/ppapi/ppapi_webplugin_impl.cc#newcode92>
> >
>
>> > webkit/plugins/ppapi/ppapi_****webplugin_impl.cc:92: LOG(ERROR) <<
>> "Couldn't
>>
>> > initialize replacement plug-in";
>> > On 2012/04/30 18:57:02, darin wrote:
>> >
>> >> do we really need all of these LOG(ERROR) statements in this patch?
>> >>
>> > just use
>> >
>> >> DLOG(ERROR) instead?  or, are you really hoping to collect
>> >>
>> > chrome_debug.log
>> >
>> >> files from end users?
>> >>
>> >
>> > Right; I do want logging if we can't load the actual plug-in, but the
>> > replacement plug-in won't ever fail as long as it is a WebViewPlugin.
>> >
>>
>
>  It just seems like there is some extraneous release-mode logging.  You
>> should only need to output one line in the log file if a plugin cannot be
>> loaded.
>> Ideally, it should give some indication of why the plugin could not be
>> loaded,
>> right?
>>
>
> That's what I'm trying to do. Apart from WebPluginImpl and
> ppapi::WebPluginImpl,
> where I have removed them, are there any other places in this CL where I
> missed
> extraneous LOG statements?
>
>
>  I just think we should be mindful of the bloatiness of LOG statements.
>>
>
>  -Darin
>>
>
>
>
>  >
>> >
>>
>
> http://codereview.chromium.****org/10093011/%3Chttp://coderev**
> iew.chromium.org/10093011/ <http://codereview.chromium.org/10093011/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/10093011/<http://codereview.chromium.org/100...
>

[commit-bot: I haz the power, 2012-04-30 20:15:24]

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

[commit-bot: I haz the power, 2012-04-30 22:37:28]

Change committed as 134620

[commit-bot: I haz the power, 2012-05-01 13:22:04]

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

[commit-bot: I haz the power, 2012-05-01 14:49:03]

Change committed as 134704

[stuartmorgan, 2012-05-01 17:52:37]

On 2012/04/23 22:44:08, John Abd-El-Malek wrote:
> has this gone through UI review? do we know how often this happens, i.e. that
> it's worth adding this UI for?

FWIW, on the Mac this will happen for any plugin that requires a drawing or
event model that we don't support. Once we drop the oldest ones there will be
some uncommon-but-not-fringe plugins that fall into that case, and on 10.5
currently there are some that require a new model we only support on 10.6+.

[Bernhard Bauer, 2012-05-02 08:54:48]

On 2012/05/01 17:52:37, stuartmorgan wrote:
> On 2012/04/23 22:44:08, John Abd-El-Malek wrote:
> > has this gone through UI review? do we know how often this happens, i.e.
that
> > it's worth adding this UI for?
> 
> FWIW, on the Mac this will happen for any plugin that requires a drawing or
> event model that we don't support. Once we drop the oldest ones there will be
> some uncommon-but-not-fringe plugins that fall into that case, and on 10.5
> currently there are some that require a new model we only support on 10.6+.

Ohhh, thanks for the info! Just to clarify, would this manifest as
WebPluginDelegateImpl ::PlatformInitialize() returning false?