[Android WebView] Implement WebSettings.{get|set}CacheMode (issue 11419093)

[mnag...@chromium.org]

Reviewers: boliu,

Description:
[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=bo...@chromium.org


Please review this at https://codereview.chromium.org/11419093/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M android_webview/browser/aw_contents_io_thread_client.h
M
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
M android_webview/java/src/org/chromium/android_webview/AwContents.java
M
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java
M android_webview/java/src/org/chromium/android_webview/AwSettings.java
M
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
M android_webview/native/aw_contents_io_thread_client_impl.h
M android_webview/native/aw_contents_io_thread_client_impl.cc

[bo...@chromium.org]

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/renderer_host/aw_resource_dispatcher_host_delegate.cc
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/11419093/diff/1/android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc#newcode92
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/renderer_host/aw_resource_dispatcher_host_delegate.cc#newcode119
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/chromium/android_webview/AwContents.java
File
android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/11419093/diff/1/android_webview/java/src/org/chromium/android_webview/AwContents.java#newcode131
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/chromium/android_webview/AwContentsIoThreadClient.java
File
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java
(right):

https://codereview.chromium.org/11419093/diff/1/android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java#newcode19
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?

https://codereview.chromium.org/11419093/

[mnag...@chromium.org]

Thanks for comments!

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

https://codereview.chromium.org/11419093/diff/1/android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc#newcode92
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/renderer_host/aw_resource_dispatcher_host_delegate.cc#newcode119
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/chromium/android_webview/AwContents.java
File
android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/11419093/diff/1/android_webview/java/src/org/chromium/android_webview/AwContents.java#newcode131
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/chromium/android_webview/AwContentsIoThreadClient.java
File
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java
(right):

https://codereview.chromium.org/11419093/diff/1/android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java#newcode19
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".

https://codereview.chromium.org/11419093/

[mnag...@chromium.org]

https://codereview.chromium.org/11419093/

[mko...@chromium.org]

lgtm




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

https://codereview.chromium.org/11419093/diff/6002/android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc#newcode32
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/renderer_host/aw_resource_dispatcher_host_delegate.cc#newcode55
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

https://codereview.chromium.org/11419093/

[mnag...@chromium.org]

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

https://codereview.chromium.org/11419093/diff/6002/android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc#newcode32
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/renderer_host/aw_resource_dispatcher_host_delegate.cc#newcode55
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!

https://codereview.chromium.org/11419093/

[bo...@chromium.org]

lgtm

Remember to run through android_fyi_dbg trybot.

https://codereview.chromium.org/11419093/

[mnag...@chromium.org]

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.

https://codereview.chromium.org/11419093/

[commi...@chromium.org]

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


https://chromiumcodereview.appspot.com/11419093/

[commi...@chromium.org]

Retried try job too often for step(s) content_unittests

https://chromiumcodereview.appspot.com/11419093/

[bo...@chromium.org]

Use NOTRY=true?

https://chromiumcodereview.appspot.com/11419093/

[mnag...@chromium.org]

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

https://codereview.chromium.org/11419093/

[bo...@chromium.org]

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...

https://codereview.chromium.org/11419093/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/mnag...@chromium.org/11419093/7005


https://chromiumcodereview.appspot.com/11419093/

[commi...@chromium.org]

Change committed as 169082

https://chromiumcodereview.appspot.com/11419093/