[Cronet] Allow UrlRequest bypass cache

[Cronet] Allow UrlRequest bypass cache

This CL is to correctly support disabling/enabling per-request caching in
HttpURLConnection.

BUG=398997
Committed: https://crrev.com/da2c8c2b832d138f470432329a812e8b37a46371
Cr-Commit-Position: refs/heads/master@{#315327}

[xunjieli, 2015-01-20 19:17:53]

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

[xunjieli, 2015-01-20 19:17:53]

Misha, could you take a look at this CL? I will send it to Matt after you've
reviewed, since he has too many reviews on his plate right now.

[xunjieli, 2015-01-21 20:11:43]

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

[xunjieli, 2015-01-21 20:11:44]

Thanks Matt for reviewing my other two CLs!! I think I will need some time to
fix those two. Sending this one to your way now. Low priority, just in case you
get some time in the meanwhile.

[mmenke, 2015-01-21 20:21:24]

On 2015/01/21 20:11:44, xunjieli wrote:
> Thanks Matt for reviewing my other two CLs!! I think I will need some time to
> fix those two. Sending this one to your way now. Low priority, just in case
you
> get some time in the meanwhile.

I'll plan to get to this tomorrow.  I'm finally pretty much caught up on my
reviews, so feel free to ping me aggressively if I don't get back to you
relatively promptly now, regardless of priority (And thanks for your patience -
much appreciated).

[mmenke, 2015-01-22 16:21:20]

https://codereview.chromium.org/860893002/diff/1/components/cronet/android/cr...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/860893002/diff/1/components/cronet/android/cr...
components/cronet/android/cronet_url_request_adapter.cc:71: }
nit:  Don't use brace if the body and condition of an if each take up only one
line.

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

https://codereview.chromium.org/860893002/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:74:
if (!getUseCaches()) {
Can you add a test for this, ideally one that compares us with the platform
implementation.

https://codereview.chromium.org/860893002/diff/1/components/cronet/android/te...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/860893002/diff/1/components/cronet/android/te...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:370:
checkRequestCaching(url, true);
Suggest doing the equivalent of checkRequestCaching(url, false, true), and
making sure we get an error.

[mmenke, 2015-01-22 21:18:39]

https://codereview.chromium.org/860893002/diff/1/components/cronet/android/te...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/860893002/diff/1/components/cronet/android/te...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:363:
String url = UploadTestServer.getFileURL("/cacheable.txt");
Should have a "checkRequestCaching(url, false, true /** bypass cache */);" here,
to make sure it doesn't write to the cache, in addition to not reading from it.

[xunjieli, 2015-01-26 21:36:09]

Thanks for the reviews! PTAL.

https://codereview.chromium.org/860893002/diff/1/components/cronet/android/cr...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/860893002/diff/1/components/cronet/android/cr...
components/cronet/android/cronet_url_request_adapter.cc:71: }
On 2015/01/22 16:21:20, mmenke wrote:
> nit:  Don't use brace if the body and condition of an if each take up only one
> line.

Done.

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

