[Android] Implement WebSettings.setAppCache{Enabled|Path}

[Android] Implement WebSettings.setAppCache{Enabled|Path}

AppCacheEnabled is mapped onto WebPreferences.application_cache_enabled, which goes directly into WebKit Settings.

AppCachePath is only used as a flag to enable AppCache. We can't make use of the full path given, because in Chromium the Application Cache directory lives inside the browser context (profile).

The tests added trigger a DCHECK in disk cache, unless the profile is empty when the test starts. This makes impossible to run them both now, so only one of them is enabled for now.

Android CTS tests WebSettings.testAppCache{Disabled|Enabled} are passing with this patch.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171074

[mnaganov (inactive), 2012-11-30 17:11:16]

Guys, please take a look.

I plan to deal with the cache-related crash, as I how a clear reproduction with
my tests.

[boliu, 2012-11-30 17:46:27]

https://codereview.chromium.org/11411229/diff/9001/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(right):

https://codereview.chromium.org/11411229/diff/9001/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2280:
clearCacheOnUiThread(views.getContents1(), true);
clearCache is global to this app, only need to call it once.

But if clearCache doesn't help with the disk cache crash, then it really is a
separate issue from crbug.com/154805

https://codereview.chromium.org/11411229/diff/9001/content/browser/android/co...
File content/browser/android/content_settings.cc (right):

https://codereview.chromium.org/11411229/diff/9001/content/browser/android/co...
content/browser/android/content_settings.cc:364:
CR_DEFINE_STATIC_LOCAL(std::string, app_cache_path, ());
This is only global in WebViews, not inside webkit, right?

[joth, 2012-11-30 22:16:59]

