Implement Android WebView BlockNetworkImages

Implement Android WebView BlockNetworkImages

This uses the WebCore ImagesEnabled setting, which is added to
ContentSettings. The setting also includes overriding
WebPermissionClient::allowImage, which here allows images with local
soucres to load as well.

Use a whitelist of local file schemes instead of blacklisting only
http(s).

BUG=


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

[boliu, 2012-08-31 01:20:14]

FYI. Will add reviewers when webkit change lands.

[mnaganov (inactive), 2012-08-31 09:10:45]

On 2012/08/31 01:20:14, boliu wrote:
> FYI. Will add reviewers when webkit change lands.

The implementation looks good, but please consider adding a test into
org.chromium.chrome.browser.test.ContentSettingsTest. You may look into a
corresponding CTS test (testBlockNetworkImage) here:
https://cs.corp.google.com/#android/cts/tests/tests/webkit/src/android/webkit...

[boliu, 2012-09-18 00:29:59]

WebKit patches landed, waiting for chromium webkit to roll past them:
http://trac.webkit.org/changeset/128645
http://trac.webkit.org/changeset/128780

No tests yet since I can't get existing AwContentSettingsTest to run without
failing/crashing. Will also need the test server support.

[mnaganov_google.com, 2012-09-18 08:55:30]

On Tue, Sep 18, 2012 at 1:29 AM,  <boliu@chromium.org> wrote:
> WebKit patches landed, waiting for chromium webkit to roll past them:
> http://trac.webkit.org/changeset/128645
> http://trac.webkit.org/changeset/128780
>

That's awesome!

> No tests yet since I can't get existing AwContentSettingsTest to run without
> failing/crashing. Will also need the test server support.
>

I guess, you mean upstream? Because I run them dowstream on a regular
basis and they work for me.

> http://codereview.chromium.org/10920033/
>
> --
>
>

[boliu, 2012-09-25 00:37:03]

Patch set 3 has one test that does not require http server. It passes downstream
and builds upstream. Obviously this does not test everything since this is a
network related setting.

Not sure what the state of upstreaming test server/forwarder is right now.

I can add more tests downstream, but not sure if it is worth diverging. The
webkit portion is pretty well tested with layout tests. The only additional
logic here is allowImage/isLocalScheme. I was considering adding a native
unittest target for testing allowImage/isLocalScheme. Any thoughts on this?

Update on webkit patch: https://bugs.webkit.org/show_bug.cgi?id=97055
I tried landing it today but it got reverted due to a logic mistake on my part.
Mistake fixed without perf regression, will try landing again tomorrow.

[mnaganov (inactive), 2012-09-25 09:55:53]

https://chromiumcodereview.appspot.com/10920033/diff/7001/android_webview/com...
File android_webview/common/aw_utils.cc (right):

