Block media loading when AwSettings.setBlockNetworkLoads is true.

Prevent media from being loading from network when AwSettings.setBlockNetworkLoads is true

BUG=313463
NOTRY=true

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

[qinmin, 2013-10-31 16:41:26]

lgtm % nit

https://codereview.chromium.org/52463004/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/52463004/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:143:
if (mSettings != null)
nit: I thought java coding style needs {} for if statement

[mkosiba (inactive), 2013-10-31 17:05:14]

+CC: joth@

https://codereview.chromium.org/52463004/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/52463004/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:146:
return true;
why is blocking the default? Did WebViewClassic behave this way?

https://codereview.chromium.org/52463004/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:150:
final void installWebContentsObserver(ContentViewCore contentViewCore,
AwSettings settings) {
method doesn't do what it says anymore. Maybe add a
AwContentsClient.setAwSettings method?

My gut feeling is that this routing is a bit dirty and that we should move
AwContentViewClient to live in AwContents. I don't have any strong feelings
about this though.
+joth@ - WDYT?

[Ben Murdoch, 2013-10-31 17:26:55]

https://codereview.chromium.org/52463004/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/52463004/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:143:
if (mSettings != null)
On 2013/10/31 16:41:26, qinmin wrote:
> nit: I thought java coding style needs {} for if statement

nit: could make this one line

return mSettings == null ? true : mSettings.getBlockNetworkLoads();

[joth, 2013-11-01 19:37:33]

https://codereview.chromium.org/52463004/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/52463004/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:58:
private AwSettings mSettings;
probably cleaner to push this in as a member of the inner AwContentViewClient
class

https://codereview.chromium.org/52463004/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:146:
return true;
On 2013/10/31 17:05:14, mkosiba wrote:
> why is blocking the default? Did WebViewClassic behave this way?

I think the choice is arbitrary - it's not meaningful to have a
AwContentViewClient without the mSettings so we shouldn't even get this call.
(could assert / NPE in this case I guess?)

We could make this more explicit by only constructing AwContentViewClient when
the AwSettings is passed in (and, make AwSettings a member of
AwContentViewClient as mentioned above)
-or we pull AwContentViewClient right out to owned by the AwContents like you
propose below. AwWebContentsDelegateAdapter already went through that latter
process. Maybe that suggests the pattern.

Separate CL?

[michaelbai, 2013-11-05 22:35:33]

PTAL, Moved AwContentsViewClient to new file.

[joth, 2013-11-05 23:00:46]

lgtm

https://codereview.chromium.org/52463004/diff/110001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/AwContentsViewClient.java
(right):

https://codereview.chromium.org/52463004/diff/110001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContentsViewClient.java:21:
public class AwContentsViewClient extends ContentViewClient {
nit: no 's'

AwContentViewClient

(it is true it's confusing having both this and AwContentsClient, but at least
this will keep them self consistent)

[mkosiba (inactive), 2013-11-07 11:12:09]

Thanks for splitting the client out! LGTM

https://codereview.chromium.org/52463004/diff/220001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java
(right):

https://codereview.chromium.org/52463004/diff/220001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:95:
return mAwSettings != null ?
FYI boolean expressions are evaluated LTR so:

return mAwSettings == null || (mAwSettings.getBlockNetworkLoads() &&
URLUtil.isNetworkUrl(url));

should work fine too. Up to you which one you think is more readable.

[commit-bot: I haz the power, 2013-11-13 21:57:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/52463004/300001

[commit-bot: I haz the power, 2013-11-13 23:10:17]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[michaelbai, 2013-11-14 01:47:56]

The error is not related to Android, land it manually

[commit-bot: I haz the power, 2013-11-14 01:53:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/52463004/300001

[commit-bot: I haz the power, 2013-11-14 02:15:12]

Change committed as 235017