Initial implementation of Cronet Async API.

components/cronet/android/cronet_url_request.cc

+// Copyright 2014 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 "components/cronet/android/cronet_url_request.h"
+
+#include "base/android/jni_android.h"
+#include "base/android/jni_string.h"
+#include "base/android/scoped_java_ref.h"
+#include "base/logging.h"
+#include "base/macros.h"
+#include "components/cronet/android/cronet_url_request_adapter.h"
+#include "components/cronet/android/cronet_url_request_context_adapter.h"
+#include "jni/CronetUrlRequest_jni.h"
+#include "net/base/net_errors.h"
+#include "net/base/request_priority.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_util.h"
+
+using base::android::ConvertUTF8ToJavaString;
+

[mmenke, 2014/11/06 16:08:26]

Maybe have a file-level comment along the lines of "This file contains all the
plumbing to handle the bidirectional communication between the Java
CronetURLRequest and C++ CronetURLRequestAdapter."

[mef, 2014/11/06 22:51:44]

Done.

+namespace cronet {
+namespace {
+
+// A delegate of CronetURLRequestAdapter that delivers callbacks to the Java
+// layer. Always called on the Network Thread.

[mmenke, 2014/11/06 16:08:26]

Maybe "Created on a Java thread, but always called and destroyed on the Network
thread?"

[mef, 2014/11/06 22:51:45]

Done.

+class JniCronetURLRequestAdapterDelegate
+    : public CronetURLRequestAdapter::CronetURLRequestAdapterDelegate {
+ public:
+  JniCronetURLRequestAdapterDelegate(JNIEnv* env, jobject owner) {
+    owner_.Reset(env, owner);
+  }
+

[mmenke, 2014/11/06 16:08:26]

// CronetURLRequestAdapter::CronetURLRequestAdapterDelegate implementation:

[mef, 2014/11/06 22:51:45]

Done.

+  void OnRedirect(const GURL& new_location, int http_status_code) override {
+    JNIEnv* env = base::android::AttachCurrentThread();
+    cronet::Java_CronetUrlRequest_onRedirect(
+        env,
+        owner_.obj(),
+        ConvertUTF8ToJavaString(env, new_location.spec()).Release(),

[mmenke, 2014/11/06 16:11:01]

Again, think this is a leak - shouldn't be releasing, since Java doesn't take
ownership, but rather grabs another reference, if needed.

[mef, 2014/11/06 17:07:46]

Good point, I think you are right, I'll change them. 
This seems to be a fairly common pattern in existing code, but that doesn't make
it correct.

[mef, 2014/11/06 22:51:45]

Done.

+        http_status_code);
+  }
+
+  void OnResponseStarted(int http_status_code) override {
+    JNIEnv* env = base::android::AttachCurrentThread();
+    cronet::Java_CronetUrlRequest_onResponseStarted(env,
+                                                    owner_.obj(),
+                                                    http_status_code);
+  }
+
+  void OnBytesRead(unsigned char* bytes_buffer,
+                   int bytes_read) override {
+    JNIEnv* env = base::android::AttachCurrentThread();
+    base::android::ScopedJavaLocalRef<jobject> java_buffer(
+        env, env->NewDirectByteBuffer(bytes_buffer, bytes_read));
+    cronet::Java_CronetUrlRequest_onDataReceived(
+        env, owner_.obj(), java_buffer.obj());
+  }
+
+  void OnRequestFinished() override {
+    JNIEnv* env = base::android::AttachCurrentThread();
+    cronet::Java_CronetUrlRequest_onComplete(env, owner_.obj());
+  }
+
+  void OnError(int net_error) override {
+    JNIEnv* env = base::android::AttachCurrentThread();
+    cronet::Java_CronetUrlRequest_onError(
+        env,
+        owner_.obj(),
+        net_error,
+        ConvertUTF8ToJavaString(env, net::ErrorToString(net_error)).Release());
+  }
+
+ private:
+  ~JniCronetURLRequestAdapterDelegate() override {
+  }
+
+  // Java object that owns the CronetURLRequestContextAdapter, which owns this
+  // delegate.
+  base::android::ScopedJavaGlobalRef<jobject> owner_;

[mmenke, 2014/11/06 16:08:26]

include base/android/scoped_java_ref.h

[mef, 2014/11/06 22:51:45]

Done, it was already there.

+
+  DISALLOW_COPY_AND_ASSIGN(JniCronetURLRequestAdapterDelegate);
+};
+
+}  // namespace
+
+// Explicitly register static JNI functions.
+bool CronetUrlRequestRegisterJni(JNIEnv* jenv) {
+  return RegisterNativesImpl(jenv);
+}
+
+static jlong CreateRequestAdapter(JNIEnv* jenv,
+                                  jobject jurl_request,
+                                  jlong jurl_request_context_adapter,
+                                  jstring jurl_string,
+                                  jint jpriority) {
+  CronetURLRequestContextAdapter* context_adapter =
+      reinterpret_cast<CronetURLRequestContextAdapter*>(
+          jurl_request_context_adapter);
+  DCHECK(context_adapter);
+
+  GURL url(base::android::ConvertJavaStringToUTF8(jenv, jurl_string));
+
+  VLOG(1) << "New chromium network request_adapter: "
+          << url.possibly_invalid_spec();
+
+  scoped_ptr<CronetURLRequestAdapter::CronetURLRequestAdapterDelegate> delegate(
+      new JniCronetURLRequestAdapterDelegate(jenv, jurl_request));
+
+  CronetURLRequestAdapter* adapter = new CronetURLRequestAdapter(
+      context_adapter,
+      delegate.Pass(),
+      url,
+      static_cast<net::RequestPriority>(jpriority));
+
+  return reinterpret_cast<jlong>(adapter);
+}
+
+static jboolean AddHeader(JNIEnv* jenv,
+                          jobject jurl_request,
+                          jlong jurl_request_adapter,
+                          jstring jname,
+                          jstring jvalue) {
+  DCHECK(jurl_request_adapter);
+  CronetURLRequestAdapter* request_adapter =
+      reinterpret_cast<CronetURLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(!request_adapter->IsOnNetworkThread());
+  std::string name(base::android::ConvertJavaStringToUTF8(jenv, jname));
+  std::string value(base::android::ConvertJavaStringToUTF8(jenv, jvalue));
+  if (!net::HttpUtil::IsValidHeaderName(name) ||
+      !net::HttpUtil::IsValidHeaderValue(value)) {
+    return JNI_FALSE;
+  }
+  request_adapter->AddRequestHeader(name, value);
+  return JNI_TRUE;
+}
+
+static jboolean SetHttpMethod(JNIEnv* jenv,
+                              jobject jurl_request,
+                              jlong jurl_request_adapter,
+                              jstring jmethod) {
+  DCHECK(jurl_request_adapter);
+  CronetURLRequestAdapter* request_adapter =
+      reinterpret_cast<CronetURLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(!request_adapter->IsOnNetworkThread());
+  std::string method(base::android::ConvertJavaStringToUTF8(jenv, jmethod));
+  // Http method is a token, just as header name.
+  if (!net::HttpUtil::IsValidHeaderName(method))
+    return JNI_FALSE;
+  request_adapter->set_method(method);
+  return JNI_TRUE;
+}
+
+static void Start(JNIEnv* jenv,
+                  jobject jurl_request,
+                  jlong jurl_request_adapter) {
+  DCHECK(jurl_request_adapter);
+  CronetURLRequestAdapter* request_adapter =
+      reinterpret_cast<CronetURLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(!request_adapter->IsOnNetworkThread());
+  request_adapter->Start();

[mmenke, 2014/11/06 16:08:26]

optional:  Think it would be cleaner to post tasks here, instead of calling into
the adapter?

Advantages:
* Adapter would live completely on the network thread after construction, with
the exception of grabbing its NetworkThread field, and this file becomes solely
responsible for thread safety, as opposed to splitting the responsibility for
that between the two classes.
* We'd get rid of all those methods that just post a task to run another method.

[mef, 2014/11/06 17:07:46]

Sounds reasonable, I'll try that.

[mef, 2014/11/06 22:51:45]

Done.

+}
+
+static void DestroyRequestAdapter(JNIEnv* jenv,
+                                  jobject jurl_request,
+                                  jlong jurl_request_adapter) {
+  DCHECK(jurl_request_adapter);
+  CronetURLRequestAdapter* request_adapter =
+      reinterpret_cast<CronetURLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(!request_adapter->IsOnNetworkThread());
+  request_adapter->Destroy();
+}
+
+static void ReceiveData(JNIEnv* jenv,
+                        jobject jcaller,
+                        jlong jurl_request_adapter) {
+  DCHECK(jurl_request_adapter);
+  CronetURLRequestAdapter* request_adapter =
+      reinterpret_cast<CronetURLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(!request_adapter->IsOnNetworkThread());
+  request_adapter->ReadData();
+}
+
+static void FollowDeferredRedirect(JNIEnv* jenv,
+                                   jobject jcaller,
+                                   jlong jurl_request_adapter) {
+  DCHECK(jurl_request_adapter);
+  CronetURLRequestAdapter* request_adapter =
+      reinterpret_cast<CronetURLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(!request_adapter->IsOnNetworkThread());
+  request_adapter->FollowDeferredRedirect();
+}
+
+static void PopulateResponseHeaders(JNIEnv* jenv,
+                                    jobject jurl_request,
+                                    jlong jurl_request_adapter,
+                                    jobject jheaders_map) {
+  DCHECK(jurl_request_adapter);
+  CronetURLRequestAdapter* request_adapter =
+      reinterpret_cast<CronetURLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(request_adapter->IsOnNetworkThread());
+
+  net::HttpResponseHeaders* headers = request_adapter->GetResponseHeaders();
+  if (headers == nullptr)
+    return;
+
+  void* iter = nullptr;
+  std::string header_name;
+  std::string header_value;
+  while (headers->EnumerateHeaderLines(&iter, &header_name, &header_value)) {
+    ScopedJavaLocalRef<jstring> name =
+        ConvertUTF8ToJavaString(jenv, header_name);
+    ScopedJavaLocalRef<jstring> value =
+        ConvertUTF8ToJavaString(jenv, header_value);
+    Java_CronetUrlRequest_onAppendResponseHeader(
+        jenv, jurl_request, jheaders_map, name.Release(), value.Release());

[mmenke, 2014/11/06 16:08:26]

I think this is a leak:  When we call into Java, I don't think it takes
ownership of the reference we pass into it, does it?  See, for example,
JniCronetURLRequestAdapterDelegate::OnBytesRead above.

On the other hand, when we return a value to Java in these JNI methods, it
*does* take ownership.

[mef, 2014/11/06 22:51:45]

Done.

+  }
+
+  // Some implementations (notably HttpURLConnection) include a mapping for the
+  // null key; in HTTP's case, this maps to the HTTP status line.
+  ScopedJavaLocalRef<jstring> status_line =
+      ConvertUTF8ToJavaString(jenv, headers->GetStatusLine());
+  Java_CronetUrlRequest_onAppendResponseHeader(
+      jenv, jurl_request, jheaders_map, nullptr, status_line.Release());

[mmenke, 2014/11/06 16:08:26]

Again, Release() -> obj()...  Actually, I think we should just remove this code.
 If we want to mimic HttpURLConnection, should add this to the wrapper - as-is,
seems like a bit of a hack, which I don't think we want to support for the async
API.

[mef, 2014/11/06 22:51:45]

Done.

+}
+
+static jstring GetNegotiatedProtocol(JNIEnv* jenv,
+                                     jobject jurl_request,
+                                     jlong jurl_request_adapter) {
+  DCHECK(jurl_request_adapter);
+  CronetURLRequestAdapter* request_adapter =
+      reinterpret_cast<CronetURLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(request_adapter->IsOnNetworkThread());
+  std::string negotiated_protocol = request_adapter->GetNegotiatedProtocol();

[mmenke, 2014/11/06 17:31:28]

This is an unnecessary string copy.  Can just inline it in the next line, or
make it a const std::string&.

[mef, 2014/11/06 22:51:45]

Done.

+  return ConvertUTF8ToJavaString(jenv, negotiated_protocol).Release();
+}
+
+static jboolean GetWasCached(JNIEnv* jenv,
+                             jobject jurl_request,
+                             jlong jurl_request_adapter) {
+  DCHECK(jurl_request_adapter);
+  CronetURLRequestAdapter* request_adapter =
+      reinterpret_cast<CronetURLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(request_adapter->IsOnNetworkThread());
+  return request_adapter->GetWasCached() ? JNI_TRUE : JNI_FALSE;
+}
+
+static jlong GetTotalReceivedBytes(JNIEnv* jenv,
+                                   jobject jurl_request,
+                                   jlong jurl_request_adapter) {
+  DCHECK(jurl_request_adapter);
+  CronetURLRequestAdapter* request_adapter =
+      reinterpret_cast<CronetURLRequestAdapter*>(jurl_request_adapter);
+  DCHECK(request_adapter->IsOnNetworkThread());
+  return request_adapter->GetTotalReceivedBytes();
+}
+
+}  // namespace cronet