[WIP] WebRestrictions

WebRestrictions WIP merged

Not ready for review yet.

I have merged the webview hacks and common stuff. chrome parts need some more work.

[knn, 2015-11-05 10:58:17]

Description was changed from

==========
WebRestrictions WIP merged

Not ready for review yet
==========

to

==========
WebRestrictions WIP merged

Not ready for review yet.

I have merged the webview hacks and common stuff. chrome parts need some more
work.
==========

[aberent, 2015-11-16 12:39:07]

aberent@chromium.org changed reviewers:
+ aberent@chromium.org

[aberent, 2015-11-16 12:39:08]

Comments so far.

https://codereview.chromium.org/1423713015/diff/1/components/web_restriction/...
File components/web_restriction/content_resolver_web_restriction_provider.h
(right):

https://codereview.chromium.org/1423713015/diff/1/components/web_restriction/...
components/web_restriction/content_resolver_web_restriction_provider.h:55: const
base::Callback<void(bool)>& callback) const override;
Change name to AsyncShouldProceed?

https://codereview.chromium.org/1423713015/diff/1/components/web_restriction/...
File
components/web_restriction/java/src/org/chromium/components/web_restriction/ContentResolverWebRestrictionProvider.java
(right):

https://codereview.chromium.org/1423713015/diff/1/components/web_restriction/...
components/web_restriction/java/src/org/chromium/components/web_restriction/ContentResolverWebRestrictionProvider.java:58:
}.startQuery(0, null, mUri, null, url, null, null);
The startQuery documentation says that the 5 argument is 'selection' which
should be "A filter declaring which rows to return, formatted as an SQL WHERE
clause (excluding the WHERE itself). Passing null will return all rows for the
given URI.".

A url is not, I think, a valid SQL WHERE clause. Are we sure that this is passed
transparently to the content provider, and not checked by any version of Android
(including future versions) for validity?

Would passing something like "url = '<url>'" (where <url> is the url we are
trying to test) be safer?

[knn, 2016-01-20 19:40:00]

Patchset #2 (id:20001) has been deleted

[aberent, 2016-01-21 12:00:44]

