Fix Cronet compilation and test errors.

Fix Cronet compilation and test errors.

- Use UrlRequestContext.CreateRequest.
- Expect GIT revision in HttpUrlRequestFactoryTest.
- Prevent getAllHeaders from being removed by proguard.

BUG=390267
Committed: https://crrev.com/ac2715c66e674556caeb648a81877dcf42bced6a
Cr-Commit-Position: refs/heads/master@{#291912}

[mef, 2014-08-25 19:28:17]

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

[xunjieli, 2014-08-25 19:33:34]

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java
(right):

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java:17:
* Tests for {@link ChunkedWritableByteChannel}
Can you fix the comment too?

[mmenke, 2014-08-25 19:46:44]

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/ja...
File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:400:
@SuppressWarnings("unused")
Should this be using the called by native thing?

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java
(right):

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:26:
class TestHttpUrlRequestListener implements HttpUrlRequestListener {
While this is much better than before, and certainly worth doing, does this
belong with the other changes?  Did something break here, too?

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java
(right):

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java:27:
assertTrue("Factory should be Chromium/n.n.n.n/r",
This line isn't quite correct

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/ur...
File components/cronet/android/url_request_adapter.cc (right):

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/ur...
components/cronet/android/url_request_adapter.cc:283: url_request_.reset();
Amusingly, I made this exact change in my CL to fix context destruction.

[mef, 2014-08-25 20:26:18]

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/ja...
File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:400:
@SuppressWarnings("unused")
On 2014/08/25 19:46:44, mmenke wrote:
> Should this be using the called by native thing?
Well, it isn't really called by native.

Per discussion with clm@ (correct me if I'm wrong) my understanding of the
problem is:
1. If getAllHeaders is not unused, then proguard removes it.
2. That makes nativeGetAllHeaders also unused, so proguard also removes it.
3. When libcronet.so is loaded it tries to register implementation of
nativeGetAllHeaders, and that fails (CHECKS) as there is no java-side method
declaration (as it is removed in step 2).

I suspect that similar issue may happen if app doesn't have calls to getHeader()
either.

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java
(right):

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:26:
class TestHttpUrlRequestListener implements HttpUrlRequestListener {
On 2014/08/25 19:46:44, mmenke wrote:
> While this is much better than before, and certainly worth doing, does this
> belong with the other changes?  Did something break here, too?
Not really. Should I split them out?

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java
(right):

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java:17:
* Tests for {@link ChunkedWritableByteChannel}
On 2014/08/25 19:33:34, xunjieli1 wrote:
> Can you fix the comment too?

Done.

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/HttpUrlRequestFactoryTest.java:27:
assertTrue("Factory should be Chromium/n.n.n.n/r",
On 2014/08/25 19:46:44, mmenke wrote:
> This line isn't quite correct

Acknowledged. Will fix.

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/ur...
File components/cronet/android/url_request_adapter.cc (right):

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/ur...
components/cronet/android/url_request_adapter.cc:283: url_request_.reset();
On 2014/08/25 19:46:44, mmenke wrote:
> Amusingly, I made this exact change in my CL to fix context destruction.
Excellent!

[xunjieli, 2014-08-25 20:32:48]

https://codereview.chromium.org/500193002/diff/20001/components/cronet/androi...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java
(right):

https://codereview.chromium.org/500193002/diff/20001/components/cronet/androi...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:99:
CronetSampleActivity activity = launchCronetSampleWithUrl(URL);
The java doc on launchCronetSampleWithUrl and
launchCronetSampleWithUrlAndCommandLineArgs is not accurate ("The URL can be
null, in which case will default to CronetSampleActivity.DEFAULT_SHELL_URL",
which won't happen from what I can see). Do you want to fix the java doc in this
CL? or I can write a patch later when I work on the tests.

[mmenke, 2014-08-25 20:36:43]

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java
(right):

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:26:
class TestHttpUrlRequestListener implements HttpUrlRequestListener {
On 2014/08/25 20:26:18, mef wrote:
> On 2014/08/25 19:46:44, mmenke wrote:
> > While this is much better than before, and certainly worth doing, does this
> > belong with the other changes?  Did something break here, too?
> Not really. Should I split them out?

Please do, thanks!

[mef, 2014-08-25 22:12:02]

Patchset #3 (id:40001) has been deleted

[mef, 2014-08-25 22:12:42]

Thanks, PTAL.

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/ja...
File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:400:
@SuppressWarnings("unused")
On 2014/08/25 20:26:18, mef wrote:
> On 2014/08/25 19:46:44, mmenke wrote:
> > Should this be using the called by native thing?
> Well, it isn't really called by native.
> 
> Per discussion with clm@ (correct me if I'm wrong) my understanding of the
> problem is:
> 1. If getAllHeaders is not unused, then proguard removes it.
> 2. That makes nativeGetAllHeaders also unused, so proguard also removes it.
> 3. When libcronet.so is loaded it tries to register implementation of
> nativeGetAllHeaders, and that fails (CHECKS) as there is no java-side method
> declaration (as it is removed in step 2).
> 
> I suspect that similar issue may happen if app doesn't have calls to
getHeader()
> either.
Per discussion with clm@ this is not a fix, but just a warning suppression.
Real fix is in proguard.cfg (keep class with its native methods).

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java
(right):

https://codereview.chromium.org/500193002/diff/1/components/cronet/android/sa...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:26:
class TestHttpUrlRequestListener implements HttpUrlRequestListener {
On 2014/08/25 20:36:43, mmenke wrote:
> On 2014/08/25 20:26:18, mef wrote:
> > On 2014/08/25 19:46:44, mmenke wrote:
> > > While this is much better than before, and certainly worth doing, does
this
> > > belong with the other changes?  Did something break here, too?
> > Not really. Should I split them out?
> 
> Please do, thanks!

Done.

https://codereview.chromium.org/500193002/diff/20001/components/cronet/androi...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java
(right):

https://codereview.chromium.org/500193002/diff/20001/components/cronet/androi...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:99:
CronetSampleActivity activity = launchCronetSampleWithUrl(URL);
On 2014/08/25 20:32:48, xunjieli1 wrote:
> The java doc on launchCronetSampleWithUrl and
> launchCronetSampleWithUrlAndCommandLineArgs is not accurate ("The URL can be
> null, in which case will default to CronetSampleActivity.DEFAULT_SHELL_URL",
> which won't happen from what I can see). Do you want to fix the java doc in
this
> CL? or I can write a patch later when I work on the tests.
SGTM. I'd do it separately, but it would be even better if you could do it. :)

[Charles, 2014-08-25 22:34:15]

lgtm

[xunjieli, 2014-08-26 13:36:29]

LGTM

https://codereview.chromium.org/500193002/diff/20001/components/cronet/androi...
File
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java
(right):

https://codereview.chromium.org/500193002/diff/20001/components/cronet/androi...
components/cronet/android/sample/javatests/src/org/chromium/cronet_sample_apk/CronetSampleUrlTest.java:99:
CronetSampleActivity activity = launchCronetSampleWithUrl(URL);
SGTM. I will put it in my TODOs. Thanks
On 2014/08/25 22:12:42, mef wrote:
> On 2014/08/25 20:32:48, xunjieli1 wrote:
> > The java doc on launchCronetSampleWithUrl and
> > launchCronetSampleWithUrlAndCommandLineArgs is not accurate ("The URL can be
> > null, in which case will default to CronetSampleActivity.DEFAULT_SHELL_URL",
> > which won't happen from what I can see). Do you want to fix the java doc in
> this
> > CL? or I can write a patch later when I work on the tests.
> SGTM. I'd do it separately, but it would be even better if you could do it. :)

[mmenke, 2014-08-26 13:50:19]

LGTM

[mef, 2014-08-26 14:58:22]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2014-08-26 14:58:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mef@chromium.org/500193002/60001

[commit-bot: I haz the power, 2014-08-26 15:51:42]

Committed patchset #3 (60001) as 6e4bf6b8e0fed151a57bdb6a80e30db14d4e3b38

[commit-bot: I haz the power, 2014-09-10 02:43:08]

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