[Android WebView] Fix LoadsImagesAutomatically tests after crbug.com/224317

[Android WebView] Fix LoadsImagesAutomatically tests after crbug.com/224317

Blink now has more reasonable behavior of LoadsImagesAutomatically and
ImagesDisabled settings. Namely, ImagesDisabled prevents any images from
showing up, including cached ones. LoadsImagesAutomatically now allows
"loading" of data uri images embedded in pages.

This allows us to simplify our wirings for implementing Android WebView's
LoadsImagesAutomatically and BlockNetworkImage. The trick is that we should
wire them cris-cross, as WebView's LoadsImagesAutomatically must block
all images from loading / showing up (as ImagesDisabled), while 
BlockNetworkImage's contract is now fulfilled by Blink's
LoadsImagesAutomatically. Please read comments on the bugs referenced
if this sounds weird to you.

AwSettingsTest.testLoadsImagesAutomaticallyWithCachedImage is removed because
this behavior is not in fact standardized in WebView Classic and isn't
preserved anymore. In fact, as cache is transient, it would be unwise for an
application to rely on the fact that cached images will stay there forever
and thus could be shown even if LoadsImagesAutomatically is disabled.

BUG=248249, 224317
R=benm@chromium.org, joth@chromium.org

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

[mnaganov (inactive), 2013-06-13 16:23:05]

Ben, this seems to conflict with your https://codereview.chromium.org/16940009/
where you are implementing more methods of WebPermissionClient. I can only
remove allowImage to avoid the conflict.

[benm (inactive), 2013-06-13 16:31:04]

On 2013/06/13 16:23:05, Mikhail Naganov (Chromium) wrote:
> Ben, this seems to conflict with your
https://codereview.chromium.org/16940009/
> where you are implementing more methods of WebPermissionClient. I can only
> remove allowImage to avoid the conflict.

Thanks for the heads up; that would be helpful if you don't mind :)

[mnaganov (inactive), 2013-06-13 16:34:10]

On 2013/06/13 16:31:04, benm wrote:
> On 2013/06/13 16:23:05, Mikhail Naganov (Chromium) wrote:
> > Ben, this seems to conflict with your
> https://codereview.chromium.org/16940009/
> > where you are implementing more methods of WebPermissionClient. I can only
> > remove allowImage to avoid the conflict.
> 
> Thanks for the heads up; that would be helpful if you don't mind :)

Done!

[mnaganov (inactive), 2013-06-13 16:51:50]

On 2013/06/13 16:34:10, Mikhail Naganov (Chromium) wrote:
> On 2013/06/13 16:31:04, benm wrote:
> > On 2013/06/13 16:23:05, Mikhail Naganov (Chromium) wrote:
> > > Ben, this seems to conflict with your
> > https://codereview.chromium.org/16940009/
> > > where you are implementing more methods of WebPermissionClient. I can only
> > > remove allowImage to avoid the conflict.
> > 
> > Thanks for the heads up; that would be helpful if you don't mind :)
> 
> Done!

Ah, the bots are using an "old" revision of Blink, it needs to be at least
r152322, so I have to wait for the roll and LKGR being updated.

[benm (inactive), 2013-06-13 16:54:27]

lgtm

[joth, 2013-06-13 17:27:14]

lgtm thanks for the quick reaction on this Mikhail.

https://codereview.chromium.org/16932006/diff/11001/android_webview/native/aw...
File android_webview/native/aw_settings.cc (right):

