[android_webview] Don't block the IO thread when reading from an InputStream.

[android_webview] Don't block the IO thread when reading from an InputStream.

This breaks up the functionality in the AndroidStreamReader..Job into
three separate classes, adds native unittests and makes the Job read
the InputStream on a background thread.
This change adds a separate unittestjava folder because the code under
javatests can't be compiled to be a part of a native unittest APK due to
resource dependencies.

TEST=AndroidWebviewTests,android_webview_unittests
BUG=None

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

[joth, 2012-11-07 16:55:38]

I didn't read every line in massive detail, but got some more general thoughts
on it.

https://codereview.chromium.org/11363123/diff/1/android_webview/unittestjava/...
File
android_webview/unittestjava/src/org/chromium/android_webview/unittest/InputStreamUnittest.java
(right):

https://codereview.chromium.org/11363123/diff/1/android_webview/unittestjava/...
android_webview/unittestjava/src/org/chromium/android_webview/unittest/InputStreamUnittest.java:5:
package org.chromium.android_webview.unittest;
this file does not define a unit test, it's defining support classes for native
tests.

How about we call the package / target / class unittest_support or something? 

also - given it's such a small bit of code, I'd not be adverse to it appearing
in the production JAR, but now you've set it up properly we may as well keep it
this way :)

https://codereview.chromium.org/11363123/diff/19/android_webview/native/andro...
File android_webview/native/android_stream_reader_url_request_job.cc (right):

https://codereview.chromium.org/11363123/diff/19/android_webview/native/andro...
android_webview/native/android_stream_reader_url_request_job.cc:8: #include
"android_webview/native/input_stream_reader.h"
hmm interesting... if input_stream_reader could be moved to aw/browser then
android_stream_reader_url_request_job could too. i.e. only input_stream need
JNI.

https://codereview.chromium.org/11363123/diff/19/android_webview/native/andro...
android_webview/native/android_stream_reader_url_request_job.cc:29: 
would be helpful to have a block comment explaining how the  thread bouncing is
handled in this file and its close collaborators (but read my other comments
first).

