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

android_webview/native/input_stream_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "android_webview/native/input_stream_impl.h"
 
 #include "base/android/jni_android.h"
 // Disable "Warnings treated as errors" for input_stream_jni as it's a Java
 // system class and we have to generate C++ hooks for all methods in the class
 // even if they're unused.
@@ skipping 66 matching lines @@
   JNIEnv* env = AttachCurrentThread();
   if (!buffer_.obj()) {
     // Allocate transfer buffer.
     base::android::ScopedJavaLocalRef<jbyteArray> temp(
         env, env->NewByteArray(kBufferSize));
     buffer_.Reset(temp);
     if (ClearException(env))
       return false;
   }
 
+  int remaining_length = length;
+  bool eof = false;
+  char* dest_write_ptr = dest->data();
   jbyteArray buffer = buffer_.obj();
   *bytes_read = 0;
 
-  const int read_size = std::min(length, kBufferSize);
-  int32_t byte_count;
-  do {
-    // Unfortunately it is valid for the Java InputStream to read 0 bytes some
-    // number of times before returning any more data. Because this method
-    // signals EOF by setting |bytes_read| to 0 and returning true necessary to
-    // call the Java-side read method until it returns something other than 0.
-    byte_count = Java_InputStream_readI_AB_I_I(
-        env, jobject_.obj(), buffer, 0, read_size);
+  while (remaining_length > 0 && !eof) {
+    const int max_transfer_length = std::min(remaining_length, kBufferSize);
+    const int transfer_length = Java_InputStream_readI_AB_I_I(
+        env, jobject_.obj(), buffer, 0, max_transfer_length);
     if (ClearException(env))
       return false;
-  } while (byte_count == 0);
-
-  // We've reached the end of the stream.
-  if (byte_count < 0)
-    return true;
-
+    eof = (transfer_length < 0);
+    // Note: it is possible, yet unlikely, that the Java InputStream returns
+    // a transfer_length == 0 from time to time. In such cases we just continue
+    // the read until we get either valid data or reach EOF.
+    if (!eof) {

[joth, 2013/09/18 20:06:00]

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:17]

+1

[Primiano Tucci (use gerrit), 2013/09/19 01:01:05]

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 :)

 #ifndef NDEBUG
-  int32_t buffer_length = env->GetArrayLength(buffer);
-  DCHECK_GE(read_size, byte_count);
-  DCHECK_GE(buffer_length, byte_count);
+      // These DCHECKs are in place to help Chromium developers in case of bugs.

[mkosiba (inactive), 2013/09/18 21:14:17]

nit: you can just remove this comment. everyone knows what DCHECKS are for :)

+      DCHECK_GE(max_transfer_length, transfer_length);
+      DCHECK_GE(env->GetArrayLength(buffer), transfer_length);
 #endif // NDEBUG
 
-  // The DCHECKs are in place to help Chromium developers in case of bugs,
-  // this check is to prevent a malicious InputStream implementation from
-  // overrunning the |dest| buffer.
-  if (byte_count > read_size)
-    return false;
+      // This check is to prevent a malicious InputStream implementation from
+      // overrunning the |dest| buffer.
+      if (transfer_length > max_transfer_length)
+        return false;
 
-  // Copy the data over to the provided C++ side buffer.
-  DCHECK_GE(length, byte_count);
-  env->GetByteArrayRegion(buffer, 0, byte_count,
-      reinterpret_cast<jbyte*>(dest->data() + *bytes_read));
-  if (ClearException(env))
-    return false;
-
-  *bytes_read = byte_count;
+      // Copy the data over to the provided C++ IOBuffer.
+      DCHECK_GE(remaining_length, transfer_length);
+      env->GetByteArrayRegion(buffer, 0, transfer_length,
+          reinterpret_cast<jbyte*>(dest_write_ptr));
+      if (ClearException(env))
+        return false;
+      remaining_length -= transfer_length;
+      dest_write_ptr += transfer_length;
+    }
+  }
+  // bytes_read can be strictly less than the req. length if EOF is encountered.

[mkosiba (inactive), 2013/09/18 21:14:17]

maybe add yet another DCHECK as well?

DCHECK(bytes_read <= length);

+  *bytes_read = length - remaining_length;
   return true;
 }
 
 } // namespace android_webview