https://chromiumcodereview.appspot.com/10920033/diff/7001/android_webview/com...
android_webview/common/aw_utils.cc:12: bool isLocalScheme(const GURL& url) {
I think this may not work in case of GMail, because it uses a custom scheme as a
base URL, so all images will relative URLs will be using that scheme and
presumably will be blocked. Perhaps, switch to using a whitelist approach for
networking schemes instead?

https://chromiumcodereview.appspot.com/10920033/diff/7001/android_webview/ren...
File android_webview/renderer/aw_render_view_ext.cc (right):

https://chromiumcodereview.appspot.com/10920033/diff/7001/android_webview/ren...
android_webview/renderer/aw_render_view_ext.cc:59: if (enabledPerSettings ||
isLocalScheme(GURL(imageURL)))
Perhaps, simplify as 'return enabledPerSettings...'?

https://chromiumcodereview.appspot.com/10920033/diff/7001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://chromiumcodereview.appspot.com/10920033/diff/7001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:727:
* Note that this does not block image loads from WebCore cache.
...that setting this flag to false does not block...

https://chromiumcodereview.appspot.com/10920033/diff/7001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:754:
* Note that unlike LoadsImagesAutomatically, this will block image loads
LoadImagesAutomatically -> {@link #setLoadsImagesAutomatically}

https://chromiumcodereview.appspot.com/10920033/diff/7001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:754:
* Note that unlike LoadsImagesAutomatically, this will block image loads
...setting this flag to false will block...

https://chromiumcodereview.appspot.com/10920033/diff/7001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:755:
* from WebCore cache as well.
Please add "The default is true."

https://chromiumcodereview.appspot.com/10920033/diff/7001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:756:
* @param flag True if the WebView should enable images.
whether the WebView should enable images

[boliu, 2012-09-25 16:55:12]

All the comments for javadoc in ContentSettings done too.

http://codereview.chromium.org/10920033/diff/7001/android_webview/common/aw_u...
File android_webview/common/aw_utils.cc (right):

http://codereview.chromium.org/10920033/diff/7001/android_webview/common/aw_u...
android_webview/common/aw_utils.cc:12: bool isLocalScheme(const GURL& url) {
On 2012/09/25 09:55:53, Mikhail Naganov (Chromium) wrote:
> I think this may not work in case of GMail, because it uses a custom scheme as
a
> base URL, so all images will relative URLs will be using that scheme and
> presumably will be blocked. Perhaps, switch to using a whitelist approach for
> networking schemes instead?

I thought I could reuse this for BlockNetworkLoads but since we are choosing to
blacklist, then that might no longer be the case. For now removed this file and
the changes in url_constants. If similar logic can be reused in
BlockNetworkLoads, then do the refactor then.

http://codereview.chromium.org/10920033/diff/7001/android_webview/renderer/aw...
File android_webview/renderer/aw_render_view_ext.cc (right):

http://codereview.chromium.org/10920033/diff/7001/android_webview/renderer/aw...
android_webview/renderer/aw_render_view_ext.cc:59: if (enabledPerSettings ||
isLocalScheme(GURL(imageURL)))
On 2012/09/25 09:55:53, Mikhail Naganov (Chromium) wrote:
> Perhaps, simplify as 'return enabledPerSettings...'?

Done.

[mnaganov (inactive), 2012-09-25 16:58:30]

lgtm

[joth, 2012-09-25 19:10:47]

lgtm % nit

http://codereview.chromium.org/10920033/diff/16001/android_webview/renderer/a...
File android_webview/renderer/aw_render_view_ext.cc (right):

http://codereview.chromium.org/10920033/diff/16001/android_webview/renderer/a...
android_webview/renderer/aw_render_view_ext.cc:56: const WebKit::WebURL&
imageURL) {
this is chrome code, so use enabled_per_settings, image_url etc

[boliu, 2012-09-25 20:16:18]

+Yaron for content/android/public

New test so PTAL.

Going to make sure the webkit patch sticks in chromium before landing this.

The new tests runs and passes downstream. Also the one requiring TestWebServer
is pretty stable; I ran it 50 times without failure.

I couldn't get any of the java tests under android_webview to run upstream, but
Ben mentioned they should be working, so it's probably a set up problem on my
end. Will look into it.

[joth, 2012-09-25 20:28:21]

new test lgtm.

http://codereview.chromium.org/10920033/diff/13004/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(right):

http://codereview.chromium.org/10920033/diff/13004/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1357:
@SmallTest
both these tests load pages and wait for load to complete, so should be
MediumTest I believe.

http://codereview.chromium.org/10920033/diff/13004/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/TestWebServer.java
(right):

http://codereview.chromium.org/10920033/diff/13004/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/TestWebServer.java:161:
public String setResponseBase64(
java doc?

http://codereview.chromium.org/10920033/diff/13004/android_webview/renderer/a...
File android_webview/renderer/aw_render_view_ext.cc (right):

http://codereview.chromium.org/10920033/diff/13004/android_webview/renderer/a...
android_webview/renderer/aw_render_view_ext.cc:56: const WebKit::WebURL&
imageURL) {
nit : imageURL => image_url

[Yaron, 2012-09-25 20:29:21]

content/public/android lgtm

[boliu, 2012-09-25 20:52:28]

http://codereview.chromium.org/10920033/diff/13004/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(right):

http://codereview.chromium.org/10920033/diff/13004/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1357:
@SmallTest
On 2012/09/25 20:28:21, joth wrote:
> both these tests load pages and wait for load to complete, so should be
> MediumTest I believe. 

Pretty much every test in this file fits that description, and currently they
are all annotated as SmallTest. Also these new tests run pretty fast.

Going to stick with SmallTest here for consistency. We could update all the
annotations in one go later if we decide these are incorrect.

http://codereview.chromium.org/10920033/diff/13004/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/TestWebServer.java
(right):

http://codereview.chromium.org/10920033/diff/13004/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/TestWebServer.java:161:
public String setResponseBase64(
On 2012/09/25 20:28:21, joth wrote:
> java doc? 

Done.

http://codereview.chromium.org/10920033/diff/13004/android_webview/renderer/a...
File android_webview/renderer/aw_render_view_ext.cc (right):

http://codereview.chromium.org/10920033/diff/13004/android_webview/renderer/a...
android_webview/renderer/aw_render_view_ext.cc:56: const WebKit::WebURL&
imageURL) {
On 2012/09/25 20:28:21, joth wrote:
> nit : imageURL => image_url

stupid me...Done

[joth, 2012-09-25 22:21:45]

On 2012/09/25 20:52:28, boliu wrote:
>
http://codereview.chromium.org/10920033/diff/13004/android_webview/javatests/...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
> (right):
> 
>
http://codereview.chromium.org/10920033/diff/13004/android_webview/javatests/...
>
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1357:
> @SmallTest
> On 2012/09/25 20:28:21, joth wrote:
> > both these tests load pages and wait for load to complete, so should be
> > MediumTest I believe. 
> 
> Pretty much every test in this file fits that description, and currently they
> are all annotated as SmallTest. Also these new tests run pretty fast.
> 
> Going to stick with SmallTest here for consistency. We could update all the
> annotations in one go later if we decide these are incorrect.
> 
> 
SGTM


http://codereview.chromium.org/10920033/diff/13004/android_webview/javatests/...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/TestWebServer.java
> (right):
> 
>
http://codereview.chromium.org/10920033/diff/13004/android_webview/javatests/...
>
android_webview/javatests/src/org/chromium/android_webview/test/TestWebServer.java:161:
> public String setResponseBase64(
> On 2012/09/25 20:28:21, joth wrote:
> > java doc? 
> 
> Done.
> 
>
http://codereview.chromium.org/10920033/diff/13004/android_webview/renderer/a...
> File android_webview/renderer/aw_render_view_ext.cc (right):
> 
>
http://codereview.chromium.org/10920033/diff/13004/android_webview/renderer/a...
> android_webview/renderer/aw_render_view_ext.cc:56: const WebKit::WebURL&
> imageURL) {
> On 2012/09/25 20:28:21, joth wrote:
> > nit : imageURL => image_url
> 
> stupid me...Done

[commit-bot: I haz the power, 2012-09-26 14:27:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/10920033/15002

[commit-bot: I haz the power, 2012-09-26 16:23:57]

Change committed as 158809