(didn't read whole patch)

https://codereview.chromium.org/11411229/diff/9001/android_webview/browser/aw...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11411229/diff/9001/android_webview/browser/aw...
android_webview/browser/aw_content_browser_client.cc:118: // instead AppCache
can be disabled for the whole WebView instance.
nit: whole WV instance, or whole application?

https://codereview.chromium.org/11411229/diff/9001/content/browser/android/co...
File content/browser/android/content_settings.cc (right):

https://codereview.chromium.org/11411229/diff/9001/content/browser/android/co...
content/browser/android/content_settings.cc:364:
CR_DEFINE_STATIC_LOCAL(std::string, app_cache_path, ());
On 2012/11/30 17:46:27, boliu wrote:
> This is only global in WebViews, not inside webkit, right?

also consider:
- CR_DEFINE_STATIC_LOCAL is not thread safe (neither construction of the global
or usage of it).
- use a different name to the FieldId member, to make this easier to read.
(g_app_cache_path would be reasonable).
- you only seem to be using this as a bool, so having a static bool maybe
simpler.
- I think all this patch is only needed for webview, so could we just implement
it in AwSettings? e.g. just put the policy we want right there on the java side?

[mnaganov (inactive), 2012-12-03 15:07:36]

Thanks for the comments!

https://codereview.chromium.org/11411229/diff/9001/android_webview/browser/aw...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11411229/diff/9001/android_webview/browser/aw...
android_webview/browser/aw_content_browser_client.cc:118: // instead AppCache
can be disabled for the whole WebView instance.
On 2012/11/30 22:16:59, joth wrote:
> nit: whole WV instance, or whole application?

Per instance. Reworded to avoid confusion.

https://codereview.chromium.org/11411229/diff/9001/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(right):

https://codereview.chromium.org/11411229/diff/9001/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2280:
clearCacheOnUiThread(views.getContents1(), true);
On 2012/11/30 17:46:27, boliu wrote:
> clearCache is global to this app, only need to call it once.
> 
> But if clearCache doesn't help with the disk cache crash, then it really is a
> separate issue from crbug.com/154805

Well, then it looks a bit confusing that it requires AwContents. I've duplicated
the comment to AwContents.clearCache into
AndroidWebViewTestBase.clearCacheOnUiThread.

https://codereview.chromium.org/11411229/diff/9001/content/browser/android/co...
File content/browser/android/content_settings.cc (right):

https://codereview.chromium.org/11411229/diff/9001/content/browser/android/co...
content/browser/android/content_settings.cc:364:
CR_DEFINE_STATIC_LOCAL(std::string, app_cache_path, ());
On 2012/11/30 22:16:59, joth wrote:
> On 2012/11/30 17:46:27, boliu wrote:
> > This is only global in WebViews, not inside webkit, right?
> 
> also consider:
> - CR_DEFINE_STATIC_LOCAL is not thread safe (neither construction of the
global
> or usage of it).
> - use a different name to the FieldId member, to make this easier to read.
> (g_app_cache_path would be reasonable).
> - you only seem to be using this as a bool, so having a static bool maybe
> simpler.
> - I think all this patch is only needed for webview, so could we just
implement
> it in AwSettings? e.g. just put the policy we want right there on the java
side?

I know it's not thread safe, but I was supposing that our gross synchronization
code serializes calls to SyncToNative, because they all go through the UI thread
and are invoked via message loops.

But I agree with your other considerations, and I have moved the code into the
Java side.

https://codereview.chromium.org/11411229/diff/9001/content/browser/android/co...
content/browser/android/content_settings.cc:364:
CR_DEFINE_STATIC_LOCAL(std::string, app_cache_path, ());
On 2012/11/30 17:46:27, boliu wrote:
> This is only global in WebViews, not inside webkit, right?

The cache storage is global in WebKit, see ApplicationCacheStorage.cpp

[boliu, 2012-12-03 17:14:43]

lgtm but should wait for joth

https://codereview.chromium.org/11411229/diff/9001/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(right):

https://codereview.chromium.org/11411229/diff/9001/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2280:
clearCacheOnUiThread(views.getContents1(), true);
On 2012/12/03 15:07:36, Mikhail Naganov (Chromium) wrote:
> On 2012/11/30 17:46:27, boliu wrote:
> > clearCache is global to this app, only need to call it once.
> > 
> > But if clearCache doesn't help with the disk cache crash, then it really is
a
> > separate issue from crbug.com/154805
> 
> Well, then it looks a bit confusing that it requires AwContents. I've
duplicated
> the comment to AwContents.clearCache into
> AndroidWebViewTestBase.clearCacheOnUiThread.

True, though we will need an ipc channel somewhere, and easiest way to get it is
from a AwContents. (the other less satisfying reason is the method is in the
webview class as well)

https://codereview.chromium.org/11411229/diff/14/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://codereview.chromium.org/11411229/diff/14/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:847:
return mPluginState == PluginState.OFF;
this is removing unneeded synchronized and not really related to this patch,
right?

[mnaganov (inactive), 2012-12-03 17:19:46]

https://codereview.chromium.org/11411229/diff/14/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://codereview.chromium.org/11411229/diff/14/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:847:
return mPluginState == PluginState.OFF;
On 2012/12/03 17:14:43, boliu wrote:
> this is removing unneeded synchronized and not really related to this patch,
> right?

Correct. I started looking into how the existing code works, and noticed
unneeded synchronization.

[joth, 2012-12-04 00:15:42]

lgtm

https://codereview.chromium.org/11411229/diff/14/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://codereview.chromium.org/11411229/diff/14/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:91:
// provided. However, we doesn't use the path, so we just check if we have
received it from the
nit: we doesn't => we don't.

https://codereview.chromium.org/11411229/diff/14/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:94:
private static boolean mAppCachePathIsSet = false;
s is for static, not m.

sGlobalFooLock
sAppCachePathIsSet

is 'global' needed in a var name if it has the s prefix?

the comment above seems to apply to the boolean. add Comment for the lock, and
move that comment down?

https://codereview.chromium.org/11411229/diff/14/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:952:
* @param appCachePath a String path to the directory containing
nit: appCachePath -> path

also the comments make it sound like this is actually used.

[mnaganov (inactive), 2012-12-04 11:11:56]

Thanks!

https://codereview.chromium.org/11411229/diff/14/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://codereview.chromium.org/11411229/diff/14/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:91:
// provided. However, we doesn't use the path, so we just check if we have
received it from the
On 2012/12/04 00:15:42, joth wrote:
> nit: we doesn't => we don't.

Done.

https://codereview.chromium.org/11411229/diff/14/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:94:
private static boolean mAppCachePathIsSet = false;
On 2012/12/04 00:15:42, joth wrote:
> s is for static, not m.
> 
> sGlobalFooLock
> sAppCachePathIsSet
>

Done.
 
> is 'global' needed in a var name if it has the s prefix?
>

It think it's still needed, because visual difference between
'mContentSettingsLock' and 'sContentSettingsLock' is subtle.
 
> the comment above seems to apply to the boolean. add Comment for the lock, and
> move that comment down?

Done.

https://codereview.chromium.org/11411229/diff/14/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:952:
* @param appCachePath a String path to the directory containing
On 2012/12/04 00:15:42, joth wrote:
> nit: appCachePath -> path
> 
> also the comments make it sound like this is actually used.

I just copied the comments verbatim from the interface. Updated both to reflect
the actual state.

[commit-bot: I haz the power, 2012-12-04 11:24:49]

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

[commit-bot: I haz the power, 2012-12-04 11:24:57]

Presubmit check for 11411229-12003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public/android

Presubmit checks took 1.3s to calculate.

[mnaganov (inactive), 2012-12-04 11:26:57]

Marcus, please take a look at the content/public/android stuff

[bulach, 2012-12-04 17:16:49]

lgtm, small suggestion:

https://codereview.chromium.org/11411229/diff/12003/android_webview/javatests...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(right):

https://codereview.chromium.org/11411229/diff/12003/android_webview/javatests...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2311:
Thread.sleep(1000);
please the same comments from 2243..

[mnaganov (inactive), 2012-12-04 18:08:31]

Thanks!

https://codereview.chromium.org/11411229/diff/12003/android_webview/javatests...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(right):

https://codereview.chromium.org/11411229/diff/12003/android_webview/javatests...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:2311:
Thread.sleep(1000);
On 2012/12/04 17:16:49, bulach wrote:
> please the same comments from 2243..

Done.

[commit-bot: I haz the power, 2012-12-04 18:10:59]

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

[commit-bot: I haz the power, 2012-12-04 22:55:44]

Change committed as 171074