[Android] Upstream WebView.allow{Content|File}Access implementation.

[Android] Upstream WebView.allow{Content|File}Access implementation.

R=benm@chromium.org,joth@chromium.org
BUG=153516
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161103

[mnaganov (inactive), 2012-10-08 14:52:44]

@abarth: FYI. I will remove Android-specific downstream code that deals with the
corresponding WebPreferences afterwards.

[abarth-chromium, 2012-10-08 16:40:22]

On 2012/10/08 14:52:44, Mikhail Naganov (Chromium) wrote:
> @abarth: FYI. I will remove Android-specific downstream code that deals with
the
> corresponding WebPreferences afterwards.

If I understand this patch correct, you're moving this logic even higher in the
stack into webview-specific Java code?  If so, that sounds great.

[joth, 2012-10-08 17:29:01]

lgtm, only nits.
(I somewhat skimmed over the test code being re-enabled; assume this was all
reviewed at least once already)

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

https://codereview.chromium.org/11090003/diff/2001/android_webview/browser/re...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:68:
request->url().SchemeIs(android_webview::kContentScheme)) {
nit: swap expression order:  if (SchemeIs(content) && io_client->ShouldBlock())
as this will avoid JNI in the common case that it's not content: url.

https://codereview.chromium.org/11090003/diff/2001/android_webview/browser/re...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:73:
if (io_client->ShouldBlockFileUrls() && request->url().SchemeIsFile()) {
ditto

https://codereview.chromium.org/11090003/diff/2001/android_webview/browser/re...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:108:
void AwResourceDispatcherHostDelegate::SetAllowOnlyLoadFromCache(
nit: maybe easier to read as SetOnlyAllowLoadFromCache

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

https://codereview.chromium.org/11090003/diff/2001/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1690:
* multiple WebViews.
nit: maybe explicitly call out it tests all four combintations of helper0{true,
false} x helper1{true, false}

(The code here is just complex enough that I couldn't easily see we do the
security critical (trivial) tests of  setAllow[File|Content]Access(false) at
all, due to the double indirection via the helpers & this shared test-run
method. I wouldn't be against adding those as very explicit stand-alone test
cases too, just to ensure the basic test is always running regardless of the
multiple webview scenarios)

https://codereview.chromium.org/11090003/diff/2001/android_webview/lib/aw_con...
File android_webview/lib/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11090003/diff/2001/android_webview/lib/aw_con...
android_webview/lib/aw_content_browser_client.cc:25: // access checks.
hmm I guess technically we could leave this and do it lazily when a WebView
attaches to this renderer with AllowContent = true. however as that's the
default it would be the common case so very little gain (but a lot of
complexity) in moving it out of here.

Worth a comment here to mention this? Or just leave as is if you prefer.

[mnaganov (inactive), 2012-10-09 09:59:35]

Thanks, Jonathan!

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

https://codereview.chromium.org/11090003/diff/2001/android_webview/browser/re...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:68:
request->url().SchemeIs(android_webview::kContentScheme)) {
On 2012/10/08 17:29:01, joth wrote:
> nit: swap expression order:  if (SchemeIs(content) &&
io_client->ShouldBlock())
> as this will avoid JNI in the common case that it's not content: url.

Done.

https://codereview.chromium.org/11090003/diff/2001/android_webview/browser/re...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:73:
if (io_client->ShouldBlockFileUrls() && request->url().SchemeIsFile()) {
On 2012/10/08 17:29:01, joth wrote:
> ditto

Done.

https://codereview.chromium.org/11090003/diff/2001/android_webview/browser/re...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:108:
void AwResourceDispatcherHostDelegate::SetAllowOnlyLoadFromCache(
On 2012/10/08 17:29:01, joth wrote:
> nit: maybe easier to read as SetOnlyAllowLoadFromCache 

Done.

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

https://codereview.chromium.org/11090003/diff/2001/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1690:
* multiple WebViews.
On 2012/10/08 17:29:01, joth wrote:
> nit: maybe explicitly call out it tests all four combintations of
helper0{true,
> false} x helper1{true, false}
> 
> (The code here is just complex enough that I couldn't easily see we do the
> security critical (trivial) tests of  setAllow[File|Content]Access(false) at
> all, due to the double indirection via the helpers & this shared test-run
> method. I wouldn't be against adding those as very explicit stand-alone test
> cases too, just to ensure the basic test is always running regardless of the
> multiple webview scenarios)

I updated the comment, hopefully to make things more clear.

The checks you are talking about are done by calls to 'setAlteredSettingValue'
and 'ensureSettingHasAlteredValue', sorry if it's not easy to tell from the
first glance. This boils down to calling
AwSettingsFileUrlAccessTestHelper.doEnsureSettingHasValue, which handles both
'enabled' and 'disabled' checks.

https://codereview.chromium.org/11090003/diff/2001/android_webview/lib/aw_con...
File android_webview/lib/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11090003/diff/2001/android_webview/lib/aw_con...
android_webview/lib/aw_content_browser_client.cc:25: // access checks.
On 2012/10/08 17:29:01, joth wrote:
> hmm I guess technically we could leave this and do it lazily when a WebView
> attaches to this renderer with AllowContent = true. however as that's the
> default it would be the common case so very little gain (but a lot of
> complexity) in moving it out of here.
> 
> Worth a comment here to mention this? Or just leave as is if you prefer.

In that case we will end up with having different paths for rejecting a
'content:' url navigation -- either due to blocking by
ChildProcessSecurityPolicy or by our delegate. And I'm not sure we really want
that, as this may produce different error messages, which is undesired.

[joth, 2012-10-09 17:13:17]

lgtm

[commit-bot: I haz the power, 2012-10-10 09:52:35]

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

[commit-bot: I haz the power, 2012-10-10 09:52:42]

Presubmit check for 11090003-1019 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  android_webview/native/android_protocol_handler.cc:146
    static void SetResourceContextForTesting(JNIEnv* env, jclass /*clazz*/,

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

Presubmit checks took 1.5s to calculate.

[mnaganov (inactive), 2012-10-10 10:08:18]

Satish, can you please take a look? I'm moving out code from 	
content/public/android/ContentSettings.java, need an owner's LGTM.

[Satish, 2012-10-10 10:13:13]

lgtm for content/public/android

[commit-bot: I haz the power, 2012-10-10 10:14:30]

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

[commit-bot: I haz the power, 2012-10-10 12:41:43]

Change committed as 161103