Fix a pair of Cronet upload bugs and add tests.

Fix a pair of Cronet upload bugs and add tests.

When it retried uploads using Channels, it would restart the upload with
data from the middle of the file, since Channels can't be rewound.  It
now just fails those requests instead.

Also, it was impossible to set the method for uploads - the default
value (POST) would overwrite any method set by the embedder.

Adds a new set of upload tests using the embedded test server, as
uploads were basically untested before.  Chunked uploads are still not
being tested, as the test server doesn't support them yet.

BUG=424712
Committed: https://crrev.com/96a960536bdd4d6c6ceec62498a577f1c367ab73
Cr-Commit-Position: refs/heads/master@{#300316}

[mmenke, 2014-10-17 20:17:02]

Patchset #3 (id:40001) has been deleted

[mmenke, 2014-10-17 20:21:26]

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

[mef, 2014-10-17 20:47:27]

sweet!

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:351:
if (mMethod != null) {
good catch!

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java
(right):

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:55:
CronetTestActivity activity = launchCronetTestApp();
Does it make sense to combine boilerplate to launch app and add interceptors
into helper method?
Or maybe that should go into launchCronetTestApp()?

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:103:
TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
nit: 80 chars, although I've heard a suggestion to adjust column width for Java
in Rietveld.

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:180:
assertEquals(0, listener.mHttpStatusCode);
can we get error code here?

[xunjieli, 2014-10-17 20:56:45]

nifty testing infra! :)

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java
(right):

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:22:
* Example test that just starts the cronet sample.
nit: stale doc.

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:32:
return activity.mChromiumRequestFactory.createRequest(
I removed |mChromiumRequestFactory|. You can use |mRequestFactory| instead.

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
File components/cronet/android/test/upload_test_server.cc (right):

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/test/upload_test_server.cc:28:
net::test_server::EmbeddedTestServer* g_test_server = NULL;
nit: nullptr?

https://codereview.chromium.org/640593004/diff/80001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/640593004/diff/80001/net/base/net_error_list....
net/base/net_error_list.h:104: NET_ERROR(UPLOAD_STREAM_REWIND_NOT_SUPPORTED,
-25)
Is it possible to test that we get this error code?

[mmenke, 2014-10-17 21:27:15]

Thanks for the feedback, both of you!

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:351:
if (mMethod != null) {
On 2014/10/17 20:47:27, mef wrote:
> good catch!

Only caught it when I was writing tests for it.  They mysteriously failing.  :)

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java
(right):

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:22:
* Example test that just starts the cronet sample.
On 2014/10/17 20:56:45, xunjieli wrote:
> nit: stale doc.

Done.

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:32:
return activity.mChromiumRequestFactory.createRequest(
On 2014/10/17 20:56:45, xunjieli wrote:
> I removed |mChromiumRequestFactory|. You can use |mRequestFactory| instead.

Done.

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:55:
CronetTestActivity activity = launchCronetTestApp();
On 2014/10/17 20:47:27, mef wrote:
> Does it make sense to combine boilerplate to launch app and add interceptors
> into helper method?
> Or maybe that should go into launchCronetTestApp()?

Reluctant to add this to launchCronetTestApp, since the test server isn't used
by other tests.  Open to doing that if we continue to use it with the new API,
and use the server more broadly.

I've gone ahead and created a separate method.  Also made activity a class
variable.

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:103:
TestHttpUrlRequestListener listener = new TestHttpUrlRequestListener();
On 2014/10/17 20:47:27, mef wrote:
> nit: 80 chars, although I've heard a suggestion to adjust column width for
Java
> in Rietveld.

Done.

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:180:
assertEquals(0, listener.mHttpStatusCode);
On 2014/10/17 20:47:27, mef wrote:
> can we get error code here?

I don't think we expose the error code, currently.  Or am I missing something?

Should definitely add a way to do that to the new API, but don't think it's
worth adding to the old one, if we don't already have one.

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
File components/cronet/android/test/upload_test_server.cc (right):

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/test/upload_test_server.cc:28:
net::test_server::EmbeddedTestServer* g_test_server = NULL;
On 2014/10/17 20:56:45, xunjieli wrote:
> nit: nullptr?

Done.

https://codereview.chromium.org/640593004/diff/80001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/640593004/diff/80001/net/base/net_error_list....
net/base/net_error_list.h:104: NET_ERROR(UPLOAD_STREAM_REWIND_NOT_SUPPORTED,
-25)
On 2014/10/17 20:56:45, xunjieli wrote:
> Is it possible to test that we get this error code?

I don't believe we expose it with the current API - we should with the new one,
but I don't think it's worth adding much more to the old one.

[mef, 2014-10-17 22:01:34]

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java
(right):

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:180:
assertEquals(0, listener.mHttpStatusCode);
On 2014/10/17 21:27:15, mmenke wrote:
> On 2014/10/17 20:47:27, mef wrote:
> > can we get error code here?
> 
> I don't think we expose the error code, currently.  Or am I missing something?
> 
> Should definitely add a way to do that to the new API, but don't think it's
> worth adding to the old one, if we don't already have one.

In old API there is a method GetErrorCode, which unfortunately does some fuzzy
mapping.
There is also a method GetErrorString which generates a string, which includes
error code, so that may be a decent check.

In new API I've added net error code to UrlRequestException that gets passed to
UrlRequestListener.

[xunjieli, 2014-10-20 13:57:48]

https://codereview.chromium.org/640593004/diff/100001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java
(right):

https://codereview.chromium.org/640593004/diff/100001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:33:
private void init() throws Exception {
nit: override setUp(). An example is
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...
.

[mmenke, 2014-10-20 17:19:38]

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java
(right):

https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:180:
assertEquals(0, listener.mHttpStatusCode);
On 2014/10/17 22:01:34, mef wrote:
> On 2014/10/17 21:27:15, mmenke wrote:
> > On 2014/10/17 20:47:27, mef wrote:
> > > can we get error code here?
> > 
> > I don't think we expose the error code, currently.  Or am I missing
something?
> > 
> > Should definitely add a way to do that to the new API, but don't think it's
> > worth adding to the old one, if we don't already have one.
> 
> In old API there is a method GetErrorCode, which unfortunately does some fuzzy
> mapping.
> There is also a method GetErrorString which generates a string, which includes
> error code, so that may be a decent check.
> 
> In new API I've added net error code to UrlRequestException that gets passed
to
> UrlRequestListener.

Thanks, done!  The test currently depends on both the string error name and the
raw numeric error code. The new API should make it possible for tests to only
depend on one of them - doesn't seem worth updating the old API to allow that,
though.

https://codereview.chromium.org/640593004/diff/100001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java
(right):

https://codereview.chromium.org/640593004/diff/100001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:33:
private void init() throws Exception {
On 2014/10/20 13:57:48, xunjieli wrote:
> nit: override setUp(). An example is
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...
> . 

Much nicer, thanks!

[xunjieli, 2014-10-20 17:23:04]

On 2014/10/20 17:19:38, mmenke wrote:
>
https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
> File
>
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java
> (right):
> 
>
https://codereview.chromium.org/640593004/diff/60001/components/cronet/androi...
>
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:180:
> assertEquals(0, listener.mHttpStatusCode);
> On 2014/10/17 22:01:34, mef wrote:
> > On 2014/10/17 21:27:15, mmenke wrote:
> > > On 2014/10/17 20:47:27, mef wrote:
> > > > can we get error code here?
> > > 
> > > I don't think we expose the error code, currently.  Or am I missing
> something?
> > > 
> > > Should definitely add a way to do that to the new API, but don't think
it's
> > > worth adding to the old one, if we don't already have one.
> > 
> > In old API there is a method GetErrorCode, which unfortunately does some
fuzzy
> > mapping.
> > There is also a method GetErrorString which generates a string, which
includes
> > error code, so that may be a decent check.
> > 
> > In new API I've added net error code to UrlRequestException that gets passed
> to
> > UrlRequestListener.
> 
> Thanks, done!  The test currently depends on both the string error name and
the
> raw numeric error code. The new API should make it possible for tests to only
> depend on one of them - doesn't seem worth updating the old API to allow that,
> though.
> 
>
https://codereview.chromium.org/640593004/diff/100001/components/cronet/andro...
> File
>
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java
> (right):
> 
>
https://codereview.chromium.org/640593004/diff/100001/components/cronet/andro...
>
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:33:
> private void init() throws Exception {
> On 2014/10/20 13:57:48, xunjieli wrote:
> > nit: override setUp(). An example is
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...
> > . 
> 
> Much nicer, thanks!

LGTM!

[mef, 2014-10-20 17:54:00]

lgtm

[mmenke, 2014-10-20 18:01:19]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-10-20 18:02:01]

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

[commit-bot: I haz the power, 2014-10-20 19:38:35]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2014-10-20 19:39:15]

Patchset 7 (id:??) landed as
https://crrev.com/96a960536bdd4d6c6ceec62498a577f1c367ab73
Cr-Commit-Position: refs/heads/master@{#300316}