https://codereview.chromium.org/11363123/diff/19/android_webview/native/andro...
android_webview/native/android_stream_reader_url_request_job.cc:34:
WorkerThread() : base::Thread("AndroidStreamReaderWorkerThread") {
nit: Worker seems spurious: AndroidStreamReaderThread is pretty clear it's the
thread that we read from streams on.

https://codereview.chromium.org/11363123/diff/19/android_webview/native/andro...
android_webview/native/android_stream_reader_url_request_job.cc:39:
LAZY_INSTANCE_INITIALIZER;
I'm not thrilled with another thread hanging out as a global here... I guess on
the plus side we'll only create it if the app actually does override the
shouldInterceptRequest method?

I presume you can't use the BrowserThread::PostBlockingPoolTaskAndReply for
these? It will allow thread reuse, and may also aid readability (and help keep
the InputStreamReader a bit less hard-wired into a given threading modal)

https://codereview.chromium.org/11363123/diff/19/android_webview/native/andro...
android_webview/native/android_stream_reader_url_request_job.cc:88: // TODO:
This could be done in the InputStreamReader but would force more
is this really a TODO then? reads like a conscious trade off decision (which is
fine)

https://codereview.chromium.org/11363123/diff/19/android_webview/native/andro...
android_webview/native/android_stream_reader_url_request_job.cc:99:
CHECK(input_stream_reader_);
nit dcheck

https://codereview.chromium.org/11363123/diff/19/android_webview/native/andro...
File android_webview/native/android_stream_reader_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/11363123/diff/19/android_webview/native/andro...
android_webview/native/android_stream_reader_url_request_job_unittest.cc:114:
class TestStreamReaderJob : public AndroidStreamReaderURLRequestJob {
this is tricky to follow: you're subclassing the thingy under test (which is
nice to avoid if you can)m but also double subclassing it's direct dependency
(InputStreamReader) and subclassin that class's delegate, and then also
subclassing the transitive dependency InputStream.
Some docs for why each of these testing seams is needed might help, but it's a
lot of test-only doubles for the next person to have to wade through.

https://codereview.chromium.org/11363123/diff/19/android_webview/native/input...
File android_webview/native/input_stream_reader.cc (right):

https://codereview.chromium.org/11363123/diff/19/android_webview/native/input...
android_webview/native/input_stream_reader.cc:12: 
ISTM this class is now fairly well abstracted from the JNI goop so could live in
aw/browser (if we also define a input_stream abstract base class in there).

https://codereview.chromium.org/11363123/diff/19/android_webview/native/input...
android_webview/native/input_stream_reader.cc:118:
BrowserThread::PostTask(BrowserThread::IO,
this class is a little bit shady, that you can call into it on "any" arbitrary
(single) thread, but it will always call you back on the IO thread.
I'd rather we make it symmetrical and put all the thread-hopping in one place -
either inside or outside this thread, not sure which I prefer.

[benm (inactive), 2012-11-15 17:39:01]

http://codereview.chromium.org/11363123/diff/19/android_webview/native/androi...
File android_webview/native/android_stream_reader_url_request_job.cc (right):

http://codereview.chromium.org/11363123/diff/19/android_webview/native/androi...
android_webview/native/android_stream_reader_url_request_job.cc:127: void
AndroidStreamReaderURLRequestJob::OnReaderReadCompleted(
Please add a comment to explain the differences between NotifyDone and
NotifyReadComplete, especially why we don't call NotifyDone in the case of a >0
result code. I understand it's probably due to something deep in the net stack
so it doesn't need to be much here but right now it looks a bit like something
was forgotten.

Maybe move the ReadComplete to the top of the function?

http://codereview.chromium.org/11363123/diff/19/android_webview/native/androi...
android_webview/native/android_stream_reader_url_request_job.cc:147:
CHECK(thread_created);
DCHECK?

http://codereview.chromium.org/11363123/diff/19/android_webview/native/input_...
File android_webview/native/input_stream.h (right):

http://codereview.chromium.org/11363123/diff/19/android_webview/native/input_...
android_webview/native/input_stream.h:19: static const int kBufferSize;
could this be a local const in input_stream.cc?

http://codereview.chromium.org/11363123/diff/19/android_webview/native/input_...
File android_webview/native/input_stream_reader.cc (right):

http://codereview.chromium.org/11363123/diff/19/android_webview/native/input_...
android_webview/native/input_stream_reader.cc:118:
BrowserThread::PostTask(BrowserThread::IO,
Agree, it'd be nice if you're always called back on the thread that you call
into it from. So here we'd hop back to the worker thread, and the worker thread
would transfer to the IO thread? Comments in the header would be simpler too.

WDYT?

http://codereview.chromium.org/11363123/diff/19/android_webview/native/input_...
File android_webview/native/input_stream_reader.h (right):

http://codereview.chromium.org/11363123/diff/19/android_webview/native/input_...
android_webview/native/input_stream_reader.h:21: class InputStreamReader {
As joth noted looks like we could abstract the public api into aw/browser and
leave an impl in native that access JNI?

[mkosiba (inactive), 2012-11-15 19:18:50]

new version out.

http://codereview.chromium.org/11363123/diff/19/android_webview/native/androi...
File android_webview/native/android_stream_reader_url_request_job.cc (right):

http://codereview.chromium.org/11363123/diff/19/android_webview/native/androi...
android_webview/native/android_stream_reader_url_request_job.cc:8: #include
"android_webview/native/input_stream_reader.h"
On 2012/11/07 16:55:38, joth wrote:
> hmm interesting... if input_stream_reader could be moved to aw/browser then
> android_stream_reader_url_request_job could too. i.e. only input_stream need
> JNI.

yup, it's just me being lazy. I'll do the move.

http://codereview.chromium.org/11363123/diff/19/android_webview/native/androi...
android_webview/native/android_stream_reader_url_request_job.cc:29: 
On 2012/11/07 16:55:38, joth wrote:
> would be helpful to have a block comment explaining how the  thread bouncing
is
> handled in this file and its close collaborators (but read my other comments
> first).

simplified this significantly - you still think one is needed?

http://codereview.chromium.org/11363123/diff/19/android_webview/native/androi...
android_webview/native/android_stream_reader_url_request_job.cc:34:
WorkerThread() : base::Thread("AndroidStreamReaderWorkerThread") {
On 2012/11/07 16:55:38, joth wrote:
> nit: Worker seems spurious: AndroidStreamReaderThread is pretty clear it's the
> thread that we read from streams on.

Done.

http://codereview.chromium.org/11363123/diff/19/android_webview/native/androi...
android_webview/native/android_stream_reader_url_request_job.cc:39:
LAZY_INSTANCE_INITIALIZER;
On 2012/11/07 16:55:38, joth wrote:
> I'm not thrilled with another thread hanging out as a global here... I guess
on
> the plus side we'll only create it if the app actually does override the
> shouldInterceptRequest method?
> 
> I presume you can't use the BrowserThread::PostBlockingPoolTaskAndReply for
> these? It will allow thread reuse, and may also aid readability (and help keep
> the InputStreamReader a bit less hard-wired into a given threading modal)

I didn't know about the PostBlockingPoolTask method. There is a sequenced
version available too but I'm starting to think we should try a parallel
approach first and scale down to single-thread if we hit issues.

http://codereview.chromium.org/11363123/diff/19/android_webview/native/androi...
android_webview/native/android_stream_reader_url_request_job.cc:88: // TODO:
This could be done in the InputStreamReader but would force more
On 2012/11/07 16:55:38, joth wrote:
> is this really a TODO then? reads like a conscious trade off decision (which
is
> fine)

ok, removed TODO.

http://codereview.chromium.org/11363123/diff/19/android_webview/native/androi...
android_webview/native/android_stream_reader_url_request_job.cc:99:
CHECK(input_stream_reader_);
On 2012/11/07 16:55:38, joth wrote:
> nit dcheck

I wanted a CHECK since we'll end up crashing anyway, but if we crash while
executing the Closure the callstack will be useless.

http://codereview.chromium.org/11363123/diff/19/android_webview/native/androi...
android_webview/native/android_stream_reader_url_request_job.cc:147:
CHECK(thread_created);
On 2012/11/15 17:39:01, benm wrote:
> DCHECK?

removed the code.

http://codereview.chromium.org/11363123/diff/19/android_webview/native/androi...
File android_webview/native/android_stream_reader_url_request_job_unittest.cc
(right):

http://codereview.chromium.org/11363123/diff/19/android_webview/native/androi...
android_webview/native/android_stream_reader_url_request_job_unittest.cc:114:
class TestStreamReaderJob : public AndroidStreamReaderURLRequestJob {
On 2012/11/07 16:55:38, joth wrote:
> this is tricky to follow: you're subclassing the thingy under test (which is
> nice to avoid if you can)m but also double subclassing it's direct dependency
> (InputStreamReader) and subclassin that class's delegate, and then also
> subclassing the transitive dependency InputStream.
> Some docs for why each of these testing seams is needed might help, but it's a
> lot of test-only doubles for the next person to have to wade through.

these unittests are really more like integration tests. I'll try and simplify
the whole thing and see how much of these I can remove.

http://codereview.chromium.org/11363123/diff/19/android_webview/native/input_...
File android_webview/native/input_stream_reader.cc (right):

http://codereview.chromium.org/11363123/diff/19/android_webview/native/input_...
android_webview/native/input_stream_reader.cc:12: 
On 2012/11/07 16:55:38, joth wrote:
> ISTM this class is now fairly well abstracted from the JNI goop so could live
in
> aw/browser (if we also define a input_stream abstract base class in there).

Done.

http://codereview.chromium.org/11363123/diff/19/android_webview/native/input_...
android_webview/native/input_stream_reader.cc:118:
BrowserThread::PostTask(BrowserThread::IO,
On 2012/11/07 16:55:38, joth wrote:
> this class is a little bit shady, that you can call into it on "any" arbitrary
> (single) thread, but it will always call you back on the IO thread.
> I'd rather we make it symmetrical and put all the thread-hopping in one place
-
> either inside or outside this thread, not sure which I prefer.

Done.

http://codereview.chromium.org/11363123/diff/13002/android_webview/native/inp...
File android_webview/native/input_stream_impl.cc (right):

http://codereview.chromium.org/11363123/diff/13002/android_webview/native/inp...
android_webview/native/input_stream_impl.cc:33: return reinterpret_cast<const
InputStreamImpl*>(input_stream);
so according to the standard this is undefined behavior, and the alternative is
to add the jobj() getter to the browser/input_stream.h interface.

[joth, 2012-11-15 20:04:26]

http://codereview.chromium.org/11363123/diff/13002/android_webview/native/inp...
File android_webview/native/input_stream_impl.cc (right):

http://codereview.chromium.org/11363123/diff/13002/android_webview/native/inp...
android_webview/native/input_stream_impl.cc:33: return reinterpret_cast<const
InputStreamImpl*>(input_stream);
On 2012/11/15 19:18:51, Martin Kosiba wrote:
> so according to the standard this is undefined behavior, and the alternative
is
> to add the jobj() getter to the browser/input_stream.h interface.

use a static_cast, and it's totally legit (unless I'm missing something).
See also http://go/down_cast (unfortunately we don't have one of those as we
don't have rtti even in debug builds, afaik)

[mkosiba (inactive), 2012-11-16 14:47:01]

https://chromiumcodereview.appspot.com/11363123/diff/13002/android_webview/na...
File android_webview/native/input_stream_impl.cc (right):

https://chromiumcodereview.appspot.com/11363123/diff/13002/android_webview/na...
android_webview/native/input_stream_impl.cc:33: return reinterpret_cast<const
InputStreamImpl*>(input_stream);
On 2012/11/15 20:04:26, joth wrote:
> On 2012/11/15 19:18:51, Martin Kosiba wrote:
> > so according to the standard this is undefined behavior, and the alternative
> is
> > to add the jobj() getter to the browser/input_stream.h interface.
> 
> use a static_cast, and it's totally legit (unless I'm missing something).
> See also http://go/down_cast (unfortunately we don't have one of those as we
> don't have rtti even in debug builds, afaik)
> 

duh.. I tried using a static_cast before and gcc complained about not being able
to cast one pointer to the other. I tried it now and it works fine... I guess I
was missing some type definitions or something..

[benm (inactive), 2012-11-16 15:43:19]

http://codereview.chromium.org/11363123/diff/21001/android_webview/browser/in...
File android_webview/browser/input_stream.h (right):

http://codereview.chromium.org/11363123/diff/21001/android_webview/browser/in...
android_webview/browser/input_stream.h:39: virtual bool Read(JNIEnv* env,
would be nice to eliminate the JNIEnvs here, can they go into the native/ impl
(just call AttachCurrentThread()?)

http://codereview.chromium.org/11363123/diff/21001/android_webview/browser/ne...
File android_webview/browser/net/input_stream_reader.h (right):

http://codereview.chromium.org/11363123/diff/21001/android_webview/browser/ne...
android_webview/browser/net/input_stream_reader.h:53: int
VerifyRequestedRange(JNIEnv* env,
same comment, can we remove jni from the browser/ layer?

http://codereview.chromium.org/11363123/diff/21001/android_webview/native/inp...
File android_webview/native/input_stream_unittest.cc (right):

http://codereview.chromium.org/11363123/diff/21001/android_webview/native/inp...
android_webview/native/input_stream_unittest.cc:47: ScopedJavaLocalRef<jobject>
empty_jstream =
counting_jstream ?

http://codereview.chromium.org/11363123/diff/21001/android_webview/native/inp...
android_webview/native/input_stream_unittest.cc:98: ASSERT_EQ(i,
buffer->data()[i]);
EXPECT?

[mkosiba (inactive), 2012-11-19 15:29:33]

https://chromiumcodereview.appspot.com/11363123/diff/21001/android_webview/br...
File android_webview/browser/input_stream.h (right):

https://chromiumcodereview.appspot.com/11363123/diff/21001/android_webview/br...
android_webview/browser/input_stream.h:39: virtual bool Read(JNIEnv* env,
On 2012/11/16 15:43:20, benm wrote:
> would be nice to eliminate the JNIEnvs here, can they go into the native/ impl
> (just call AttachCurrentThread()?)

Done.

https://chromiumcodereview.appspot.com/11363123/diff/21001/android_webview/br...
File android_webview/browser/net/input_stream_reader.h (right):

https://chromiumcodereview.appspot.com/11363123/diff/21001/android_webview/br...
android_webview/browser/net/input_stream_reader.h:53: int
VerifyRequestedRange(JNIEnv* env,
On 2012/11/16 15:43:20, benm wrote:
> same comment, can we remove jni from the browser/ layer?

Done.

https://chromiumcodereview.appspot.com/11363123/diff/21001/android_webview/na...
File android_webview/native/input_stream_unittest.cc (right):

https://chromiumcodereview.appspot.com/11363123/diff/21001/android_webview/na...
android_webview/native/input_stream_unittest.cc:47: ScopedJavaLocalRef<jobject>
empty_jstream =
On 2012/11/16 15:43:20, benm wrote:
> counting_jstream ?

Done.

https://chromiumcodereview.appspot.com/11363123/diff/21001/android_webview/na...
android_webview/native/input_stream_unittest.cc:98: ASSERT_EQ(i,
buffer->data()[i]);
On 2012/11/16 15:43:20, benm wrote:
> EXPECT?

Done.

[mkosiba (inactive), 2012-11-20 10:49:11]

ok, so looks like the latest patch fixes the double-free in unittests - care to
take another look Joth?

[joth, 2012-11-20 20:46:34]

lgtm. just nits

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/i...
File android_webview/browser/input_stream.h (right):

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/i...
android_webview/browser/input_stream.h:16: class InputStream {
nit: comment it's an abstract wrapper, typically used to access the java class
of same name. and where you'd get an instance from?

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/i...
android_webview/browser/input_stream.h:40: protected:
nit: \n before

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/android_stream_reader_url_request_job.cc:131: int
*bytes_read) {
ubernit: put '*' next to the type

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/android_stream_reader_url_request_job.cc:174: // We
only care about "Range" header here.
nit: maybe expand the comment a little about what the overall handling is we're
doing with the range header.

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/android_stream_reader_url_request_job.h:81: virtual
android_webview::InputStreamReader*
comment ownership is not returned. (I come from a world where CreateXxx always
returned ownership. maybe use a different method name if you think of one).

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
File android_webview/browser/net/input_stream_reader.cc (right):

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/input_stream_reader.cc:81: int64_t skipped,
bytes_to_skip = byte_range.first_byte_position();
nit: skipped can be declared inside loop.

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/input_stream_reader.cc:88: } while ((bytes_to_skip
-= skipped) > 0);
handle bytes_to_skip < 0

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
File android_webview/browser/net/input_stream_reader.h (right):

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/input_stream_reader.h:23: // The constructor is
called on the IO thread, not on the WorkerThread.
nit: worker thread.
(WorkerThread is a proper class name, and totally unrelated to this either way)

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/input_stream_reader.h:35: virtual int
Seek(net::HttpByteRange byte_range);
nit: pass by const&

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

https://codereview.chromium.org/11363123/diff/12025/android_webview/native/in...
android_webview/native/input_stream_impl.cc:50: DCHECK(env);
nit: never need DCHECK(env), AttachCurrentThread does it already

https://codereview.chromium.org/11363123/diff/12025/android_webview/native/in...
android_webview/native/input_stream_impl.cc:51: // TODO: Use unsafe version once
BUG 157880 is fixed.
nit: bug 160011 seems specific to this file? (multiple times: could just factor
out one TODO at top of file)

https://codereview.chromium.org/11363123/diff/12025/android_webview/native/in...
android_webview/native/input_stream_impl.cc:73: // TODO: Use unsafe version once
BUG 157880 is fixed.
don't think you mean TODO here? line 87.

https://codereview.chromium.org/11363123/diff/12025/android_webview/native/in...
android_webview/native/input_stream_impl.cc:82: int read_size = std::min(length,
kBufferSize);
move this inside the loop, else it will over-read on the final loop iteration
(when 2 or more iteration were required).

https://codereview.chromium.org/11363123/diff/12025/android_webview/native/in...
File android_webview/native/input_stream_impl.h (right):

https://codereview.chromium.org/11363123/diff/12025/android_webview/native/in...
android_webview/native/input_stream_impl.h:26: // |stream| should be an instance
of the InputStreamImpl Java class.
InputStreamImpl Java class? or InputStream Java class ?

https://codereview.chromium.org/11363123/diff/12025/android_webview/native/in...
android_webview/native/input_stream_impl.h:38: protected:
nit: \n before

[mkosiba (inactive), 2012-11-21 15:19:47]

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/i...
File android_webview/browser/input_stream.h (right):

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/i...
android_webview/browser/input_stream.h:16: class InputStream {
On 2012/11/20 20:46:35, joth wrote:
> nit: comment it's an abstract wrapper, typically used to access the java class
> of same name. and where you'd get an instance from?

Added a comment but will skip the 'where do you get an instance from' bit
because that will easily go stale (and it's pretty obvious from grepping that
there is a native input_stream_impl)

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/i...
android_webview/browser/input_stream.h:40: protected:
On 2012/11/20 20:46:35, joth wrote:
> nit: \n before

Done.

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/android_stream_reader_url_request_job.cc:131: int
*bytes_read) {
On 2012/11/20 20:46:35, joth wrote:
> ubernit: put '*' next to the type

Done.

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/android_stream_reader_url_request_job.cc:174: // We
only care about "Range" header here.
On 2012/11/20 20:46:35, joth wrote:
> nit: maybe expand the comment a little about what the overall handling is
we're
> doing with the range header.

Done.

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/android_stream_reader_url_request_job.h:81: virtual
android_webview::InputStreamReader*
On 2012/11/20 20:46:35, joth wrote:
> comment ownership is not returned. (I come from a world where CreateXxx always
> returned ownership. maybe use a different method name if you think of one).

umm.. why do you think ownership is not returned? this is a regular factory
method (in the sense that the caller must take ownership of the result), the
fact that the caller is the same class as the method is implemented shouldn't
matter.

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
File android_webview/browser/net/input_stream_reader.cc (right):

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/input_stream_reader.cc:81: int64_t skipped,
bytes_to_skip = byte_range.first_byte_position();
On 2012/11/20 20:46:35, joth wrote:
> nit: skipped can be declared inside loop.

Done.

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/input_stream_reader.cc:88: } while ((bytes_to_skip
-= skipped) > 0);
On 2012/11/20 20:46:35, joth wrote:
> handle bytes_to_skip < 0

I assume you mean handling skipped > bytes_to_skip.
Also - good spot! Made me fix a buffer overrun.

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
File android_webview/browser/net/input_stream_reader.h (right):

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/input_stream_reader.h:23: // The constructor is
called on the IO thread, not on the WorkerThread.
On 2012/11/20 20:46:35, joth wrote:
> nit: worker thread.
> (WorkerThread is a proper class name, and totally unrelated to this either
way)

Done.

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/input_stream_reader.h:35: virtual int
Seek(net::HttpByteRange byte_range);
On 2012/11/20 20:46:35, joth wrote:
> nit: pass by const&

Done.

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

https://codereview.chromium.org/11363123/diff/12025/android_webview/native/in...
android_webview/native/input_stream_impl.cc:50: DCHECK(env);
On 2012/11/20 20:46:35, joth wrote:
> nit: never need DCHECK(env), AttachCurrentThread does it already

Done.

https://codereview.chromium.org/11363123/diff/12025/android_webview/native/in...
android_webview/native/input_stream_impl.cc:51: // TODO: Use unsafe version once
BUG 157880 is fixed.
On 2012/11/20 20:46:35, joth wrote:
> nit: bug 160011 seems specific to this file? (multiple times: could just
factor
> out one TODO at top of file)

fixing 160011 would currently affect only this file. yes.

https://codereview.chromium.org/11363123/diff/12025/android_webview/native/in...
android_webview/native/input_stream_impl.cc:73: // TODO: Use unsafe version once
BUG 157880 is fixed.
On 2012/11/20 20:46:35, joth wrote:
> don't think you mean TODO here? line 87.

Done.

https://codereview.chromium.org/11363123/diff/12025/android_webview/native/in...
android_webview/native/input_stream_impl.cc:82: int read_size = std::min(length,
kBufferSize);
On 2012/11/20 20:46:35, joth wrote:
> move this inside the loop, else it will over-read on the final loop iteration
> (when 2 or more iteration were required).

right, good spot.

https://codereview.chromium.org/11363123/diff/12025/android_webview/native/in...
File android_webview/native/input_stream_impl.h (right):

https://codereview.chromium.org/11363123/diff/12025/android_webview/native/in...
android_webview/native/input_stream_impl.h:26: // |stream| should be an instance
of the InputStreamImpl Java class.
On 2012/11/20 20:46:35, joth wrote:
> InputStreamImpl Java class? or InputStream Java class ?

Done. Search and replace in comments is not always what you want.

https://codereview.chromium.org/11363123/diff/12025/android_webview/native/in...
android_webview/native/input_stream_impl.h:38: protected:
On 2012/11/20 20:46:35, joth wrote:
> nit: \n before

Done.

[commit-bot: I haz the power, 2012-11-21 18:19:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/11363123/11025

[commit-bot: I haz the power, 2012-11-21 18:20:18]

Presubmit check for 11363123-11025 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  android_webview/native/android_protocol_handler.cc:258
    static void SetResourceContextForTesting(JNIEnv* env, jclass /*clazz*/,

** Presubmit Warnings **
Your #include order seems to be broken. Send mail to
marja@chromium.org if this is not the case.
      android_webview/native/input_stream_impl.cc:13

Presubmit checks took 2.0s to calculate.

[joth, 2012-11-21 21:11:00]

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://codereview.chromium.org/11363123/diff/12025/android_webview/browser/n...
android_webview/browser/net/android_stream_reader_url_request_job.h:81: virtual
android_webview::InputStreamReader*
On 2012/11/21 15:19:47, Martin Kosiba wrote:
> On 2012/11/20 20:46:35, joth wrote:
> > comment ownership is not returned. (I come from a world where CreateXxx
always
> > returned ownership. maybe use a different method name if you think of one).
> 
> umm.. why do you think ownership is not returned? this is a regular factory
> method (in the sense that the caller must take ownership of the result), the
> fact that the caller is the same class as the method is implemented shouldn't
> matter.

I've become too used to see scoped_ptr to indicate ownership transfer; forgot
all about refcounted. (also the scoped members below confused me into thinking
ownership was retained)

- could comment why this is virtual; seems it could be static. (or indeed, why
it exists at all)
- could return scoped_refptr<> to be super clear. (until recently it was
unadvised to do that due to the implicit conversion to T*, but now that's going 
I think it makes a lot of sense to return the full owning type and makes it
nicely consistent with contemporary scoped_ptr usage patterns.)