Web restrictions component.

Web restrictions component.

The Web restrictions component provides a resource throttle that allows Chrome on Android and Webview to restrict URL access using a content provider provided by a different app.
It also allows that content provider to return custom error pages when requests fail, that can then be used by the user to request access to those URLs.

This is intended for use, in particular:
- For supervised users of Webview, where Chrome provides the content provider.
- For enterprise environments, where the content provider will be provided by a third party app.

The individual pieces are:
A content resolver based web restriction provider.
A gin wrapper and a resource throttle to tie it together.

Preliminary code making use of this is in https://codereview.chromium.org/1423713015.

This CL replaces https://codereview.chromium.org/1631603002/

BUG=569879
Committed: https://crrev.com/d6457207130f1a5abb0d36a27fbf9f4cb31f131f
Cr-Commit-Position: refs/heads/master@{#378601}

[aberent, 2016-02-10 15:56:03]

This replaces https://codereview.chromium.org/1631603002/ with a change of
author. The first patchset is identical to
https://codereview.chromium.org/1631603002/#ps240001 (patch set 12 on that CL).

The remaining issues will be fixed in later patch sets.

[aberent, 2016-02-10 16:00:42]

This replaces https://codereview.chromium.org/1631603002/ with a change of
author. The first patchset is identical to
https://codereview.chromium.org/1631603002/#ps240001 (patch set 12 on that CL).

The remaining issues will be fixed in later patch sets.

[aberent, 2016-02-19 12:27:51]

Description was changed from

==========
Web restrictions component only.

Splitting away non webview specific pieces from
https://codereview.chromium.org/1423713015
so that Anthony can work parallely on the chrome bits while the webview bits are
reviewed.

The individual pieces are:
A content resolver based web restriction provider.
A gin wrapper and a resource throttle to tie it together.

BUG=569879
==========

to

==========
Web restrictions component.

The Web restrictions component provides a resource throttle that allows Chrome
on Android and Webview to restrict URL access using a content provider provided
by a different app.
It also allows that content provider to return custom error pages when requests
fail, that can then be used by the user to request access to those URLs.

This is intended for use, in particular:
- For supervised users of Webview, where Chrome provides the content provider.
- For enterprise environments, where the content provider will be provided by a
third party app.

The individual pieces are:
A content resolver based web restriction provider.
A gin wrapper and a resource throttle to tie it together.

BUG=569879
==========

[aberent, 2016-02-19 12:30:14]

Description was changed from

==========
Web restrictions component.

The Web restrictions component provides a resource throttle that allows Chrome
on Android and Webview to restrict URL access using a content provider provided
by a different app.
It also allows that content provider to return custom error pages when requests
fail, that can then be used by the user to request access to those URLs.

This is intended for use, in particular:
- For supervised users of Webview, where Chrome provides the content provider.
- For enterprise environments, where the content provider will be provided by a
third party app.

The individual pieces are:
A content resolver based web restriction provider.
A gin wrapper and a resource throttle to tie it together.

BUG=569879
==========

to

==========
Web restrictions component.

The Web restrictions component provides a resource throttle that allows Chrome
on Android and Webview to restrict URL access using a content provider provided
by a different app.
It also allows that content provider to return custom error pages when requests
fail, that can then be used by the user to request access to those URLs.

This is intended for use, in particular:
- For supervised users of Webview, where Chrome provides the content provider.
- For enterprise environments, where the content provider will be provided by a
third party app.

The individual pieces are:
A content resolver based web restriction provider.
A gin wrapper and a resource throttle to tie it together.

Preliminary code making use of this is in
https://codereview.chromium.org/1423713015.

This CL replaces https://codereview.chromium.org/1631603002/

BUG=569879
==========

[aberent, 2016-02-19 12:30:52]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-19 12:33:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1684153002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1684153002/80001

[aberent, 2016-02-19 12:37:50]

aberent@chromium.org changed reviewers:
+ bauerb@chromium.org, jochen@chromium.org

[aberent, 2016-02-19 12:37:51]

jochen@ - Please review files in //components top level directory
bauerb@ - Please review //components/web_restrictions

[Bernhard Bauer, 2016-02-19 13:46:26]

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File components/web_restrictions/browser/web_restrictions_client.cc (right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client.cc:207:
WebRestrictionsClient::ShouldProceedTask(
Does this need to be a private static method, or could it also go into the
anonymous namespace above?

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client.cc:220: if
(j_error_page.obj() == nullptr) {
You can use .is_null() for this.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client.cc:221:
result.error_page = nullptr;
And this isn't really necessary, as the string will start out empty anyway.

(Also, why nullptr?)

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client.cc:223:
result.error_page =
And finally, here you could just pass a pointer to the string as an outparam.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File components/web_restrictions/browser/web_restrictions_client_unittest.cc
(right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client_unittest.cc:15: *
built with the real version of the Java WebRestrictionsClient.
This seems very fragile. How do you know that the real WebRestrictionsClient
isn't going to end up on the class path?

Would it work instead to add a hook to WebRestrictionsClient.create() that
returns a mock subclass?

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client_unittest.cc:22: bool
callback_called;
This one isn't really necessary -- the run loop will run until its quit closure
is called, so if you are after a Run() call, you know that has happened.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client_unittest.cc:23: bool
returned_result;
|g_returned_result|?

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client_unittest.cc:25: void
ResultCallback(base::Closure quit_closure, bool result) {
Pass the closure by const reference.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client_unittest.cc:30: } 
// namespace
Nit: Empty line before this one.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc
(right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc:21:
bool resume_called_;
Add accessor for these (at least the ones you need).

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc:26:
TestResourceController(const base::Closure quit_closure)
Pass by const ref.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc:52:
content::TestBrowserThreadBundle thread_bundle;
It might be worth moving these into a test fixture.

[jochen (gone - plz use gerrit), 2016-02-19 14:51:21]

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File components/web_restrictions/browser/DEPS (right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/DEPS:2: "+content/public",
content/public/browser please

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File components/web_restrictions/renderer/DEPS (right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/renderer/DEPS:2: "+content/public",
/renderer

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File components/web_restrictions/renderer/web_restrictions_gin_wrapper.cc
(right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/renderer/web_restrictions_gin_wrapper.cc:32:
global->Set(gin::StringToV8(isolate, "webRestriction"), controller.ToV8());
this is put into the global namespace, why is this not implemented in Blink?
Also, where's the intent to implement & ship thread on blink-dev?

[Bernhard Bauer, 2016-02-19 15:13:31]

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File components/web_restrictions/renderer/web_restrictions_gin_wrapper.cc
(right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/renderer/web_restrictions_gin_wrapper.cc:32:
global->Set(gin::StringToV8(isolate, "webRestriction"), controller.ToV8());
On 2016/02/19 14:51:21, jochen wrote:
> this is put into the global namespace, why is this not implemented in Blink?
> Also, where's the intent to implement & ship thread on blink-dev?

This is not a publicly available web platform API -- it's only available to a
custom error page, equivalent to the callback on an interstitial page (except
not implemented in an interstitial, because those are not supported in WebView).

[aberent, 2016-02-19 19:40:06]

aberent@chromium.org changed reviewers:
+ brettw@chromium.org, cbentzel@chromium.org

[aberent, 2016-02-19 19:40:09]

cbentzel@ - Please review new dependency on '//net'
brettw@ - Please review new dependency on '//url'

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File components/web_restrictions/browser/DEPS (right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/DEPS:2: "+content/public",
On 2016/02/19 14:51:21, jochen wrote:
> content/public/browser please

Done.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File components/web_restrictions/browser/web_restrictions_client.cc (right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client.cc:207:
WebRestrictionsClient::ShouldProceedTask(
On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> Does this need to be a private static method, or could it also go into the
> anonymous namespace above?

Sadly it can't unless I make ShouldProceedResult public.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client.cc:220: if
(j_error_page.obj() == nullptr) {
On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> You can use .is_null() for this.

Done.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client.cc:221:
result.error_page = nullptr;
On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> And this isn't really necessary, as the string will start out empty anyway.
> 
> (Also, why nullptr?)

Done.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client.cc:223:
result.error_page =
On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> And finally, here you could just pass a pointer to the string as an outparam.

This has to be passed to the OnShouldProceedComplete. Splitting it into a
seperate parameter so breaks the model of PostTaskAndReplyWithResult() which
expects a single object to be passed from the task to the reply function. I
could, of course, use PostTaskAndReply(), but would then need my own handling of
the lifetime of the result objects.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File components/web_restrictions/browser/web_restrictions_client_unittest.cc
(right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client_unittest.cc:15: *
built with the real version of the Java WebRestrictionsClient.
On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> This seems very fragile. How do you know that the real WebRestrictionsClient
> isn't going to end up on the class path?
> 
> Would it work instead to add a hook to WebRestrictionsClient.create() that
> returns a mock subclass?

Done.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client_unittest.cc:22: bool
callback_called;
On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> This one isn't really necessary -- the run loop will run until its quit
closure
> is called, so if you are after a Run() call, you know that has happened.

Done.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client_unittest.cc:23: bool
returned_result;
On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> |g_returned_result|?

Done.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client_unittest.cc:25: void
ResultCallback(base::Closure quit_closure, bool result) {
On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> Pass the closure by const reference.

Done.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client_unittest.cc:30: } 
// namespace
On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> Nit: Empty line before this one.

Done.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc
(right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc:21:
bool resume_called_;
On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> Add accessor for these (at least the ones you need).

Done.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc:26:
TestResourceController(const base::Closure quit_closure)
On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> Pass by const ref.

Done.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc:52:
content::TestBrowserThreadBundle thread_bundle;
On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> It might be worth moving these into a test fixture.

Done.

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File components/web_restrictions/renderer/DEPS (right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/renderer/DEPS:2: "+content/public",
On 2016/02/19 14:51:21, jochen wrote:
> /renderer

Done.

[aberent, 2016-02-19 19:40:43]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-19 19:43:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1684153002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1684153002/120001

[commit-bot: I haz the power, 2016-02-19 19:55:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-19 19:55:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aberent, 2016-02-22 11:45:30]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-22 11:45:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1684153002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1684153002/140001

[commit-bot: I haz the power, 2016-02-22 11:54:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-22 11:54:21]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[Bernhard Bauer, 2016-02-22 12:39:17]

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File components/web_restrictions/browser/web_restrictions_client.cc (right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client.cc:223:
result.error_page =
On 2016/02/19 19:40:08, aberent wrote:
> On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> > And finally, here you could just pass a pointer to the string as an
outparam.
> 
> This has to be passed to the OnShouldProceedComplete. Splitting it into a
> seperate parameter so breaks the model of PostTaskAndReplyWithResult() which
> expects a single object to be passed from the task to the reply function. I
> could, of course, use PostTaskAndReply(), but would then need my own handling
of
> the lifetime of the result objects.

Sorry, I meant using the outparam form of ConvertJavaStringToUTF8 to set the
error page.

https://codereview.chromium.org/1684153002/diff/140001/components/components_...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1684153002/diff/140001/components/components_...
components/components_tests.gyp:1685: 'target_name':
'components_java_test_stubs_jni_headers',
Nit: Usually these are called "test_support" and prefixed with the name of the
component, so it would be "web_restrictions_test_support_jni_headers" (I know,
quite a mouthful...)

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
File
components/web_restrictions/browser/javatest/src/org/chromium/components/webrestrictions/MockWebRestrictionsClient.java
(right):

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/browser/javatest/src/org/chromium/components/webrestrictions/MockWebRestrictionsClient.java:29:
MockWebRestrictionsClient() {}
Make this private?

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/browser/javatest/src/org/chromium/components/webrestrictions/MockWebRestrictionsClient.java:32:
void init(String authority, final long nativeProvider) {
final isn't necessary.

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/browser/javatest/src/org/chromium/components/webrestrictions/MockWebRestrictionsClient.java:39:
private static WebRestrictionsClient create(String authority, long
nativeProvider) {
I don't think this is still called.

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
File components/web_restrictions/browser/mock_web_restrictions_client.h (right):

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/browser/mock_web_restrictions_client.h:18:
RegisterNativesImpl(env_);
This would register the JNI methods every time this object is created, no? That
seems wrong. I think we need to export this as a static method and then call it
from ComponentsTestSuite::Initialize
(https://code.google.com/p/chromium/codesearch#chromium/src/components/test/ru...).

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/browser/mock_web_restrictions_client.h:27: JNIEnv*
env_;
It's bad form to hold on to a JNIEnv object. Just call AttachCurrentThread() in
the destructor.

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
File components/web_restrictions/browser/web_restrictions_client_unittest.cc
(right):

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client_unittest.cc:31:
WebRestrictionsClient client;
You could also put these into a test fixture.

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
File
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc
(right):

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc:125:
// Using a different URL should be deferred
You could put each of these cases into a separate scope, then you don't need the
"2" suffix for one-shot objects like RunLoop.

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

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
File components/web_restrictions/renderer/web_restrictions_gin_wrapper.cc
(right):

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/renderer/web_restrictions_gin_wrapper.cc:42:
render_frame()->GetWebFrame()->reload();
render_frame() might be nullptr

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
File components/web_restrictions/renderer/web_restrictions_gin_wrapper.h
(right):

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/renderer/web_restrictions_gin_wrapper.h:20: public
content::RenderFrameObserver {
you must override OnDestruct - lifetime of this class is managed by v8

[aberent, 2016-02-23 13:52:37]

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File components/web_restrictions/browser/web_restrictions_client.cc (right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client.cc:223:
result.error_page =
On 2016/02/22 12:39:16, Bernhard Bauer wrote:
> On 2016/02/19 19:40:08, aberent wrote:
> > On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> > > And finally, here you could just pass a pointer to the string as an
> outparam.
> > 
> > This has to be passed to the OnShouldProceedComplete. Splitting it into a
> > seperate parameter so breaks the model of PostTaskAndReplyWithResult() which
> > expects a single object to be passed from the task to the reply function. I
> > could, of course, use PostTaskAndReply(), but would then need my own
handling
> of
> > the lifetime of the result objects.
> 
> Sorry, I meant using the outparam form of ConvertJavaStringToUTF8 to set the
> error page.

Done.

https://codereview.chromium.org/1684153002/diff/140001/components/components_...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1684153002/diff/140001/components/components_...
components/components_tests.gyp:1685: 'target_name':
'components_java_test_stubs_jni_headers',
On 2016/02/22 12:39:16, Bernhard Bauer wrote:
> Nit: Usually these are called "test_support" and prefixed with the name of the
> component, so it would be "web_restrictions_test_support_jni_headers" (I know,
> quite a mouthful...)

Done. Also, for this to make sense, moved to web_restrictions.gypi, and made
similar name changes and moved components_java_test_stubs.

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
File
components/web_restrictions/browser/javatest/src/org/chromium/components/webrestrictions/MockWebRestrictionsClient.java
(right):

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/browser/javatest/src/org/chromium/components/webrestrictions/MockWebRestrictionsClient.java:29:
MockWebRestrictionsClient() {}
On 2016/02/22 12:39:16, Bernhard Bauer wrote:
> Make this private?

Done.

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/browser/javatest/src/org/chromium/components/webrestrictions/MockWebRestrictionsClient.java:32:
void init(String authority, final long nativeProvider) {
On 2016/02/22 12:39:16, Bernhard Bauer wrote:
> final isn't necessary.

Done.

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/browser/javatest/src/org/chromium/components/webrestrictions/MockWebRestrictionsClient.java:39:
private static WebRestrictionsClient create(String authority, long
nativeProvider) {
On 2016/02/22 12:39:16, Bernhard Bauer wrote:
> I don't think this is still called.

Done.

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
File components/web_restrictions/browser/mock_web_restrictions_client.h (right):

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/browser/mock_web_restrictions_client.h:18:
RegisterNativesImpl(env_);
On 2016/02/22 12:39:17, Bernhard Bauer wrote:
> This would register the JNI methods every time this object is created, no?
That
> seems wrong. I think we need to export this as a static method and then call
it
> from ComponentsTestSuite::Initialize
>
(https://code.google.com/p/chromium/codesearch#chromium/src/components/test/ru...).

Done.

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/browser/mock_web_restrictions_client.h:27: JNIEnv*
env_;
On 2016/02/22 12:39:16, Bernhard Bauer wrote:
> It's bad form to hold on to a JNIEnv object. Just call AttachCurrentThread()
in
> the destructor.

Done.

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
File components/web_restrictions/browser/web_restrictions_client_unittest.cc
(right):

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client_unittest.cc:31:
WebRestrictionsClient client;
On 2016/02/22 12:39:17, Bernhard Bauer wrote:
> You could also put these into a test fixture.

Done.

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
File
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc
(right):

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc:125:
// Using a different URL should be deferred
On 2016/02/22 12:39:17, Bernhard Bauer wrote:
> You could put each of these cases into a separate scope, then you don't need
the
> "2" suffix for one-shot objects like RunLoop.

Refactored to have a separate test for each case.

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
File components/web_restrictions/renderer/web_restrictions_gin_wrapper.cc
(right):

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/renderer/web_restrictions_gin_wrapper.cc:42:
render_frame()->GetWebFrame()->reload();
On 2016/02/22 14:22:43, jochen wrote:
> render_frame() might be nullptr

Done.

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
File components/web_restrictions/renderer/web_restrictions_gin_wrapper.h
(right):

https://codereview.chromium.org/1684153002/diff/140001/components/web_restric...
components/web_restrictions/renderer/web_restrictions_gin_wrapper.h:20: public
content::RenderFrameObserver {
On 2016/02/22 14:22:44, jochen wrote:
> you must override OnDestruct - lifetime of this class is managed by v8

Done.

[Bernhard Bauer, 2016-02-23 14:29:13]

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File components/web_restrictions/browser/web_restrictions_client.cc (right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client.cc:223:
result.error_page =
On 2016/02/23 13:52:37, aberent wrote:
> On 2016/02/22 12:39:16, Bernhard Bauer wrote:
> > On 2016/02/19 19:40:08, aberent wrote:
> > > On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> > > > And finally, here you could just pass a pointer to the string as an
> > outparam.
> > > 
> > > This has to be passed to the OnShouldProceedComplete. Splitting it into a
> > > seperate parameter so breaks the model of PostTaskAndReplyWithResult()
which
> > > expects a single object to be passed from the task to the reply function.
I
> > > could, of course, use PostTaskAndReply(), but would then need my own
> handling
> > of
> > > the lifetime of the result objects.
> > 
> > Sorry, I meant using the outparam form of ConvertJavaStringToUTF8 to set the
> > error page.
> 
> Done.

Not really? 😃

https://codereview.chromium.org/1684153002/diff/180001/components/test/run_al...
File components/test/run_all_unittests.cc (right):

https://codereview.chromium.org/1684153002/diff/180001/components/test/run_al...
components/test/run_all_unittests.cc:72:
ASSERT_TRUE(web_restrictions::MockWebRestrictionsClient::Register(env));
Bonus points for sorting these alphabetically.

https://codereview.chromium.org/1684153002/diff/180001/components/web_restric...
File components/web_restrictions.gypi (right):

https://codereview.chromium.org/1684153002/diff/180001/components/web_restric...
components/web_restrictions.gypi:75: 'target_name':
'web_restrictions_java_test_support',
I think this should be web_restrictions_test_support_java (the Java parts of
web_restrictions_test_support, which we don't have because
MockWebRestrictionsClient is directly part of the unit test source set).

https://codereview.chromium.org/1684153002/diff/180001/components/web_restric...
File components/web_restrictions/browser/mock_web_restrictions_client.h (right):

https://codereview.chromium.org/1684153002/diff/180001/components/web_restric...
components/web_restrictions/browser/mock_web_restrictions_client.h:16:
MockWebRestrictionsClient() {
Usually, these method bodies should be out of line. I think you'll need to add
an implementation file for this anyway, because the generated JNI header
contains method stubs, so if someone were to include it (via this header) in
multiple compilation units, it would violate the One Definition Rule.

https://codereview.chromium.org/1684153002/diff/180001/components/web_restric...
File
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc
(right):

https://codereview.chromium.org/1684153002/diff/180001/components/web_restric...
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc:57:
// Mock the Java WebRestrictionsClient. The real version
Move members after methods please.

[jochen (gone - plz use gerrit), 2016-02-23 14:38:01]

lgtm

[aberent, 2016-02-23 16:26:35]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[aberent, 2016-02-23 16:26:51]

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
File components/web_restrictions/browser/web_restrictions_client.cc (right):

https://codereview.chromium.org/1684153002/diff/100001/components/web_restric...
components/web_restrictions/browser/web_restrictions_client.cc:223:
result.error_page =
On 2016/02/23 14:29:13, Bernhard Bauer wrote:
> On 2016/02/23 13:52:37, aberent wrote:
> > On 2016/02/22 12:39:16, Bernhard Bauer wrote:
> > > On 2016/02/19 19:40:08, aberent wrote:
> > > > On 2016/02/19 13:46:26, Bernhard Bauer wrote:
> > > > > And finally, here you could just pass a pointer to the string as an
> > > outparam.
> > > > 
> > > > This has to be passed to the OnShouldProceedComplete. Splitting it into
a
> > > > seperate parameter so breaks the model of PostTaskAndReplyWithResult()
> which
> > > > expects a single object to be passed from the task to the reply
function.
> I
> > > > could, of course, use PostTaskAndReply(), but would then need my own
> > handling
> > > of
> > > > the lifetime of the result objects.
> > > 
> > > Sorry, I meant using the outparam form of ConvertJavaStringToUTF8 to set
the
> > > error page.
> > 
> > Done.
> 
> Not really? 😃

Done. Really, this time.

https://codereview.chromium.org/1684153002/diff/180001/components/test/run_al...
File components/test/run_all_unittests.cc (right):

https://codereview.chromium.org/1684153002/diff/180001/components/test/run_al...
components/test/run_all_unittests.cc:72:
ASSERT_TRUE(web_restrictions::MockWebRestrictionsClient::Register(env));
On 2016/02/23 14:29:13, Bernhard Bauer wrote:
> Bonus points for sorting these alphabetically.

Done.

https://codereview.chromium.org/1684153002/diff/180001/components/web_restric...
File components/web_restrictions.gypi (right):

https://codereview.chromium.org/1684153002/diff/180001/components/web_restric...
components/web_restrictions.gypi:75: 'target_name':
'web_restrictions_java_test_support',
On 2016/02/23 14:29:13, Bernhard Bauer wrote:
> I think this should be web_restrictions_test_support_java (the Java parts of
> web_restrictions_test_support, which we don't have because
> MockWebRestrictionsClient is directly part of the unit test source set).

Done.

https://codereview.chromium.org/1684153002/diff/180001/components/web_restric...
File components/web_restrictions/browser/mock_web_restrictions_client.h (right):

https://codereview.chromium.org/1684153002/diff/180001/components/web_restric...
components/web_restrictions/browser/mock_web_restrictions_client.h:16:
MockWebRestrictionsClient() {
On 2016/02/23 14:29:13, Bernhard Bauer wrote:
> Usually, these method bodies should be out of line. I think you'll need to add
> an implementation file for this anyway, because the generated JNI header
> contains method stubs, so if someone were to include it (via this header) in
> multiple compilation units, it would violate the One Definition Rule.

Done.

https://codereview.chromium.org/1684153002/diff/180001/components/web_restric...
File
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc
(right):

https://codereview.chromium.org/1684153002/diff/180001/components/web_restric...
components/web_restrictions/browser/web_restrictions_resource_throttle_unittest.cc:57:
// Mock the Java WebRestrictionsClient. The real version
On 2016/02/23 14:29:13, Bernhard Bauer wrote:
> Move members after methods please.

Done.

[commit-bot: I haz the power, 2016-02-23 16:27:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1684153002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1684153002/200001

[commit-bot: I haz the power, 2016-02-23 16:34:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-23 16:34:44]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[aberent, 2016-02-23 17:11:18]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-23 17:15:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1684153002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1684153002/220001

[commit-bot: I haz the power, 2016-02-23 17:38:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-23 17:38:29]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Bernhard Bauer, 2016-02-23 18:09:12]

LGTM, thanks!

https://codereview.chromium.org/1684153002/diff/220001/components/web_restric...
File components/web_restrictions/BUILD.gn (right):

https://codereview.chromium.org/1684153002/diff/220001/components/web_restric...
components/web_restrictions/BUILD.gn:88: static_library("test_support") {
Should this be a source_set?

[brettw, 2016-02-23 19:12:26]

https://codereview.chromium.org/1684153002/diff/220001/components/web_restric...
File components/web_restrictions/BUILD.gn (right):

https://codereview.chromium.org/1684153002/diff/220001/components/web_restric...
components/web_restrictions/BUILD.gn:88: static_library("test_support") {
Yeah, we should make all static_libraries source sets unless there's a reason
not to. It will compile slightly faster and has fewer surprising behaviors.

[cbentzel, 2016-02-24 04:03:02]

Where is the error page HTML being used? That's the portion which concerns me
the most but I don't actually see it being used by the throttle or other
locations.

[And I know the main reason I'm here is the //net DEPS but thought I'd throw
that in]

[aberent, 2016-02-24 10:55:52]

On 2016/02/24 04:03:02, cbentzel (Out til Feb22) wrote:
> Where is the error page HTML being used? That's the portion which concerns me
> the most but I don't actually see it being used by the throttle or other
> locations.
> 
> [And I know the main reason I'm here is the //net DEPS but thought I'd throw
> that in]

It is used in the other (draft CL) for WebView in
https://codereview.chromium.org/1423713015/diff/60001/android_webview/rendere....
I have still to write the code to use it in Chrome.

[aberent, 2016-02-24 10:59:40]

https://codereview.chromium.org/1684153002/diff/220001/components/web_restric...
File components/web_restrictions/BUILD.gn (right):

https://codereview.chromium.org/1684153002/diff/220001/components/web_restric...
components/web_restrictions/BUILD.gn:88: static_library("test_support") {
On 2016/02/23 18:09:12, Bernhard Bauer wrote:
> Should this be a source_set?

Done.

https://codereview.chromium.org/1684153002/diff/220001/components/web_restric...
components/web_restrictions/BUILD.gn:88: static_library("test_support") {
On 2016/02/23 19:12:26, brettw wrote:
> Yeah, we should make all static_libraries source sets unless there's a reason
> not to. It will compile slightly faster and has fewer surprising behaviors.

Done.

[aberent, 2016-02-24 11:00:29]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-24 11:03:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1684153002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1684153002/240001

[commit-bot: I haz the power, 2016-02-24 11:16:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-24 11:16:07]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[cbentzel, 2016-02-24 14:20:36]

On 2016/02/24 10:55:52, aberent wrote:
> On 2016/02/24 04:03:02, cbentzel (Out til Feb22) wrote:
> > Where is the error page HTML being used? That's the portion which concerns
me
> > the most but I don't actually see it being used by the throttle or other
> > locations.
> > 
> > [And I know the main reason I'm here is the //net DEPS but thought I'd throw
> > that in]
> 
> It is used in the other (draft CL) for WebView in
>
https://codereview.chromium.org/1423713015/diff/60001/android_webview/rendere....
> I have still to write the code to use it in Chrome.

OK. Error page interstitiatls are full of edge cases, so please make sure to
loop in mmenke when it comes to using it. There's definite security implications
as well. I'll review this assuming that it won't be a problem.

[cbentzel, 2016-03-01 17:32:09]

net/  DEPS usage LGTM (sorry about the gap, forgot about this!)

[Bernhard Bauer, 2016-03-01 17:33:19]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2016-03-01 17:33:19]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1684153002/#ps240001
(title: "Use source_set for test_support")

[commit-bot: I haz the power, 2016-03-01 17:34:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1684153002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1684153002/240001

[commit-bot: I haz the power, 2016-03-01 17:46:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-01 17:46:20]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bernhard Bauer, 2016-03-01 19:29:35]

Brett, friendly ping on DEPS review for url/?

[brettw, 2016-03-01 22:35:21]

deps lgtm

[Bernhard Bauer, 2016-03-01 22:58:59]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2016-03-01 22:59:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1684153002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1684153002/240001

[commit-bot: I haz the power, 2016-03-01 23:34:55]

Description was changed from

==========
Web restrictions component.

The Web restrictions component provides a resource throttle that allows Chrome
on Android and Webview to restrict URL access using a content provider provided
by a different app.
It also allows that content provider to return custom error pages when requests
fail, that can then be used by the user to request access to those URLs.

This is intended for use, in particular:
- For supervised users of Webview, where Chrome provides the content provider.
- For enterprise environments, where the content provider will be provided by a
third party app.

The individual pieces are:
A content resolver based web restriction provider.
A gin wrapper and a resource throttle to tie it together.

Preliminary code making use of this is in
https://codereview.chromium.org/1423713015.

This CL replaces https://codereview.chromium.org/1631603002/

BUG=569879
==========

to

==========
Web restrictions component.

The Web restrictions component provides a resource throttle that allows Chrome
on Android and Webview to restrict URL access using a content provider provided
by a different app.
It also allows that content provider to return custom error pages when requests
fail, that can then be used by the user to request access to those URLs.

This is intended for use, in particular:
- For supervised users of Webview, where Chrome provides the content provider.
- For enterprise environments, where the content provider will be provided by a
third party app.

The individual pieces are:
A content resolver based web restriction provider.
A gin wrapper and a resource throttle to tie it together.

Preliminary code making use of this is in
https://codereview.chromium.org/1423713015.

This CL replaces https://codereview.chromium.org/1631603002/

BUG=569879
==========

[commit-bot: I haz the power, 2016-03-01 23:34:56]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-03-01 23:37:14]

Description was changed from

==========
Web restrictions component.

The Web restrictions component provides a resource throttle that allows Chrome
on Android and Webview to restrict URL access using a content provider provided
by a different app.
It also allows that content provider to return custom error pages when requests
fail, that can then be used by the user to request access to those URLs.

This is intended for use, in particular:
- For supervised users of Webview, where Chrome provides the content provider.
- For enterprise environments, where the content provider will be provided by a
third party app.

The individual pieces are:
A content resolver based web restriction provider.
A gin wrapper and a resource throttle to tie it together.

Preliminary code making use of this is in
https://codereview.chromium.org/1423713015.

This CL replaces https://codereview.chromium.org/1631603002/

BUG=569879
==========

to

==========
Web restrictions component.

The Web restrictions component provides a resource throttle that allows Chrome
on Android and Webview to restrict URL access using a content provider provided
by a different app.
It also allows that content provider to return custom error pages when requests
fail, that can then be used by the user to request access to those URLs.

This is intended for use, in particular:
- For supervised users of Webview, where Chrome provides the content provider.
- For enterprise environments, where the content provider will be provided by a
third party app.

The individual pieces are:
A content resolver based web restriction provider.
A gin wrapper and a resource throttle to tie it together.

Preliminary code making use of this is in
https://codereview.chromium.org/1423713015.

This CL replaces https://codereview.chromium.org/1631603002/

BUG=569879

Committed: https://crrev.com/d6457207130f1a5abb0d36a27fbf9f4cb31f131f
Cr-Commit-Position: refs/heads/master@{#378601}
==========

[commit-bot: I haz the power, 2016-03-01 23:37:16]

Patchset 13 (id:??) landed as
https://crrev.com/d6457207130f1a5abb0d36a27fbf9f4cb31f131f
Cr-Commit-Position: refs/heads/master@{#378601}