This CB fixes the following issues:-...

This CB fixes the following issue1. http://code.google.com/p/chromium/issues/detail?id=206This is a performance issue while loading PDF documents. The fix is to support PDF fast webview, which is basically support for the NPN_RequestRead API, which allows a plugin to request specific byte ranges in HTTP GET requests. This also needs support for seekable streams. Our support for seekable streams is limited to HTTP servers which allow byte range requests. Firefox also supports a mode in which the the browser caches the file on disk for servers which don't support byte range requests. The plugin_data_stream.cc/.h files are being removed as there is not much value in their existence. The needed functionality is available in the PluginStreamUrl class, which now services manual data streams as well. Testing this is a touch tricky as we need a HTTP server which serves byte range requests. Will add those in a subsequent CB.Also fixed a bug in the multipart parser where we need to ignore leading newline characters while parsing the header.

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

[jam, 2008-09-17 21:27:42]

Since this isn't a small change, can you split this up into separate
changelists, one for each issue?

[ananta, 2008-09-17 21:54:12]

Done. Please take another look.

Thanks
Ananta

[jam, 2008-09-17 23:54:08]

cool, some comments:

http://codereview.chromium.org/2896/diff/225/48
File chrome/common/plugin_messages_internal.h (right):

http://codereview.chromium.org/2896/diff/225/48#newcode147
Line 147: int /* Offset of data */)
use lower case 'o' for consistency, also make this use underscores, as we
usually put the same name as the function parameter.  same for below.

http://codereview.chromium.org/2896/diff/225/42
File webkit/build/glue/glue.vcproj (left):

http://codereview.chromium.org/2896/diff/225/42#oldcode610
Line 610: </File>
you need to update the scons file as well

http://codereview.chromium.org/2896/diff/225/39
File webkit/glue/plugins/plugin_instance.cc (right):

http://codereview.chromium.org/2896/diff/225/39#newcode370
Line 370: scoped_refptr<PluginStreamUrl>
local_stream_ref(plugin_data_stream_.get());
why is this needed?

http://codereview.chromium.org/2896/diff/225/39#newcode459
Line 459: webplugin_->CancelDocumentLoad();
why can't we have multiple concurrent reads?

http://codereview.chromium.org/2896/diff/225/39#newcode472
Line 472: stream_index != open_streams_.end(); ++stream_index) {
this should be aligned with the stream_index above)

http://codereview.chromium.org/2896/diff/225/40
File webkit/glue/plugins/plugin_instance.h (right):

http://codereview.chromium.org/2896/diff/225/40#newcode244
Line 244: scoped_refptr<PluginStreamUrl>            plugin_data_stream_;
spacing is off

http://codereview.chromium.org/2896/diff/225/38
File webkit/glue/plugins/plugin_stream.h (right):

http://codereview.chromium.org/2896/diff/225/38#newcode62
Line 62: bool seekable() const {
can you put all these simple getters/setters on one line (i.e. like open() or
instance() )

http://codereview.chromium.org/2896/diff/225/38#newcode124
Line 124: // Set to true if we own the NPStream.
this comment seems unrelated to the variable below

http://codereview.chromium.org/2896/diff/225/35
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/2896/diff/225/35#newcode1010
Line 1010: // A better way of doing this.
can you do this now?  all this casting is a little hard to read and looks a
little hairy to maintain...

http://codereview.chromium.org/2896/diff/225/41
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/2896/diff/225/41#newcode71
Line 71: size_t byte_range_lb_start_offset =
it's not clear that lb is lower bound...

http://codereview.chromium.org/2896/diff/225/41#newcode98
Line 98: }
this parsing code should be separated into a function that has a unit test..

http://codereview.chromium.org/2896/diff/225/41#newcode594
Line 594: // starting to receive data.
I thought we said this stuff (and below) will be in a separate cl?

http://codereview.chromium.org/2896/diff/225/41#newcode1139
Line 1139: if (range_info) {
no need for the extra brackets

http://codereview.chromium.org/2896/diff/225/41#newcode1213
Line 1213: 
this parsing code should go into another function with unit tests

http://codereview.chromium.org/2896/diff/225/28
File webkit/glue/webplugin_impl.h (right):

http://codereview.chromium.org/2896/diff/225/28#newcode250
Line 250: // Handles HTTP byte range requests issued by the plugin. The
range_info
The comments should be only listed once in webplugin.h

[ananta, 2008-09-18 07:34:12]

Hi John

I have updated the CB as per your comments. Added unit tests
for the multipart parsing stuff.

We match Firefox behavior in canceling existing requests on streams when the
plugin invokes NPN_RequestRead. The manual stream is a special case as it is not
initiated by the plugin and instead the data comes in from webkit.

Thanks
Ananta

[john.abdelmalek, 2008-09-18 20:46:55]

lgtm looks much better, you can check in after fixing these nits

http://codereview.chromium.org/2896/diff/289/106
File chrome/common/plugin_messages_internal.h (right):

http://codereview.chromium.org/2896/diff/289/106#newcode252
Line 252: std::string /* range info */,
add underscores here too

http://codereview.chromium.org/2896/diff/289/110
File webkit/glue/multipart_response_delegate.cc (right):

http://codereview.chromium.org/2896/diff/289/110#newcode301
Line 301: StringToInt(byte_range_upper_bound, content_range_upper_bound);
should check the return value of both calls to StringToInt and return false on
error.

http://codereview.chromium.org/2896/diff/289/93
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/2896/diff/289/93#newcode1010
Line 1010: // A better way of doing this.
you can take out this comment now