[Android WebView] Make InputStreamImpl::Read fill the IOBuffer.

[Android WebView] Make InputStreamImpl::Read fill the IOBuffer.

This CL improves the performances of InputStreamImpl::Read. The
previous implementation was limiting each Read to transfer at most
4k. This CL makes it possible to fill, in a single run, the IOBuffer
up to the length provided by the caller.
In practice this will reduces the number of thread hops and IPCs.
Note: the max transfer size, at this point, is imited onlt by the
kMaxAllocationSize constant defined in async_resounrce_handler.cc.

BUG=294367
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224056

[joth, 2013-09-18 20:05:59]

seems reasonable, but I didn't read every single line...

(this has unit tests.... is there anything we should add to them to cover this
change?)
Thanks

https://codereview.chromium.org/24082006/diff/1/android_webview/native/input_...
File android_webview/native/input_stream_impl.cc (right):

https://codereview.chromium.org/24082006/diff/1/android_webview/native/input_...
android_webview/native/input_stream_impl.cc:103: if (!eof) {
I'd find this easier as:

if (transfer_length < 0)  // EOF
  break;


then you can drop the "global" eof flag

[mkosiba (inactive), 2013-09-18 21:14:16]

LGTM

Thanks for fixing my slow code :)

https://codereview.chromium.org/24082006/diff/1/android_webview/native/input_...
File android_webview/native/input_stream_impl.cc (right):

https://codereview.chromium.org/24082006/diff/1/android_webview/native/input_...
android_webview/native/input_stream_impl.cc:103: if (!eof) {
On 2013/09/18 20:06:00, joth wrote:
> I'd find this easier as:
> 
> if (transfer_length < 0)  // EOF
>   break;
> 
> 
> then you can drop the "global" eof flag

+1

https://codereview.chromium.org/24082006/diff/1/android_webview/native/input_...
android_webview/native/input_stream_impl.cc:105: // These DCHECKs are in place
to help Chromium developers in case of bugs.
nit: you can just remove this comment. everyone knows what DCHECKS are for :)

https://codereview.chromium.org/24082006/diff/1/android_webview/native/input_...
android_webview/native/input_stream_impl.cc:125: // bytes_read can be strictly
less than the req. length if EOF is encountered.
maybe add yet another DCHECK as well?

DCHECK(bytes_read <= length);

[Primiano Tucci (use gerrit), 2013-09-19 01:01:04]

https://codereview.chromium.org/24082006/diff/1/android_webview/native/input_...
File android_webview/native/input_stream_impl.cc (right):

https://codereview.chromium.org/24082006/diff/1/android_webview/native/input_...
android_webview/native/input_stream_impl.cc:103: if (!eof) {
On 2013/09/18 21:14:17, mkosiba wrote:
> On 2013/09/18 20:06:00, joth wrote:
> > I'd find this easier as:
> > 
> > if (transfer_length < 0)  // EOF
> >   break;
> > 
> > 
> > then you can drop the "global" eof flag
> 
> +1

Hmm to be honest I did find a bit more readable having 
"while (remaining_length > 0 && !eof) " at the beginning of the loop, which
makes more evident the fact that the method can either fill the buffer or return
earlier if EOF is reached.
But apparently, we are 2:1, so let's remove the eof field :)

[joth, 2013-09-19 01:11:58]

lgtm

https://codereview.chromium.org/24082006/diff/16001/android_webview/native/in...
File android_webview/native/input_stream_impl.cc (right):

https://codereview.chromium.org/24082006/diff/16001/android_webview/native/in...
android_webview/native/input_stream_impl.cc:99: break;
nit: I'd put \n after each these early-outs

https://codereview.chromium.org/24082006/diff/16001/android_webview/native/in...
android_webview/native/input_stream_impl.cc:103: if (transfer_length > 0) {
heh, we could go further on this line

if (transfer_length == 0)
  continue;

main reason I suggest is it more tightly ties the statement to the comment
directly above it.

https://codereview.chromium.org/24082006/diff/16001/android_webview/native/in...
android_webview/native/input_stream_impl.cc:107: #endif // NDEBUG
the "#ifndef NDEBUG" guard not needed here now. (it was to guard the
GetArrayLength(buffer) call but you're doing that inline so can just simplify
it)

[Primiano Tucci (use gerrit), 2013-09-19 01:48:57]

https://codereview.chromium.org/24082006/diff/16001/android_webview/native/in...
File android_webview/native/input_stream_impl.cc (right):

https://codereview.chromium.org/24082006/diff/16001/android_webview/native/in...
android_webview/native/input_stream_impl.cc:99: break;
On 2013/09/19 01:11:58, joth wrote:
> nit: I'd put \n after each these early-outs 

Done.

https://codereview.chromium.org/24082006/diff/16001/android_webview/native/in...
android_webview/native/input_stream_impl.cc:103: if (transfer_length > 0) {
On 2013/09/19 01:11:58, joth wrote:
> heh, we could go further on this line
> 
> if (transfer_length == 0)
>   continue;
> 
> main reason I suggest is it more tightly ties the statement to the comment
> directly above it.

Done.

https://codereview.chromium.org/24082006/diff/16001/android_webview/native/in...
android_webview/native/input_stream_impl.cc:107: #endif // NDEBUG
On 2013/09/19 01:11:58, joth wrote:
> the "#ifndef NDEBUG" guard not needed here now. (it was to guard the
> GetArrayLength(buffer) call but you're doing that inline so can just simplify
> it)

Done.

[commit-bot: I haz the power, 2013-09-19 01:54:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/primiano@chromium.org/24082006/22001

[mkosiba (inactive), 2013-09-19 01:58:01]

https://codereview.chromium.org/24082006/diff/22001/android_webview/native/in...
File android_webview/native/input_stream_unittest.cc (right):

https://codereview.chromium.org/24082006/diff/22001/android_webview/native/in...
android_webview/native/input_stream_unittest.cc:110: const int bytes_requested =
16 * 1024;
sorry for the late feedback. could we maybe express this in multiples of
kBufferSize? Then the tests will continue to test the same thing if we ever 
increase the kBufferSize

[Primiano Tucci (use gerrit), 2013-09-19 01:58:54]

https://codereview.chromium.org/24082006/diff/22001/android_webview/native/in...
File android_webview/native/input_stream_unittest.cc (right):

https://codereview.chromium.org/24082006/diff/22001/android_webview/native/in...
android_webview/native/input_stream_unittest.cc:110: const int bytes_requested =
16 * 1024;
On 2013/09/19 01:58:01, mkosiba wrote:
> sorry for the late feedback. could we maybe express this in multiples of
> kBufferSize? Then the tests will continue to test the same thing if we ever 
> increase the kBufferSize

Done.

[commit-bot: I haz the power, 2013-09-19 01:59:15]

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

[commit-bot: I haz the power, 2013-09-19 06:17:55]

Change committed as 224056