https://codereview.chromium.org/16932006/diff/11001/android_webview/native/aw...
android_webview/native/aw_settings.cc:159: // Blink's LoadsImagesAutomatically
and ImagesEnabled should be
nit: should -> must.
(coupled with a bug link, I thought this was a comment in future tense "we
should consider.....". Silly language English).

[abarth-chromium, 2013-06-13 18:20:10]

https://codereview.chromium.org/16932006/diff/11001/android_webview/renderer/...
File android_webview/renderer/aw_render_view_ext.cc (left):

https://codereview.chromium.org/16932006/diff/11001/android_webview/renderer/...
android_webview/renderer/aw_render_view_ext.cc:172: const WebKit::WebURL&
image_url) {
Oh, it's great that you don't need this override anymore.

[joth, 2013-06-14 00:25:25]

https://codereview.chromium.org/16932006/diff/11001/android_webview/renderer/...
File android_webview/renderer/aw_render_view_ext.cc (left):

https://codereview.chromium.org/16932006/diff/11001/android_webview/renderer/...
android_webview/renderer/aw_render_view_ext.cc:172: const WebKit::WebURL&
image_url) {
On 2013/06/13 18:20:10, abarth wrote:
> Oh, it's great that you don't need this override anymore.

Hmmm I looked again, and I'm not sure we don't?
This functionality is used by setBlockNetworkImage which was not the subject of
this CL, is still supported (AFAIK) and is covered by the still existing (but
alas, disaled)  
testBlockNetworkImagesBlocksNetworkImageAndReloadInPlace test.

mnaganov - can you confirm this is not needed for AwSettings.setImagesEnabled()
?

[joth, 2013-06-14 00:26:07]

(so not lgtm, pending confirmation.)

[mnaganov (inactive), 2013-06-14 06:05:29]

On 2013/06/14 00:25:25, joth wrote:
>
https://codereview.chromium.org/16932006/diff/11001/android_webview/renderer/...
> File android_webview/renderer/aw_render_view_ext.cc (left):
> 
>
https://codereview.chromium.org/16932006/diff/11001/android_webview/renderer/...
> android_webview/renderer/aw_render_view_ext.cc:172: const WebKit::WebURL&
> image_url) {
> On 2013/06/13 18:20:10, abarth wrote:
> > Oh, it's great that you don't need this override anymore.
> 
> Hmmm I looked again, and I'm not sure we don't?
> This functionality is used by setBlockNetworkImage which was not the subject
of
> this CL, is still supported (AFAIK) and is covered by the still existing (but
> alas, disaled)  
> testBlockNetworkImagesBlocksNetworkImageAndReloadInPlace test.
> 
> mnaganov - can you confirm this is not needed for
AwSettings.setImagesEnabled()
> ?

Jonathan, I'm sorry if this CL alone doesn't provide enough context. I would
like to kindly ask you to re-read comments crbug.com/224317#c25 and
crbug.com/224317#c26. To implement setBlockNetworkImage we are now using Blink's
setLoadsImagesAutomatically because after pdr@'s patch it is really preventing
images from being downloaded from network, while allowing data uri images to be
displayed. I have checked
testBlockNetworkImagesBlocksNetworkImageAndReloadInPlace, and it actually
passes, but as there is a note on its flakiness I decided not to enable it in
this patch. I will investigate this and will try to enable it back.

[joth, 2013-06-14 20:29:45]

On 2013/06/14 06:05:29, Mikhail Naganov (Chromium) wrote:
> On 2013/06/14 00:25:25, joth wrote:
> >
>
https://codereview.chromium.org/16932006/diff/11001/android_webview/renderer/...
> > File android_webview/renderer/aw_render_view_ext.cc (left):
> > 
> >
>
https://codereview.chromium.org/16932006/diff/11001/android_webview/renderer/...
> > android_webview/renderer/aw_render_view_ext.cc:172: const WebKit::WebURL&
> > image_url) {
> > On 2013/06/13 18:20:10, abarth wrote:
> > > Oh, it's great that you don't need this override anymore.
> > 
> > Hmmm I looked again, and I'm not sure we don't?
> > This functionality is used by setBlockNetworkImage which was not the subject
> of
> > this CL, is still supported (AFAIK) and is covered by the still existing
(but
> > alas, disaled)  
> > testBlockNetworkImagesBlocksNetworkImageAndReloadInPlace test.
> > 
> > mnaganov - can you confirm this is not needed for
> AwSettings.setImagesEnabled()
> > ?
> 
> Jonathan, I'm sorry if this CL alone doesn't provide enough context. I would
> like to kindly ask you to re-read comments crbug.com/224317#c25 and
> crbug.com/224317#c26. To implement setBlockNetworkImage we are now using
Blink's
> setLoadsImagesAutomatically because after pdr@'s patch it is really preventing
> images from being downloaded from network, while allowing data uri images to
be
> displayed. I have checked
> testBlockNetworkImagesBlocksNetworkImageAndReloadInPlace, and it actually
> passes, but as there is a note on its flakiness I decided not to enable it in
> this patch. I will investigate this and will try to enable it back.

lgtm

awesome, was just wanting to double check setBlockNetworkImage hadn't been
forgotten (the discussion on
https://code.google.com/p/chromium/issues/detail?id=224317#c25 does indeed
explain all, so I added that to the BUG= link here)
Might make sense to add another bit to this CL description just to mention this
is also re-implementing setBlockNetworkImage in terms of
WebSettings.setLoadsImagesAutomatically  rather than
WebPermissionClient::allowImage - I could add that, just wanted to make sure I
got the names right!

Thanks

[mnaganov (inactive), 2013-06-17 08:56:53]

I added more explanations to the description, hopefully it is clearer now.

https://codereview.chromium.org/16932006/diff/11001/android_webview/native/aw...
File android_webview/native/aw_settings.cc (right):

https://codereview.chromium.org/16932006/diff/11001/android_webview/native/aw...
android_webview/native/aw_settings.cc:159: // Blink's LoadsImagesAutomatically
and ImagesEnabled should be
On 2013/06/13 17:27:14, joth wrote:
> nit: should -> must.
> (coupled with a bug link, I thought this was a comment in future tense "we
> should consider.....". Silly language English).

Done.

[mnaganov (inactive), 2013-06-17 09:21:01]

Committed patchset #3 manually as r206692 (presubmit successful).