https://codereview.chromium.org/1423713015/diff/40001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/1423713015/diff/40001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:271:
"com.google.android.apps.chrome.SupervisedUserProvider");
I think this should be read from a policy (app restriction), since we may want
to disable this, or (for enterprise usage) use something other than Chrome as
the source of this. Probably needs to be confirmed with the Unicorn team, since
we then have to make sure that the policy is set correctly for all WebView apps.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction.gypi (right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction.gypi:22: #TODO GN
Will be needed. Android bots are now using GN.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction.gypi:37: #TODO GN
Will be needed. Android bots are now using GN.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/content_resolver_web_restriction_provider.cc
(right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/content_resolver_web_restriction_provider.cc:60:
android::Java_ContentResolverWebRestrictionProvider_getErrorPage(
The error page isn't set until there is a response to ShouldProceed, so this
will always return NULL.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File
components/web_restriction/java/src/org/chromium/components/web_restriction/ContentResolverWebRestrictionProvider.java
(right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/java/src/org/chromium/components/web_restriction/ContentResolverWebRestrictionProvider.java:60:
}.startQuery(0, null, mQueryUri, null, url, null, null);
I think mQueryUri needs to be "url = <mQueryUri>"

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/java/src/org/chromium/components/web_restriction/ContentResolverWebRestrictionProvider.java:69:
// Don't care about completion notifications.
Maybe you do. Insert will return null if something goes wrong with sending the
request; do you want to tell the user that something has gone wrong?

[knn, 2016-01-21 12:17:22]

Thanks for the quick review.
I'll upload a new version after lunch.

https://codereview.chromium.org/1423713015/diff/40001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/1423713015/diff/40001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:271:
"com.google.android.apps.chrome.SupervisedUserProvider");
On 2016/01/21 12:00:43, aberent wrote:
> I think this should be read from a policy (app restriction), since we may want
> to disable this, or (for enterprise usage) use something other than Chrome as
> the source of this. Probably needs to be confirmed with the Unicorn team,
since
> we then have to make sure that the policy is set correctly for all WebView
apps.
> 

I was thinking maybe we could override it from policy to make the unicorn case
easier to support but what you said is definitely cleaner.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/content_resolver_web_restriction_provider.cc
(right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/content_resolver_web_restriction_provider.cc:60:
android::Java_ContentResolverWebRestrictionProvider_getErrorPage(
On 2016/01/21 12:00:43, aberent wrote:
> The error page isn't set until there is a response to ShouldProceed, so this
> will always return NULL.

My bad.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File
components/web_restriction/java/src/org/chromium/components/web_restriction/ContentResolverWebRestrictionProvider.java
(right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/java/src/org/chromium/components/web_restriction/ContentResolverWebRestrictionProvider.java:60:
}.startQuery(0, null, mQueryUri, null, url, null, null);
On 2016/01/21 12:00:43, aberent wrote:
> I think mQueryUri needs to be "url = <mQueryUri>"

You probably mean the |url| field = "url = 'blah'" right? Done.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/java/src/org/chromium/components/web_restriction/ContentResolverWebRestrictionProvider.java:69:
// Don't care about completion notifications.
On 2016/01/21 12:00:43, aberent wrote:
> Maybe you do. Insert will return null if something goes wrong with sending the
> request; do you want to tell the user that something has gone wrong?

Good point!

[Bernhard Bauer, 2016-01-22 17:32:31]

bauerb@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2016-01-22 17:32:32]

Nice! Can I see a demo once I'm back? Pleeeeaaase? 😃

https://codereview.chromium.org/1423713015/diff/40001/android_webview/android...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/1423713015/diff/40001/android_webview/android...
android_webview/android_webview.gyp:246:
'../components/components.gyp:web_restriction',
I think using the plural would be a bit more idiomatic here, ie.
"web_restrictions".

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/content_resolver_web_restriction_provider.cc
(right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/content_resolver_web_restriction_provider.cc:25: //
TODO Update cache
What cache?

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/content_resolver_web_restriction_provider.cc:65:
error_page_ = "";
.clear()

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/content_resolver_web_restriction_provider.h
(right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/content_resolver_web_restriction_provider.h:18:
namespace android {
Empty line after this one?

Also, add a comment for this class?

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/content_resolver_web_restriction_provider.h:34:
~SelfDeletingCallback();
Methods come before members in each section.

Also, DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/content_resolver_web_restriction_provider.h:78:
std::string error_page_;
|error_page_html_| maybe?

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File
components/web_restriction/java/src/org/chromium/components/web_restriction/ContentResolverWebRestrictionProvider.java
(right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/java/src/org/chromium/components/web_restriction/ContentResolverWebRestrictionProvider.java:5:
package org.chromium.components.web_restriction;
Don't use underscores in Java package names, i.e. use just |webrestriction|.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/java/src/org/chromium/components/web_restriction/ContentResolverWebRestrictionProvider.java:31:
mSupportsRequest = mContext.getContentResolver().getType(baseUri) != null;
That's clever! I would probably do this for the request URI though (so the
content resolver can return a type for the base URI, but not for the request
URI).

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/web_restriction_gin_wrapper.h (right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_gin_wrapper.h:20: static
gin::WrapperInfo kWrapperInfo;
What's this for?

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/web_restriction_provider.h (right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_provider.h:14: enum UrlAccess {
ALLOW, DISALLOW, UNKNOWN };
Put the values on individual lines.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_provider.h:18: class Observer {
You could consider using a callback instead of a single-method interface.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_provider.h:23: virtual
~WebRestrictionProvider(){};
No semicolon after a method body.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/web_restriction_resource_throttle.cc (right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_resource_throttle.cc:45: UrlAccess
access = provider_->ShouldProceed(is_main_frame_, url);
Hm… we call this method, and if it returns UNKNOWN we immediately call the
three-argument method. Why don't we just merge these into a single method? It
would return ALLOW or DISALLOW for an immediate decision, or UNKNOWN (or
possibly renamed to something else like PENDING) if it will call the callback
with the result. This is a pattern that is commonly used in net:: APIs.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_resource_throttle.cc:57: default:
Remove this; the compiler will check it for you.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/web_restriction_resource_throttle.h (right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_resource_throttle.h:14: 
Remove this line (or add one after line 15).

[knn, 2016-01-25 11:54:24]

Please continue reviewing the split cl in
https://codereview.chromium.org/1631603002

Hopefully, that piece will unblock you Anthony.

I have uploaded the entire cl here as well for comparison.

https://codereview.chromium.org/1423713015/diff/40001/android_webview/android...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/1423713015/diff/40001/android_webview/android...
android_webview/android_webview.gyp:246:
'../components/components.gyp:web_restriction',
On 2016/01/22 17:32:31, Bernhard Bauer wrote:
> I think using the plural would be a bit more idiomatic here, ie.
> "web_restrictions".

In that case we should rename the component as well right?
Is that what you are suggesting?

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction.gypi (right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction.gypi:22: #TODO GN
On 2016/01/21 12:00:43, aberent wrote:
> Will be needed. Android bots are now using GN.

Done.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction.gypi:37: #TODO GN
On 2016/01/21 12:00:43, aberent wrote:
> Will be needed. Android bots are now using GN.

Done.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/content_resolver_web_restriction_provider.cc
(right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/content_resolver_web_restriction_provider.cc:25: //
TODO Update cache
On 2016/01/22 17:32:31, Bernhard Bauer wrote:
> What cache?

Cache of known statuses.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/content_resolver_web_restriction_provider.cc:65:
error_page_ = "";
On 2016/01/22 17:32:31, Bernhard Bauer wrote:
> .clear()

Done.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/content_resolver_web_restriction_provider.h
(right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/content_resolver_web_restriction_provider.h:18:
namespace android {
On 2016/01/22 17:32:31, Bernhard Bauer wrote:
> Empty line after this one?
> 
> Also, add a comment for this class?

Done.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/content_resolver_web_restriction_provider.h:34:
~SelfDeletingCallback();
On 2016/01/22 17:32:31, Bernhard Bauer wrote:
> Methods come before members in each section.
> 
> Also, DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/content_resolver_web_restriction_provider.h:78:
std::string error_page_;
On 2016/01/22 17:32:31, Bernhard Bauer wrote:
> |error_page_html_| maybe?

Removed this to support error page per url.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File
components/web_restriction/java/src/org/chromium/components/web_restriction/ContentResolverWebRestrictionProvider.java
(right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/java/src/org/chromium/components/web_restriction/ContentResolverWebRestrictionProvider.java:5:
package org.chromium.components.web_restriction;
On 2016/01/22 17:32:31, Bernhard Bauer wrote:
> Don't use underscores in Java package names, i.e. use just |webrestriction|.

Done.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/java/src/org/chromium/components/web_restriction/ContentResolverWebRestrictionProvider.java:31:
mSupportsRequest = mContext.getContentResolver().getType(baseUri) != null;
On 2016/01/22 17:32:31, Bernhard Bauer wrote:
> That's clever! I would probably do this for the request URI though (so the
> content resolver can return a type for the base URI, but not for the request
> URI).

Done.
TODO: We will need to update the Content Provider as well.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/web_restriction_gin_wrapper.h (right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_gin_wrapper.h:20: static
gin::WrapperInfo kWrapperInfo;
On 2016/01/22 17:32:32, Bernhard Bauer wrote:
> What's this for?

Distinguishes broadly the different embedders of gin: {Blink, Chrome, Pdf
viewer}

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/web_restriction_provider.h (right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_provider.h:14: enum UrlAccess {
ALLOW, DISALLOW, UNKNOWN };
On 2016/01/22 17:32:32, Bernhard Bauer wrote:
> Put the values on individual lines.

Done.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_provider.h:18: class Observer {
On 2016/01/22 17:32:32, Bernhard Bauer wrote:
> You could consider using a callback instead of a single-method interface.

Done.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_provider.h:23: virtual
~WebRestrictionProvider(){};
On 2016/01/22 17:32:32, Bernhard Bauer wrote:
> No semicolon after a method body.

Done.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/web_restriction_resource_throttle.cc (right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_resource_throttle.cc:45: UrlAccess
access = provider_->ShouldProceed(is_main_frame_, url);
This looks much better now! Thanks!

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_resource_throttle.cc:57: default:
On 2016/01/22 17:32:32, Bernhard Bauer wrote:
> Remove this; the compiler will check it for you.

Done.

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/web_restriction_resource_throttle.h (right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_resource_throttle.h:14: 
On 2016/01/22 17:32:32, Bernhard Bauer wrote:
> Remove this line (or add one after line 15).

Done.

[Bernhard Bauer, 2016-01-26 09:52:41]

https://codereview.chromium.org/1423713015/diff/40001/android_webview/android...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/1423713015/diff/40001/android_webview/android...
android_webview/android_webview.gyp:246:
'../components/components.gyp:web_restriction',
On 2016/01/25 11:54:24, knn wrote:
> On 2016/01/22 17:32:31, Bernhard Bauer wrote:
> > I think using the plural would be a bit more idiomatic here, ie.
> > "web_restrictions".
> 
> In that case we should rename the component as well right?
> Is that what you are suggesting?

Yes, I would call the component "web_restrictions", and use that everywhere else
as well (except for Java package names, which shouldn't have underscores, so it
would be just "webrestrictions" there. I hope that's not too much additional
churn?

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/web_restriction_gin_wrapper.h (right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_gin_wrapper.h:20: static
gin::WrapperInfo kWrapperInfo;
On 2016/01/25 11:54:24, knn wrote:
> On 2016/01/22 17:32:32, Bernhard Bauer wrote:
> > What's this for?
> 
> Distinguishes broadly the different embedders of gin: {Blink, Chrome, Pdf
> viewer}

Sorry, should have been more specific: This doesn't seem to be referenced
anywhere, and it can't run a static initializer either because those are
disallowed, so I don't see what effect it would have.

[knn, 2016-01-26 14:42:30]

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/web_restriction_gin_wrapper.h (right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_gin_wrapper.h:20: static
gin::WrapperInfo kWrapperInfo;
On 2016/01/26 09:52:41, Bernhard Bauer wrote:
> On 2016/01/25 11:54:24, knn wrote:
> > On 2016/01/22 17:32:32, Bernhard Bauer wrote:
> > > What's this for?
> > 
> > Distinguishes broadly the different embedders of gin: {Blink, Chrome, Pdf
> > viewer}
> 
> Sorry, should have been more specific: This doesn't seem to be referenced
> anywhere, and it can't run a static initializer either because those are
> disallowed, so I don't see what effect it would have.

Perhaps this answers your question,
https://code.google.com/p/chromium/codesearch#chromium/src/gin/wrappable.h&l=50
Let me know if I misunderstood.

[Bernhard Bauer, 2016-01-26 15:02:41]

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
File components/web_restriction/web_restriction_gin_wrapper.h (right):

https://codereview.chromium.org/1423713015/diff/40001/components/web_restrict...
components/web_restriction/web_restriction_gin_wrapper.h:20: static
gin::WrapperInfo kWrapperInfo;
On 2016/01/26 14:42:29, knn wrote:
> On 2016/01/26 09:52:41, Bernhard Bauer wrote:
> > On 2016/01/25 11:54:24, knn wrote:
> > > On 2016/01/22 17:32:32, Bernhard Bauer wrote:
> > > > What's this for?
> > > 
> > > Distinguishes broadly the different embedders of gin: {Blink, Chrome, Pdf
> > > viewer}
> > 
> > Sorry, should have been more specific: This doesn't seem to be referenced
> > anywhere, and it can't run a static initializer either because those are
> > disallowed, so I don't see what effect it would have.
> 
> Perhaps this answers your question,
>
https://code.google.com/p/chromium/codesearch#chromium/src/gin/wrappable.h&l=50
> Let me know if I misunderstood.

Ah, that makes sense. Thanks!

[jochen (gone - plz use gerrit), 2016-02-22 14:24:42]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-02-22 14:24:44]

https://codereview.chromium.org/1423713015/diff/60001/android_webview/rendere...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/1423713015/diff/60001/android_webview/rendere...
android_webview/renderer/aw_content_renderer_client.cc:221:
web_restriction::WebRestrictionGinWrapper::Install(render_frame);
you install the gin wrapper before you actually navigate to the error string,
no?

The error_page component only injects its gin wrappers once the render frame
finished loading the error html

[aberent, 2016-04-15 18:27:32]

Replaced by https://codereview.chromium.org/1890203002/