Record received data in WebViewPlugin and replay it when loading the real plugin.

Record received data in WebViewPlugin and replay it when loading the real plugin.

BUG=49686
TEST=Block all plugins, directly open a page that is displayed by a plugin (an SWF file, or a PDF if the plugin works), and click on the placeholder. The plugin should load normally.

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

[Bernhard Bauer, 2010-07-26 17:18:08]

Please review.

[Bernhard Bauer, 2010-07-30 17:40:13]

On 2010/07/26 17:18:08, Bernhard Bauer wrote:
> Please review.

ping?

[darin (slow to review), 2010-07-30 19:05:21]

http://codereview.chromium.org/3038027/diff/2001/3002
File webkit/glue/plugins/webview_plugin.cc (right):

http://codereview.chromium.org/3038027/diff/2001/3002#newcode126
webkit/glue/plugins/webview_plugin.cc:126: response_.reset(new
WebURLResponse(response));
WebURLResponse is itself a smart pointer.  you don't need to allocate a new
WebURLResponse here.

just do:  response_ = response;

and make response_ be of type |WebURLResponse|.

http://codereview.chromium.org/3038027/diff/2001/3002#newcode130
webkit/glue/plugins/webview_plugin.cc:130: data_.append(data, data_length);
hmm... plugin documents can be quite large, especially PDFs.

we might want to invest in a more efficient container here.  maybe just using a
std::deque would be enough.

[Bernhard Bauer, 2010-07-30 21:06:24]

On 2010/07/30 19:05:21, darin wrote:
> http://codereview.chromium.org/3038027/diff/2001/3002
> File webkit/glue/plugins/webview_plugin.cc (right):
> 
> http://codereview.chromium.org/3038027/diff/2001/3002#newcode126
> webkit/glue/plugins/webview_plugin.cc:126: response_.reset(new
> WebURLResponse(response));
> WebURLResponse is itself a smart pointer.  you don't need to allocate a new
> WebURLResponse here.
> 
> just do:  response_ = response;
> 
> and make response_ be of type |WebURLResponse|.

done.

> http://codereview.chromium.org/3038027/diff/2001/3002#newcode130
> webkit/glue/plugins/webview_plugin.cc:130: data_.append(data, data_length);
> hmm... plugin documents can be quite large, especially PDFs.
> 
> we might want to invest in a more efficient container here.  maybe just using
a
> std::deque would be enough.

done.

[darin (slow to review), 2010-07-30 22:18:26]

http://codereview.chromium.org/3038027/diff/8001/9001
File webkit/glue/plugins/webview_plugin.cc (right):

http://codereview.chromium.org/3038027/diff/8001/9001#newcode60
webkit/glue/plugins/webview_plugin.cc:60: plugin->didReceiveData(buf.get(),
length);
oh, hmm... when i suggested using std::deque, i didn't realize it would imply
having to make a copy of the fully formed buffer here.  another idea is to just
keep a list of strings, and append a string for each incoming
WebViewPlugin::didReceiveData call.

i think this issue deserves some optimization attention since large plugin
documents (PDFs) are quite common.  we may even need to consider spooling these
files to disk if they get too large.

this may be something to measure with a histogram so we can see how bad the
problem is.

[Bernhard Bauer, 2010-07-31 17:45:07]

On 2010/07/30 22:18:26, darin wrote:
> http://codereview.chromium.org/3038027/diff/8001/9001
> File webkit/glue/plugins/webview_plugin.cc (right):
> 
> http://codereview.chromium.org/3038027/diff/8001/9001#newcode60
> webkit/glue/plugins/webview_plugin.cc:60: plugin->didReceiveData(buf.get(),
> length);
> oh, hmm... when i suggested using std::deque, i didn't realize it would imply
> having to make a copy of the fully formed buffer here.  another idea is to
just
> keep a list of strings, and append a string for each incoming
> WebViewPlugin::didReceiveData call.

ok, I used a std::list<std::string> this time.

> i think this issue deserves some optimization attention since large plugin
> documents (PDFs) are quite common.  we may even need to consider spooling
these
> files to disk if they get too large.
> 
> this may be something to measure with a histogram so we can see how bad the
> problem is.

I added histograms for the total amount of data in KiB, and for the number of
data chunks recorded.

I'm not sure how big of a problem memory is going to be, because the plugin has
to keep the document in memory anyway, so my inner computer scientist says we
still take O(n) memory ;-)

Come to think of it, we might even be able to delete each data chunk immediately
after feeding it to the plugin, so we would free up memory in the webviewplugin
simultaneously with allocating it in the real plugin.

[darin (slow to review), 2010-07-31 23:14:13]

LGTM

A traditional plugin could spool to disk and use a temporary file.  Pepper
plugins will be able to do that as well.  I don't know that any plugins actually
do that though ;-)

(I should have suggested a std::queue instead of a std::list since you only need
queue-like functionality.)

[darin (slow to review), 2010-07-31 23:15:56]

Oh, one more thought:

The tight didReceiveData loop may result in some significant "jank" as the
main thread of WebKit is blocked until all of the plugin data is pushed into
the plugin.  It may be a good idea to break it up with some PostTask calls
so that we preserve the responsiveness of the WebKit and plugin main thread.
 This can be a follow-up CL.

-Darin


On Sat, Jul 31, 2010 at 4:14 PM, <darin@chromium.org> wrote:

> LGTM
>
> A traditional plugin could spool to disk and use a temporary file.  Pepper
> plugins will be able to do that as well.  I don't know that any plugins
> actually
> do that though ;-)
>
> (I should have suggested a std::queue instead of a std::list since you only
> need
> queue-like functionality.)
>
>
> http://codereview.chromium.org/3038027/show
>

[Bernhard Bauer, 2010-08-02 15:34:35]

On 2010/07/31 23:15:56, darin wrote:
> Oh, one more thought:
> 
> The tight didReceiveData loop may result in some significant "jank" as the
> main thread of WebKit is blocked until all of the plugin data is pushed into
> the plugin.  It may be a good idea to break it up with some PostTask calls
> so that we preserve the responsiveness of the WebKit and plugin main thread.
>  This can be a follow-up CL.

How can I make sure the plugin still exists when the task is executed? The only
thing coming to my mind is calling ref() on the WebPluginContainer and deref()
afterwards, but that'd involve digging around in WebKit internals (because
WebPluginContainer doesn't inherit from RefCounted, only WebPluginContainerImpl
does) and seems too hackish to me.

[darin (slow to review), 2010-08-02 16:55:26]

On Mon, Aug 2, 2010 at 8:34 AM, <bauerb@chromium.org> wrote:

> On 2010/07/31 23:15:56, darin wrote:
>
>> Oh, one more thought:
>>
>
>  The tight didReceiveData loop may result in some significant "jank" as the
>> main thread of WebKit is blocked until all of the plugin data is pushed
>> into
>> the plugin.  It may be a good idea to break it up with some PostTask calls
>> so that we preserve the responsiveness of the WebKit and plugin main
>> thread.
>>  This can be a follow-up CL.
>>
>
> How can I make sure the plugin still exists when the task is executed? The
> only
> thing coming to my mind is calling ref() on the WebPluginContainer and
> deref()
> afterwards, but that'd involve digging around in WebKit internals (because
> WebPluginContainer doesn't inherit from RefCounted, only
> WebPluginContainerImpl
> does) and seems too hackish to me.


This is a problem solved by ScopedRunnableMethodFactory<T>.

-Darin



>
>
> http://codereview.chromium.org/3038027/show
>