Use "@NativeClassQualifiedName" in CronetUrlRequest.java

This CL uses @NativeClassQualifiedName in
CronetUrlRequest.java to reduce JNI code. After the
refactoring, cronet_url_request.cc is merged with
cronet_url_request_adapter.cc, since the former mostly
contains JNI code that can be generated by
@NativeClassQualifiedName.
This CL also inlines CronetUrlRequestAdapterDelegate, since
we have only one file now.

BUG=458276
Committed: https://crrev.com/a8177757e64b400d45f7bc7227242e8813ab53fb
Cr-Commit-Position: refs/heads/master@{#323976}

[xunjieli, 2015-04-02 20:25:11]

Patchset #1 (id:1) has been deleted

[xunjieli, 2015-04-02 20:25:16]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2015-04-02 20:25:22]

Patchset #1 (id:40001) has been deleted

[xunjieli, 2015-04-02 20:25:27]

Patchset #1 (id:60001) has been deleted

[xunjieli, 2015-04-02 20:25:33]

Patchset #1 (id:80001) has been deleted

[xunjieli, 2015-04-02 20:25:37]

Patchset #1 (id:100001) has been deleted

[xunjieli, 2015-04-02 20:31:54]

xunjieli@chromium.org changed reviewers:
+ mef@chromium.org, mmenke@chromium.org

[xunjieli, 2015-04-02 20:34:28]

Looks like "@NativeClassQualifiedName" can save us about 200 lines of code. I
think the code is easier to reason about with one file removed. PTAL when you
get time, not high priority.

[mmenke, 2015-04-02 21:07:05]

I'd actually like to see this land sooner rather than later, before I get too
far on working on switching to a pull-based API.

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:7: #include <jni.h>
nit:  Not needed (included in header file)

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:10: #include
"base/android/jni_string.h"
nit:  These two are used in the header

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:14: #include
"base/macros.h"
nit:  in header

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:35: }  // namespace
nit:  Either need static or anonymous namespace, not both.  For just a couple
constants, static may be cleaner.

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:75: owner_.Reset(env,
jurl_request);
DCHECK(!context_->IsOnNetworkThread());?

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:123: void
CronetURLRequestAdapter::StartOnNetworkThread() {
For this, ReadDataOnNetworkThread, and DestroyOnNetworkThread, definition order
should match declaration order in the header.

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:198:
base::Unretained(url_request_.get())));
While this works, I think it's a little ugly and more regression prone than
posting a task that runs a CronetURLRequestAdapter method that calls
FollowDeferredRedirect on the other thread.

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.h (right):

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.h:44: // to set up a
request must be called exclusively on the network thread.
Update docs.

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.h:83: void
FollowDeferredRedirect(JNIEnv* env, jobject jcaller);
Suggest putting this between Start and ReadData, to match the order they're used
in (And match order in the CC file, of course)

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.h:87: void
DisableCache(JNIEnv* env, jobject jcaller);
This should go up with the other ones that must be called prior to start, on any
thread (And same for the cc file)

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java
(right):

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:695:
private native void nativeDisableCache(long nativePtr);
Order here should match order in the h and cc files.

[mmenke, 2015-04-02 21:10:54]

I agree that this is a really nice cleanup!  Definitely worth doing.

[xunjieli, 2015-04-02 21:50:13]

Thanks for the review! Glad to know that this is worth doing :)

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:7: #include <jni.h>
On 2015/04/02 21:07:05, mmenke wrote:
> nit:  Not needed (included in header file)

Done.

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:10: #include
"base/android/jni_string.h"
On 2015/04/02 21:07:05, mmenke wrote:
> nit:  These two are used in the header

Done.

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:14: #include
"base/macros.h"
On 2015/04/02 21:07:05, mmenke wrote:
> nit:  in header

Done.

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:35: }  // namespace
On 2015/04/02 21:07:05, mmenke wrote:
> nit:  Either need static or anonymous namespace, not both.  For just a couple
> constants, static may be cleaner.

Done.

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:75: owner_.Reset(env,
jurl_request);
On 2015/04/02 21:07:05, mmenke wrote:
> DCHECK(!context_->IsOnNetworkThread());?

