[Android WebView] Implement WebSettings.{get|set}CacheMode

[Android WebView] Implement WebSettings.{get|set}CacheMode

The mapping between WebSettings cache modes and net::LOAD_... flags is taken
from the current Android implementation.

R=boliu@chromium.org
NOTRY=true

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169082

[boliu, 2012-11-20 20:00:09]

Mostly nits. Only blocking thing is verify this works with as expected with
blockNetworkLoads.

Also seems like we should start consolidating all the jni calls we are making
from the io thread on every request. But definitely not part of this patch.

https://codereview.chromium.org/11419093/diff/1/android_webview/browser/rende...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/11419093/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:92:
request_->set_load_flags(load_flags);
nit: Let's do better than existing code. I think the the 4 cache flags are
mutually exclusive, so can we make sure only one of them is set? Looking at this
code, I think it can work. See SetOnlyAllowLoadFromCache above.

https://codereview.chromium.org/11419093/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:119:
SetOnlyAllowLoadFromCache(request_);
Can you make sure the flags above are interacting correctly with this which also
sets flags?

https://codereview.chromium.org/11419093/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/11419093/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:131: //
Called on the IO thread.
nit: Just write this comment once above the class?

https://codereview.chromium.org/11419093/diff/1/android_webview/java/src/org/...
File
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java
(right):

https://codereview.chromium.org/11419093/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java:19:
// Called on the IO thread.
nit: All the methods are called from IO thread, and is stated above in the class
comments. Can we remove the individual ones on the methods?

[mnaganov (inactive), 2012-11-21 10:37:08]

Thanks for comments!

https://codereview.chromium.org/11419093/diff/1/android_webview/browser/rende...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/11419093/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:92:
request_->set_load_flags(load_flags);
On 2012/11/20 20:00:09, boliu wrote:
> nit: Let's do better than existing code. I think the the 4 cache flags are
> mutually exclusive, so can we make sure only one of them is set? Looking at
this
> code, I think it can work. See SetOnlyAllowLoadFromCache above.

Done. BTW, there was an error in SetOnlyAllowLoadFromCache, as AND-ing flags
effectively produces zero.

https://codereview.chromium.org/11419093/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:119:
SetOnlyAllowLoadFromCache(request_);
On 2012/11/20 20:00:09, boliu wrote:
> Can you make sure the flags above are interacting correctly with this which
also
> sets flags?

Added a test. In fact, setting cache mode doesn't have any effect when network
loads are blocked (I verified with the legacy WebView for sure), so I moved the
new code under the else branch of network loads blocking.

https://codereview.chromium.org/11419093/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/11419093/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:131: //
Called on the IO thread.
On 2012/11/20 20:00:09, boliu wrote:
> nit: Just write this comment once above the class?

Done.

https://codereview.chromium.org/11419093/diff/1/android_webview/java/src/org/...
File
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java
(right):

https://codereview.chromium.org/11419093/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java:19:
// Called on the IO thread.
On 2012/11/20 20:00:09, boliu wrote:
> nit: All the methods are called from IO thread, and is stated above in the
class
> comments. Can we remove the individual ones on the methods?

Done. I rephrased the class comment to avoid excessive mentioning of "IO
thread".

[mkosiba (inactive), 2012-11-21 12:35:15]

lgtm

https://codereview.chromium.org/11419093/diff/6002/android_webview/browser/re...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/11419093/diff/6002/android_webview/browser/re...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:32:
const int all_flags = net::LOAD_BYPASS_CACHE |
nit: s/all_flags/valid_flags/

https://codereview.chromium.org/11419093/diff/6002/android_webview/browser/re...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:55:
class ApplyWebSettingsResourceThrottle : public content::ResourceThrottle {
might as well call this IoThreadClientThrottle to match Ben's next change

[mnaganov (inactive), 2012-11-21 13:17:09]

https://codereview.chromium.org/11419093/diff/6002/android_webview/browser/re...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/11419093/diff/6002/android_webview/browser/re...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:32:
const int all_flags = net::LOAD_BYPASS_CACHE |
On 2012/11/21 12:35:15, Martin Kosiba wrote:
> nit: s/all_flags/valid_flags/

"valid_flags" doesn't make much sense to me. Renamed to
"all_cache_control_flags" for clarity.

https://codereview.chromium.org/11419093/diff/6002/android_webview/browser/re...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:55:
class ApplyWebSettingsResourceThrottle : public content::ResourceThrottle {
On 2012/11/21 12:35:15, Martin Kosiba wrote:
> might as well call this IoThreadClientThrottle to match Ben's next change

OK, renamed and updated the comment. But this will not save you from merge
conflicts!

[boliu, 2012-11-21 15:22:58]

lgtm

Remember to run through android_fyi_dbg trybot.

[mnaganov (inactive), 2012-11-21 15:50:35]

On 2012/11/21 15:22:58, boliu wrote:
> lgtm
> 
> Remember to run through android_fyi_dbg trybot.

Thanks! I did that for Patch Set 3. The next one only renames things.

[commit-bot: I haz the power, 2012-11-21 15:50:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/11419093/7002

[commit-bot: I haz the power, 2012-11-21 16:44:05]

Retried try job too often for step(s) content_unittests

[boliu, 2012-11-21 16:45:32]

Use NOTRY=true?

[mnaganov (inactive), 2012-11-21 16:57:32]

On 2012/11/21 16:45:32, boliu wrote:
> Use NOTRY=true?

This is because of your https://chromiumcodereview.appspot.com/11280064 -- you
have also moved clearCacheOnUiThread into AndroidWebViewTestBase.java but in a
different place

[boliu, 2012-11-21 16:59:23]

On 2012/11/21 16:57:32, Mikhail Naganov (Chromium) wrote:
> On 2012/11/21 16:45:32, boliu wrote:
> > Use NOTRY=true?
> 
> This is because of your https://chromiumcodereview.appspot.com/11280064 -- you
> have also moved clearCacheOnUiThread into AndroidWebViewTestBase.java but in a
> different place

Ahh, right...sorry. Slipped in before you there...

[commit-bot: I haz the power, 2012-11-21 19:02:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/11419093/7005

[commit-bot: I haz the power, 2012-11-21 19:04:46]

Change committed as 169082