https://codereview.chromium.org/860893002/diff/1/components/cronet/android/ja...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:74:
if (!getUseCaches()) {
On 2015/01/22 16:21:20, mmenke wrote:
> Can you add a test for this, ideally one that compares us with the platform
> implementation.

Partially done. It looks like the default implementation does not cache the
result. See my test in CronetHttpURLConnectTest for more details.

https://codereview.chromium.org/860893002/diff/1/components/cronet/android/te...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/860893002/diff/1/components/cronet/android/te...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:363:
String url = UploadTestServer.getFileURL("/cacheable.txt");
On 2015/01/22 21:18:39, mmenke wrote:
> Should have a "checkRequestCaching(url, false, true /** bypass cache */);"
here,
> to make sure it doesn't write to the cache, in addition to not reading from
it.

Done.

https://codereview.chromium.org/860893002/diff/1/components/cronet/android/te...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:370:
checkRequestCaching(url, true);
On 2015/01/22 16:21:20, mmenke wrote:
> Suggest doing the equivalent of checkRequestCaching(url, false, true), and
> making sure we get an error.

Done.

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java
(right):

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:127:
// Starts the server again to prepare for comparison.
Had to change this inaccurate comment. If remove start server (line128), the
test crashes when we try to connect to the server in the next half of the test
(using Cronet's implementation). It doesn't crash in teardown().

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:621:
checkRequestCaching(url, true /** useCaches */, true /** success */);
Strangely, the default implementation fails to return a cached response. If the
server is shut down, the request just fails with a connection refused error. So
I annotated this test and the next with "@OnlyRunCronetHttpURLConnection".

[mef, 2015-01-30 16:06:09]

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
components/cronet/android/cronet_url_request_adapter.cc:68: int load_flags =
context_->default_load_flags();
Would it make sense to make |load_flags_| a member variable, which is
initialized by context_->default_load_flags() in constructor?

BypassCache() would set net::LOAD_DISABLE_CACHE.

Start() would use load_flags_ instead of default_load_flags().

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
File components/cronet/android/cronet_url_request_adapter.h (right):

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
components/cronet/android/cronet_url_request_adapter.h:146: bool bypass_cache_;
I'd call it 'initial_bypass_cache_' to match others.

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:97: public
void bypassCache();
I'm a bit hesitant about calling it 'bypassCache' as it doesn't directly map
neither in useCaches() from java.net nor into LOAD_DISABLE_CACHE from net/.

[xunjieli, 2015-01-30 22:39:46]

Thanks for the review! PTAL

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
components/cronet/android/cronet_url_request_adapter.cc:68: int load_flags =
context_->default_load_flags();
On 2015/01/30 16:06:08, mef wrote:
> Would it make sense to make |load_flags_| a member variable, which is
> initialized by context_->default_load_flags() in constructor?
> 
> BypassCache() would set net::LOAD_DISABLE_CACHE.
> 
> Start() would use load_flags_ instead of default_load_flags().
> 

Done.

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
File components/cronet/android/cronet_url_request_adapter.h (right):

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
components/cronet/android/cronet_url_request_adapter.h:146: bool bypass_cache_;
On 2015/01/30 16:06:08, mef wrote:
> I'd call it 'initial_bypass_cache_' to match others.

Removed bypass_cache_ and added load_flags_.

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:97: public
void bypassCache();
On 2015/01/30 16:06:08, mef wrote:
> I'm a bit hesitant about calling it 'bypassCache' as it doesn't directly map
> neither in useCaches() from http://java.net nor into LOAD_DISABLE_CACHE from
net/.

Not sure what naming scheme will best suit in this case. What about use
"setUseCachesFalse()" on the java side, and use "DisableCaches()" on the c++
side? WDYT? Any suggestions?

[mmenke, 2015-02-02 20:35:52]

LGTM

https://codereview.chromium.org/860893002/diff/40001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java
(right):

https://codereview.chromium.org/860893002/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:615:
@OnlyRunCronetHttpURLConnection
Why can't we run these with the default implementation?

https://codereview.chromium.org/860893002/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:642:
private void checkRequestCaching(String requestUrl, boolean useCaches,
Suggest putting this above the tests that use it, like in the other file.

[xunjieli, 2015-02-02 20:48:20]

https://codereview.chromium.org/860893002/diff/40001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java
(right):

https://codereview.chromium.org/860893002/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:615:
@OnlyRunCronetHttpURLConnection
On 2015/02/02 20:35:52, mmenke wrote:
> Why can't we run these with the default implementation?

Commented in a previous patch. 

Strangely, the default implementation fails to return a cached response (line
621). If the
server is shut down, the request just fails with a connection refused error. So
I annotated this test and the next with "@OnlyRunCronetHttpURLConnection".

https://codereview.chromium.org/860893002/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:642:
private void checkRequestCaching(String requestUrl, boolean useCaches,
On 2015/02/02 20:35:52, mmenke wrote:
> Suggest putting this above the tests that use it, like in the other file.

will do.

[mmenke, 2015-02-02 21:03:40]

https://codereview.chromium.org/860893002/diff/40001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java
(right):

https://codereview.chromium.org/860893002/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:615:
@OnlyRunCronetHttpURLConnection
On 2015/02/02 20:48:20, xunjieli wrote:
> On 2015/02/02 20:35:52, mmenke wrote:
> > Why can't we run these with the default implementation?
> 
> Commented in a previous patch. 
> 
> Strangely, the default implementation fails to return a cached response (line
> 621). If the
> server is shut down, the request just fails with a connection refused error.
So
> I annotated this test and the next with "@OnlyRunCronetHttpURLConnection".

Sorry, should have checked for your earlier comment.  Should probably mention
that in a comment before both these tests.

[xunjieli, 2015-02-02 21:05:08]

https://codereview.chromium.org/860893002/diff/40001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java
(right):

https://codereview.chromium.org/860893002/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:615:
@OnlyRunCronetHttpURLConnection
On 2015/02/02 21:03:40, mmenke wrote:
> On 2015/02/02 20:48:20, xunjieli wrote:
> > On 2015/02/02 20:35:52, mmenke wrote:
> > > Why can't we run these with the default implementation?
> > 
> > Commented in a previous patch. 
> > 
> > Strangely, the default implementation fails to return a cached response
(line
> > 621). If the
> > server is shut down, the request just fails with a connection refused error.
> So
> > I annotated this test and the next with "@OnlyRunCronetHttpURLConnection".
> 
> Sorry, should have checked for your earlier comment.  Should probably mention
> that in a comment before both these tests.

Good idea! will do. Sorry abt that.

[mef, 2015-02-04 20:39:42]

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:97: public
void bypassCache();
On 2015/01/30 22:39:46, xunjieli wrote:
> On 2015/01/30 16:06:08, mef wrote:
> > I'm a bit hesitant about calling it 'bypassCache' as it doesn't directly map
> > neither in useCaches() from http://java.net nor into LOAD_DISABLE_CACHE from
> net/.
> 
> Not sure what naming scheme will best suit in this case. What about use
> "setUseCachesFalse()" on the java side, and use "DisableCaches()" on the c++
> side? WDYT? Any suggestions? 

"setUseCachesFalse" seems a bit wordy. Maybe "disableCache" on both sides? 
Matt, do you have any suggestions as native speaker?

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

https://codereview.chromium.org/860893002/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:322:
mBypassCache = true;
why can't we just call nativeBypassCache here?

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

https://codereview.chromium.org/860893002/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:649:
checkRequestCaching(url, true /** useCaches */, true /** success */);
I find the use of comments interesting, is it recommended by coding guide?

Would it make sense to define custom enums like { useCaches, dontUseCaches } and
{ expectSuccess, expectError }?

[mmenke, 2015-02-04 20:54:33]

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/UrlRequest.java
(right):

https://codereview.chromium.org/860893002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/UrlRequest.java:97: public
void bypassCache();
On 2015/02/04 20:39:42, mef wrote:
> On 2015/01/30 22:39:46, xunjieli wrote:
> > On 2015/01/30 16:06:08, mef wrote:
> > > I'm a bit hesitant about calling it 'bypassCache' as it doesn't directly
map
> > > neither in useCaches() from http://java.net nor into LOAD_DISABLE_CACHE
from
> > net/.
> > 
> > Not sure what naming scheme will best suit in this case. What about use
> > "setUseCachesFalse()" on the java side, and use "DisableCaches()" on the c++
> > side? WDYT? Any suggestions? 
> 
> "setUseCachesFalse" seems a bit wordy. Maybe "disableCache" on both sides? 
> Matt, do you have any suggestions as native speaker?

I'm fine with disableCache.  Other options I don't like quite as much: 
skipCache, [set]noCache.

[xunjieli, 2015-02-04 21:27:05]

SGTM. I changed the name to disableCache. PTAL.

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

https://codereview.chromium.org/860893002/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java:322:
mBypassCache = true;
On 2015/02/04 20:39:42, mef wrote:
> why can't we just call nativeBypassCache here?

Cronet's UrlRequestAdapter is only created in start(), so we can't call
nativeBypassCache() here.

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

https://codereview.chromium.org/860893002/diff/60001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:649:
checkRequestCaching(url, true /** useCaches */, true /** success */);
On 2015/02/04 20:39:42, mef wrote:
> I find the use of comments interesting, is it recommended by coding guide?
> 
> Would it make sense to define custom enums like { useCaches, dontUseCaches }
and
> { expectSuccess, expectError }?

Done.

[mef, 2015-02-06 16:55:03]

lgtm

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

https://codereview.chromium.org/860893002/diff/100001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:366:
checkRequestCaching(url, false, true /** bypass cache */);
nit: bypass -> disable

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

https://codereview.chromium.org/860893002/diff/100001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:720:
private static enum CacheSetting { USE_CACHE, DONT_USE_CACHE};
nit: should there be a space before closing } for symmetry?

[xunjieli, 2015-02-06 21:01:47]

New patchsets have been uploaded after l-g-t-m from mef@chromium.org

[xunjieli, 2015-02-06 21:24:51]

Patchset #7 (id:120001) has been deleted

[xunjieli, 2015-02-09 16:38:58]

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

https://codereview.chromium.org/860893002/diff/100001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:366:
checkRequestCaching(url, false, true /** bypass cache */);
On 2015/02/06 16:55:03, mef wrote:
> nit: bypass -> disable

Done.

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

https://codereview.chromium.org/860893002/diff/100001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:720:
private static enum CacheSetting { USE_CACHE, DONT_USE_CACHE};
On 2015/02/06 16:55:03, mef wrote:
> nit: should there be a space before closing } for symmetry? 

Done.

[xunjieli, 2015-02-09 16:39:05]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2015-02-09 16:40:18]

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

[commit-bot: I haz the power, 2015-02-09 16:43:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-09 16:43:42]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[xunjieli, 2015-02-09 16:51:47]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2015-02-09 16:52:34]

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

[commit-bot: I haz the power, 2015-02-09 17:16:13]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-02-09 17:16:31]

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