Done.

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:123: void
CronetURLRequestAdapter::StartOnNetworkThread() {
On 2015/04/02 21:07:05, mmenke wrote:
> For this, ReadDataOnNetworkThread, and DestroyOnNetworkThread, definition
order
> should match declaration order in the header.

Done.

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:198:
base::Unretained(url_request_.get())));
On 2015/04/02 21:07:05, mmenke wrote:
> While this works, I think it's a little ugly and more regression prone than
> posting a task that runs a CronetURLRequestAdapter method that calls
> FollowDeferredRedirect on the other thread.

Done.

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.h (right):

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.h:44: // to set up a
request must be called exclusively on the network thread.
On 2015/04/02 21:07:05, mmenke wrote:
> Update docs.

Done.

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.h:83: void
FollowDeferredRedirect(JNIEnv* env, jobject jcaller);
On 2015/04/02 21:07:05, mmenke wrote:
> Suggest putting this between Start and ReadData, to match the order they're
used
> in (And match order in the CC file, of course)

Done.

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.h:87: void
DisableCache(JNIEnv* env, jobject jcaller);
On 2015/04/02 21:07:05, mmenke wrote:
> This should go up with the other ones that must be called prior to start, on
any
> thread (And same for the cc file)

Done. Good rule for ordering. Thanks!

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java
(right):

https://codereview.chromium.org/923213002/diff/120001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:695:
private native void nativeDisableCache(long nativePtr);
On 2015/04/02 21:07:05, mmenke wrote:
> Order here should match order in the h and cc files.

Done.

[mmenke, 2015-04-03 18:04:20]

LGTM!

https://codereview.chromium.org/923213002/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.h (right):

https://codereview.chromium.org/923213002/diff/140001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.h:43: // next step like
FollowDeferredRedirect, ReadData or Destroy. Pulic methods can
nit:  Pulic -> Public

[xunjieli, 2015-04-03 18:11:51]

Thanks for the review! will wait for Misha to have a chance to take a look.

https://codereview.chromium.org/923213002/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.h (right):

https://codereview.chromium.org/923213002/diff/140001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.h:43: // next step like
FollowDeferredRedirect, ReadData or Destroy. Pulic methods can
On 2015/04/03 18:04:20, mmenke wrote:
> nit:  Pulic -> Public

Done. Oops. thanks!

[mef, 2015-04-06 20:41:31]

lgtm

https://codereview.chromium.org/923213002/diff/160001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.h (right):

https://codereview.chromium.org/923213002/diff/160001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.h:94:
base::android::ScopedJavaLocalRef<jstring> GetHttpStatusText(
add comment.

https://codereview.chromium.org/923213002/diff/160001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.h:131: // Java object that
owns the CronetURLRequestContextAdapter.
owns this CronetURLRequestAdapter

[xunjieli, 2015-04-06 21:28:21]

Thanks for the review!

https://codereview.chromium.org/923213002/diff/160001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.h (right):

https://codereview.chromium.org/923213002/diff/160001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.h:94:
base::android::ScopedJavaLocalRef<jstring> GetHttpStatusText(
On 2015/04/06 20:41:30, mef wrote:
> add comment.

Done.

https://codereview.chromium.org/923213002/diff/160001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.h:131: // Java object that
owns the CronetURLRequestContextAdapter.
On 2015/04/06 20:41:31, mef wrote:
> owns this CronetURLRequestAdapter

Done.

[xunjieli, 2015-04-06 21:28:31]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2015-04-06 21:28:31]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
mef@chromium.org
Link to the patchset: https://codereview.chromium.org/923213002/#ps180001
(title: "Comments"")

[commit-bot: I haz the power, 2015-04-06 21:28:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/923213002/180001

[commit-bot: I haz the power, 2015-04-06 23:22:18]

Committed patchset #4 (id:180001)

[commit-bot: I haz the power, 2015-04-06 23:23:17]

Patchset 4 (id:??) landed as
https://crrev.com/a8177757e64b400d45f7bc7227242e8813ab53fb
Cr-Commit-Position: refs/heads/master@{#323976}