Implement Web Restrictions in WebView.

Implement Web Restrictions in WebView.

This CL:
- Lets WebView ask a content provider (the name of which is set by a
WebView policy) whether a URL should be accessible.
- Implements an error page when a URL is not accessible, that, if it
is supported by the content provider, lets the user request access
- Makes some fixes to the supervised user content provider so that
this works with WebView.

BUG=604330
Committed: https://crrev.com/a7f273b27fd4f7ec3d809c83b4fb53b0f3167c55
Cr-Commit-Position: refs/heads/master@{#412791}

[aberent, 2016-04-15 18:15:03]

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

[aberent, 2016-04-15 18:16:04]

Description was changed from

==========
Implement Web Restrictions in WebView.

This CL:
- Lets WebView ask a content provider (the name of which is set by a
WebView policy) whether a CL should be accessible.
- Implements an error page when a URL is not accessible, that, if it
is supported by the content provider, lets the user request access
- Makes some fixes to the supervised user content provider so that
this works with WebView.

Implement Web Restrictions in WebView.

This CL:
- Lets WebView ask a content provider (the name of which is set by a
WebView policy) whether a CL should be accessible.
- Implements an error page when a URL is not accessible, that, if it
is supported by the content provider, lets the user request access
- Makes some fixes to the supervised user content provider so that
this works with WebView.

Policy now working


Unicorn on WebView working, needs lots of tidying up


Working!

BUG=
==========

to

==========
Implement Web Restrictions in WebView.

This CL:
- Lets WebView ask a content provider (the name of which is set by a
WebView policy) whether a CL should be accessible.
- Implements an error page when a URL is not accessible, that, if it
is supported by the content provider, lets the user request access
- Makes some fixes to the supervised user content provider so that
this works with WebView.

BUG=
==========

[aberent, 2016-04-15 18:25:37]

On 2016/04/15 18:15:03, aberent wrote:

Note that this replaces and incorporated
https://codereview.chromium.org/1423713015/

[aberent, 2016-04-15 18:26:01]

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

[aberent, 2016-04-15 18:26:02]

jochen@chromium.org: Please review changes in

[jochen (gone - plz use gerrit), 2016-04-17 17:07:06]

can you reference a tracking bug from the cl description please?

https://codereview.chromium.org/1890203002/diff/1/android_webview/renderer/aw...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/1890203002/diff/1/android_webview/renderer/aw...
android_webview/renderer/aw_content_renderer_client.cc:294:
render_frame->GetWebFrame()->loadHTMLString(
why does this not override the previously installed gin wrapper?

https://codereview.chromium.org/1890203002/diff/1/components/supervised_user_...
File components/supervised_user_error_page/supervised_user_gin_wrapper.cc
(right):

https://codereview.chromium.org/1890203002/diff/1/components/supervised_user_...
components/supervised_user_error_page/supervised_user_gin_wrapper.cc:68:
render_frame()->ExecuteJavaScript(base::ASCIIToUTF16(jsFunc));
please have the requestPermission method take a callback paramter you invoke
here instead of directly calling to a JS method.

see eg here for how to do this:
https://code.google.com/p/chromium/codesearch#chromium/src/components/test_ru...

https://codereview.chromium.org/1890203002/diff/1/components/supervised_user_...
File components/supervised_user_error_page/supervised_user_gin_wrapper.h
(right):

https://codereview.chromium.org/1890203002/diff/1/components/supervised_user_...
components/supervised_user_error_page/supervised_user_gin_wrapper.h:6: #define
COMPONENTS_WEB_RESTRICTION_WEB_RESTRICTION_GIN_WRAPPER_H_
please update the macro name

[Thiemo Nagel, 2016-04-18 09:01:39]

> Lets WebView ask a content provider (the name of which is set by a
> WebView policy) whether a CL should be accessible.

s/CL/URL/

[aberent, 2016-04-18 09:53:42]

Description was changed from

==========
Implement Web Restrictions in WebView.

This CL:
- Lets WebView ask a content provider (the name of which is set by a
WebView policy) whether a CL should be accessible.
- Implements an error page when a URL is not accessible, that, if it
is supported by the content provider, lets the user request access
- Makes some fixes to the supervised user content provider so that
this works with WebView.

BUG=
==========

to

==========
Implement Web Restrictions in WebView.

This CL:
- Lets WebView ask a content provider (the name of which is set by a
WebView policy) whether a URL should be accessible.
- Implements an error page when a URL is not accessible, that, if it
is supported by the content provider, lets the user request access
- Makes some fixes to the supervised user content provider so that
this works with WebView.

BUG=604330
==========

[aberent, 2016-04-18 10:26:57]

On 2016/04/18 09:01:39, Thiemo Nagel (no reviews) wrote:
> > Lets WebView ask a content provider (the name of which is set by a
> > WebView policy) whether a CL should be accessible.
> 
> s/CL/URL/

Fixed, and tracking bug created.

[aberent, 2016-04-18 12:45:12]

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

[commit-bot: I haz the power, 2016-04-18 12:45:26]

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

[commit-bot: I haz the power, 2016-04-18 12:48:01]

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

[commit-bot: I haz the power, 2016-04-18 12:48:04]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aberent, 2016-04-18 14:24:38]

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

[commit-bot: I haz the power, 2016-04-18 14:24:45]

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

[commit-bot: I haz the power, 2016-04-18 14:30:53]

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

[commit-bot: I haz the power, 2016-04-18 14:30:54]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Bernhard Bauer, 2016-04-18 14:48:35]

https://codereview.chromium.org/1890203002/diff/1/android_webview/browser/aw_...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/1890203002/diff/1/android_webview/browser/aw_...
android_webview/browser/aw_browser_context.h:175: StringPrefMember
web_restrictions_authority_;
You only need a PrefMember if you want to read the value on a different thread,
or much more frequently than it's written. Otherwise, you register for changes
with PrefRegistrar.

https://codereview.chromium.org/1890203002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1890203002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:53:
static class SupervisedUserReply<T> {
I still think you could do this with a BlockingQueue of capacity 1, with add()
for insertion, and take() or poll() for removal.

https://codereview.chromium.org/1890203002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:66:
mLatch.await();
Right now we block indefinitely, right? We might want to add a timeout of some
sorts...

https://codereview.chromium.org/1890203002/diff/1/components/supervised_user_...
File
components/supervised_user_error_page/resources/supervised_user_block_interstitial.js
(right):

https://codereview.chromium.org/1890203002/diff/1/components/supervised_user_...
components/supervised_user_error_page/resources/supervised_user_block_interstitial.js:55:
$('back-button').hidden = showDetailsLink;
Do we want to hide the back button in this case?

https://codereview.chromium.org/1890203002/diff/1/components/supervised_user_...
components/supervised_user_error_page/resources/supervised_user_block_interstitial.js:75:
if (window.domAutomationController &&
Not sure this is necessary -- just don't set 'showFeedbackLink' to true for
WebView?

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
File components/web_restrictions/browser/web_restictions_client_result.h
(right):

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restictions_client_result.h:6: #define
COMPONENTS_WEB_RESTRICTIONS_BROWSER_WEB_RESTICTIONS_CLIENT_RESULT_H_
This file should be .../web_restrictions_client_result.h (missing an "r").

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restictions_client_result.h:18:
WebRestrictionsClientResult();
Is this constructor used?

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restictions_client_result.h:20:
base::android::ScopedJavaGlobalRef<jobject>& jresult);
Nit: empty line after the constructors.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restictions_client_result.h:32:
base::android::ScopedJavaGlobalRef<jobject> jresult_;
DISALLOW_COPY_AND_ASSIGN? It would be weird to make a copy of this object.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
File components/web_restrictions/browser/web_restrictions_client.cc (right):

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.cc:46:
io_thread_task_runner_ = content::BrowserThread::GetMessageLoopProxyForThread(
If you are not injecting this as a dependency, just directly post to the IO
thread using BrowserThread::PostTask. That way you can also check thread usage
with DCHECK_CURRENTLY_ON(), which gives nicer error messages.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.cc:69:
SetAuthorityTask(content_provider_authority);
You could just friend the test class and call SetAuthorityTask() directly in the
test, no?

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.cc:118: if
(result.get()) {
The .get() is unnecessary -- unique_ptr has a bool overload.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.cc:162: std::string
url,
This should be const-ref (as should have been the GURL previously).

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.cc:223:
lock_.Acquire();
Use base::AutoLock for this. That way you can also return nullptr if the entry
is not in the cache, so you don't need the local variable for |result|.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.cc:253: void
NotifyWebRestrictionsChanged(
Actually, we could remove this and just directly call OnWebRestrictionsChanged()
from Java (with the appropriate signa ture changes), no?

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
File components/web_restrictions/browser/web_restrictions_client.h (right):

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.h:25: class Lock;
You already include the header for this.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.h:32: class
WebRestrictionsCache {
Can't this be inside of WebRestrictionsClient?

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.h:34:
std::unique_ptr<const WebRestrictionsClientResult> GetCacheEntry(
The const seems a bit much. If a client gets a fresh copy, they can do with it
what they want anyway, and there aren't even any non-const methods on this
class, so it's not like we avoid accidental mutation.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.h:43:
std::unordered_map<std::string, WebRestrictionsClientResult> cache_data_;
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.h:65: const
std::string& url,
Why the change to std::string? I would use a GURL for as long as possible (i.e.
only stringify it right before going to Java).

[Torne, 2016-04-20 16:12:32]

Haven't reviewed the whole thing in detail, but took a look through the
android_webview changes and had a couple of questions:

https://codereview.chromium.org/1890203002/diff/20001/android_webview/BUILD.gn
File android_webview/BUILD.gn (right):

https://codereview.chromium.org/1890203002/diff/20001/android_webview/BUILD.g...
android_webview/BUILD.gn:105:
"$root_gen_dir/components/components_resources.pak",
What's the APK size impact of adding these additional resources?

(also, what's the APK size impact of this change in general?)

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

https://codereview.chromium.org/1890203002/diff/20001/android_webview/rendere...
android_webview/renderer/aw_content_renderer_client.cc:212: if (error.reason ==
net::ERR_BLOCKED_BY_ADMINISTRATOR) {
Is this kicking off an async request to get a better error page, and then
allowing the default error page to get rendered synchronously? It's not very
clear; maybe a comment? (or am I reading it wrong)

https://codereview.chromium.org/1890203002/diff/20001/android_webview/rendere...
android_webview/renderer/aw_content_renderer_client.cc:294:
render_frame->GetWebFrame()->loadHTMLString(
Is this observable to the application? (other than by inspecting the DOM; I mean
does it trigger any callbacks in webview). I guess this doesn't count as a
navigation?

[Torne, 2016-04-20 16:12:33]

Haven't reviewed the whole thing in detail, but took a look through the
android_webview changes and had a couple of questions:

https://codereview.chromium.org/1890203002/diff/20001/android_webview/BUILD.gn
File android_webview/BUILD.gn (right):

https://codereview.chromium.org/1890203002/diff/20001/android_webview/BUILD.g...
android_webview/BUILD.gn:105:
"$root_gen_dir/components/components_resources.pak",
What's the APK size impact of adding these additional resources?

(also, what's the APK size impact of this change in general?)

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

https://codereview.chromium.org/1890203002/diff/20001/android_webview/rendere...
android_webview/renderer/aw_content_renderer_client.cc:212: if (error.reason ==
net::ERR_BLOCKED_BY_ADMINISTRATOR) {
Is this kicking off an async request to get a better error page, and then
allowing the default error page to get rendered synchronously? It's not very
clear; maybe a comment? (or am I reading it wrong)

https://codereview.chromium.org/1890203002/diff/20001/android_webview/rendere...
android_webview/renderer/aw_content_renderer_client.cc:294:
render_frame->GetWebFrame()->loadHTMLString(
Is this observable to the application? (other than by inspecting the DOM; I mean
does it trigger any callbacks in webview). I guess this doesn't count as a
navigation?

[aberent, 2016-05-18 20:06:51]

https://codereview.chromium.org/1890203002/diff/1/android_webview/browser/aw_...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/1890203002/diff/1/android_webview/browser/aw_...
android_webview/browser/aw_browser_context.h:175: StringPrefMember
web_restrictions_authority_;
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> You only need a PrefMember if you want to read the value on a different
thread,
> or much more frequently than it's written. Otherwise, you register for changes
> with PrefRegistrar.

Done.

https://codereview.chromium.org/1890203002/diff/1/android_webview/renderer/aw...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/1890203002/diff/1/android_webview/renderer/aw...
android_webview/renderer/aw_content_renderer_client.cc:294:
render_frame->GetWebFrame()->loadHTMLString(
On 2016/04/17 17:07:05, jochen wrote:
> why does this not override the previously installed gin wrapper?
I am not sure that it didn't. I have now created a RenderFrameObserver to
install gin wrapper in DidClearWindowObject, which seems to match how the other
gin wrappers are installed, and seems to work reliably.

https://codereview.chromium.org/1890203002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1890203002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:53:
static class SupervisedUserReply<T> {
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> I still think you could do this with a BlockingQueue of capacity 1, with add()
> for insertion, and take() or poll() for removal.

Done.

https://codereview.chromium.org/1890203002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:66:
mLatch.await();
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> Right now we block indefinitely, right? We might want to add a timeout of some
> sorts...

Done.

https://codereview.chromium.org/1890203002/diff/1/components/supervised_user_...
File
components/supervised_user_error_page/resources/supervised_user_block_interstitial.js
(right):

https://codereview.chromium.org/1890203002/diff/1/components/supervised_user_...
components/supervised_user_error_page/resources/supervised_user_block_interstitial.js:55:
$('back-button').hidden = showDetailsLink;
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> Do we want to hide the back button in this case?

Done.

https://codereview.chromium.org/1890203002/diff/1/components/supervised_user_...
components/supervised_user_error_page/resources/supervised_user_block_interstitial.js:75:
if (window.domAutomationController &&
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> Not sure this is necessary -- just don't set 'showFeedbackLink' to true for
> WebView?

We could do, but the code that sets 'showFeedbackLink' is somewhere that doesn't
conveniently know whether it is being called from Chrome or Webview. See
supervised_user_error_page.cc

https://codereview.chromium.org/1890203002/diff/1/components/supervised_user_...
File components/supervised_user_error_page/supervised_user_gin_wrapper.cc
(right):

https://codereview.chromium.org/1890203002/diff/1/components/supervised_user_...
components/supervised_user_error_page/supervised_user_gin_wrapper.cc:68:
render_frame()->ExecuteJavaScript(base::ASCIIToUTF16(jsFunc));
On 2016/04/17 17:07:05, jochen (OOO) wrote:
> please have the requestPermission method take a callback paramter you invoke
> here instead of directly calling to a JS method.
> 
> see eg here for how to do this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/test_ru...

Done. Please, however, review carefully. I have copied code from elsewhere
without fully understanding the V8 interface or gin, so, while it seems to work
I do not really know whether it is correct.

https://codereview.chromium.org/1890203002/diff/1/components/supervised_user_...
File components/supervised_user_error_page/supervised_user_gin_wrapper.h
(right):

https://codereview.chromium.org/1890203002/diff/1/components/supervised_user_...
components/supervised_user_error_page/supervised_user_gin_wrapper.h:6: #define
COMPONENTS_WEB_RESTRICTION_WEB_RESTRICTION_GIN_WRAPPER_H_
On 2016/04/17 17:07:05, jochen wrote:
> please update the macro name 

Done.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
File components/web_restrictions/browser/web_restictions_client_result.h
(right):

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restictions_client_result.h:6: #define
COMPONENTS_WEB_RESTRICTIONS_BROWSER_WEB_RESTICTIONS_CLIENT_RESULT_H_
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> This file should be .../web_restrictions_client_result.h (missing an "r").

Done.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restictions_client_result.h:18:
WebRestrictionsClientResult();
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> Is this constructor used?
Not now that I have changed the code to use std::map::emplace instead of
std::map::operator[]. Done.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restictions_client_result.h:20:
base::android::ScopedJavaGlobalRef<jobject>& jresult);
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> Nit: empty line after the constructors.

Done.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restictions_client_result.h:32:
base::android::ScopedJavaGlobalRef<jobject> jresult_;
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> DISALLOW_COPY_AND_ASSIGN? It would be weird to make a copy of this object.

No. It needs to be copyable:
1 - To allow its insertion in the cache.
2 - To allow its thread-safe use. See web_restrictions_client.cc

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
File components/web_restrictions/browser/web_restrictions_client.cc (right):

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.cc:46:
io_thread_task_runner_ = content::BrowserThread::GetMessageLoopProxyForThread(
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> If you are not injecting this as a dependency, just directly post to the IO
> thread using BrowserThread::PostTask. That way you can also check thread usage
> with DCHECK_CURRENTLY_ON(), which gives nicer error messages.

Done.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.cc:69:
SetAuthorityTask(content_provider_authority);
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> You could just friend the test class and call SetAuthorityTask() directly in
the
> test, no?

Done.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.cc:118: if
(result.get()) {
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> The .get() is unnecessary -- unique_ptr has a bool overload.

Done.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.cc:162: std::string
url,
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> This should be const-ref (as should have been the GURL previously).

Done.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.cc:223:
lock_.Acquire();
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> Use base::AutoLock for this. That way you can also return nullptr if the entry
> is not in the cache, so you don't need the local variable for |result|.

Done.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.cc:253: void
NotifyWebRestrictionsChanged(
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> Actually, we could remove this and just directly call
OnWebRestrictionsChanged()
> from Java (with the appropriate signa ture changes), no?

Done.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
File components/web_restrictions/browser/web_restrictions_client.h (right):

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.h:25: class Lock;
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> You already include the header for this.

Done.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.h:32: class
WebRestrictionsCache {
On 2016/04/18 14:48:35, Bernhard Bauer wrote:
> Can't this be inside of WebRestrictionsClient?

Done.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.h:34:
std::unique_ptr<const WebRestrictionsClientResult> GetCacheEntry(
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> The const seems a bit much. If a client gets a fresh copy, they can do with it
> what they want anyway, and there aren't even any non-const methods on this
> class, so it's not like we avoid accidental mutation.

Done.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.h:43:
std::unordered_map<std::string, WebRestrictionsClientResult> cache_data_;
On 2016/04/18 14:48:35, Bernhard Bauer wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/1890203002/diff/1/components/web_restrictions...
components/web_restrictions/browser/web_restrictions_client.h:65: const
std::string& url,
On 2016/04/18 14:48:34, Bernhard Bauer wrote:
> Why the change to std::string? I would use a GURL for as long as possible
(i.e.
> only stringify it right before going to Java).

For consistency the methods should all take the same type (string or GURL) for
the url. The Mojo interface requires a string, so I want
GetCachedWebRestrictions and RequestPermission to take strings. If I use a GURL
throughout I do a lot of conversion between strings and GURLs both here and in
the Mojo interface, whereas if I use a string throughout this class a conversion
is only needed in the caller.

Using a string internally also means that I can use std::unordered_map for the
cache without having to define a hash function for GURLs.

[aberent, 2016-05-18 20:15:48]

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

[commit-bot: I haz the power, 2016-05-18 20:16:25]

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

[commit-bot: I haz the power, 2016-05-18 20:20:18]

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

[commit-bot: I haz the power, 2016-05-18 20:20:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Bernhard Bauer, 2016-05-19 10:00:15]

https://codereview.chromium.org/1890203002/diff/60001/android_webview/browser...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/1890203002/diff/60001/android_webview/browser...
android_webview/browser/aw_browser_context.h:172:
std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
You could make the PrefChangeRegistrar a direct member.

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

https://codereview.chromium.org/1890203002/diff/60001/android_webview/rendere...
android_webview/renderer/aw_content_renderer_client.cc:59: namespace {
Nit: Empty lines inside the namespace.

https://codereview.chromium.org/1890203002/diff/60001/android_webview/rendere...
android_webview/renderer/aw_content_renderer_client.cc:239: if
(result->intParams["Allow access requests"]) {
Extract the string (and the others) to constants?

https://codereview.chromium.org/1890203002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1890203002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:66:
mQueue.add(reply);
This would throw an exception anyway if there was an element in the queue.

https://codereview.chromium.org/1890203002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:256:
* Only used for testing
This comment is basically implied by the annotation :)

https://codereview.chromium.org/1890203002/diff/60001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1890203002/diff/60001/components/policy/resou...
components/policy/resources/policy_templates.json:8528: 'supported_on':
['webview_android:52-'],
Add android here as well? Alternatively, update the description to remove the
browser.

https://codereview.chromium.org/1890203002/diff/60001/components/supervised_u...
File components/supervised_user_error_page/gin_wrapper.cc (right):

https://codereview.chromium.org/1890203002/diff/60001/components/supervised_u...
components/supervised_user_error_page/gin_wrapper.cc:42: // render frame. Remove
it so that the wrapper doesn't re-installed when
Nit: "reinstalls itself" or "doesn't get reinstalled".

https://codereview.chromium.org/1890203002/diff/60001/components/web_restrict...
File components/web_restrictions/browser/web_restrictions_client.h (right):

https://codereview.chromium.org/1890203002/diff/60001/components/web_restrict...
components/web_restrictions/browser/web_restrictions_client.h:116: //
synchronously
Friend declarations come first in the private section.

https://codereview.chromium.org/1890203002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1890203002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:68777: -             ChromeOS version"/>
Merge conflict?

[aberent, 2016-05-19 15:44:03]

https://codereview.chromium.org/1890203002/diff/60001/android_webview/browser...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/1890203002/diff/60001/android_webview/browser...
android_webview/browser/aw_browser_context.h:172:
std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
On 2016/05/19 10:00:15, Bernhard Bauer wrote:
> You could make the PrefChangeRegistrar a direct member.

Done.

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

https://codereview.chromium.org/1890203002/diff/60001/android_webview/rendere...
android_webview/renderer/aw_content_renderer_client.cc:59: namespace {
On 2016/05/19 10:00:15, Bernhard Bauer wrote:
> Nit: Empty lines inside the namespace.

Done.

https://codereview.chromium.org/1890203002/diff/60001/android_webview/rendere...
android_webview/renderer/aw_content_renderer_client.cc:239: if
(result->intParams["Allow access requests"]) {
On 2016/05/19 10:00:15, Bernhard Bauer wrote:
> Extract the string (and the others) to constants?

Actually I have just taken this if statement out. Installing the GIN wrapper is
harmless if we don't allow access requests, and extra logic gives more
opportunity for bugs.

I don't know where the other strings in this file come from, and they are
nothing to do with this CL, so I don't want to touch them.

https://codereview.chromium.org/1890203002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1890203002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:66:
mQueue.add(reply);
On 2016/05/19 10:00:15, Bernhard Bauer wrote:
> This would throw an exception anyway if there was an element in the queue.

Done.

https://codereview.chromium.org/1890203002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:256:
* Only used for testing
On 2016/05/19 10:00:15, Bernhard Bauer wrote:
> This comment is basically implied by the annotation :)

Done.

https://codereview.chromium.org/1890203002/diff/60001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1890203002/diff/60001/components/policy/resou...
components/policy/resources/policy_templates.json:8528: 'supported_on':
['webview_android:52-'],
On 2016/05/19 10:00:15, Bernhard Bauer wrote:
> Add android here as well? Alternatively, update the description to remove the
> browser.

Removed browser from description for now (this will be a separate CL). Also
changed version to 53, because this isn't going to land before the branch point
(today).

https://codereview.chromium.org/1890203002/diff/60001/components/supervised_u...
File components/supervised_user_error_page/gin_wrapper.cc (right):

https://codereview.chromium.org/1890203002/diff/60001/components/supervised_u...
components/supervised_user_error_page/gin_wrapper.cc:42: // render frame. Remove
it so that the wrapper doesn't re-installed when
On 2016/05/19 10:00:15, Bernhard Bauer wrote:
> Nit: "reinstalls itself" or "doesn't get reinstalled".

Done (different wording).

https://codereview.chromium.org/1890203002/diff/60001/components/web_restrict...
File components/web_restrictions/browser/web_restrictions_client.h (right):

https://codereview.chromium.org/1890203002/diff/60001/components/web_restrict...
components/web_restrictions/browser/web_restrictions_client.h:116: //
synchronously
On 2016/05/19 10:00:15, Bernhard Bauer wrote:
> Friend declarations come first in the private section.

Done.

https://codereview.chromium.org/1890203002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1890203002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:68777: -             ChromeOS version"/>
On 2016/05/19 10:00:15, Bernhard Bauer wrote:
> Merge conflict?

Fixed by local rebase.

[aberent, 2016-05-19 17:18:59]

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

[commit-bot: I haz the power, 2016-05-19 17:19:39]

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

[aberent, 2016-05-19 17:22:13]

https://codereview.chromium.org/1890203002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1890203002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:68777: -             ChromeOS version"/>
On 2016/05/19 15:44:03, aberent wrote:
> On 2016/05/19 10:00:15, Bernhard Bauer wrote:
> > Merge conflict?
> 
> Fixed by local rebase.

Actually maybe not, I need to investigate what is actually going on here.

[commit-bot: I haz the power, 2016-05-19 17:39:49]

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

[commit-bot: I haz the power, 2016-05-19 17:39:50]

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

[Bernhard Bauer, 2016-05-20 08:09:18]

LGTM!

[aberent, 2016-06-07 14:58:30]

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

[commit-bot: I haz the power, 2016-06-07 14:58:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1890203002/90001

[commit-bot: I haz the power, 2016-06-07 15:22:01]

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

[commit-bot: I haz the power, 2016-06-07 15:22:03]

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

[jochen (gone - plz use gerrit), 2016-06-07 19:16:18]

lgtm with comments

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
File components/supervised_user_error_page/DEPS (right):

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
components/supervised_user_error_page/DEPS:2: "+components/web_restrictions",
that includes browser code, please restrict to /renderer

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
File components/supervised_user_error_page/gin_wrapper.cc (right):

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
components/supervised_user_error_page/gin_wrapper.cc:32: return;
won't "this" leak?

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
components/supervised_user_error_page/gin_wrapper.cc:37: return;
same here

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
File components/supervised_user_error_page/gin_wrapper.h (right):

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
components/supervised_user_error_page/gin_wrapper.h:33: class Loader : public
content::RenderFrameObserver {
please add a virtual dtor

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
components/supervised_user_error_page/gin_wrapper.h:46: };
disallow copy & assign

[jochen (gone - plz use gerrit), 2016-06-07 19:16:36]

lgtm with comments

[aberent, 2016-06-08 09:47:41]

torne@ - I am still looking at how to remove unnecessary resources from the APK,
but I am now back to working on this and it is otherwise ready for a detailed
review. PTAL at the WebView components.

https://codereview.chromium.org/1890203002/diff/20001/android_webview/BUILD.gn
File android_webview/BUILD.gn (right):

https://codereview.chromium.org/1890203002/diff/20001/android_webview/BUILD.g...
android_webview/BUILD.gn:105:
"$root_gen_dir/components/components_resources.pak",
On 2016/04/20 16:12:33, Torne wrote:
> What's the APK size impact of adding these additional resources?
> 
> (also, what's the APK size impact of this change in general?)
At present it adds ~1.5M to the APK file size (is this a good measure?), 90% of
which is accounted for by the two new pak files (component_resources.pak and
webui_resources.pak). I will investigate what I can do to reduce this.

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

https://codereview.chromium.org/1890203002/diff/20001/android_webview/rendere...
android_webview/renderer/aw_content_renderer_client.cc:212: if (error.reason ==
net::ERR_BLOCKED_BY_ADMINISTRATOR) {
On 2016/04/20 16:12:33, Torne wrote:
> Is this kicking off an async request to get a better error page, and then
> allowing the default error page to get rendered synchronously? It's not very
> clear; maybe a comment? (or am I reading it wrong)

No longer applies. I have changed this get the error page details synchronously.
Anything else gives leaves too many openings for race conditions etc.

https://codereview.chromium.org/1890203002/diff/20001/android_webview/rendere...
android_webview/renderer/aw_content_renderer_client.cc:294:
render_frame->GetWebFrame()->loadHTMLString(
On 2016/04/20 16:12:33, Torne wrote:
> Is this observable to the application? (other than by inspecting the DOM; I
mean
> does it trigger any callbacks in webview). I guess this doesn't count as a
> navigation?

Removed since the error page is now built synchronously.

[Thiemo Nagel, 2016-06-08 12:24:58]

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

[Thiemo Nagel, 2016-06-08 12:25:00]

https://codereview.chromium.org/1890203002/diff/90001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1890203002/diff/90001/components/policy/resou...
components/policy/resources/policy_templates.json:8607: 'tags': [],
Please add 'filtering'.

[aberent, 2016-07-12 18:48:25]

To avoid bloating WebView's resources the new patch sets only allow access
requests in Monochrome. This makes sense since it anyway is only intended for
Android N or later, and making access requests is only possible if both WebView
and Chrome are installed and enabled, which on Android N and later means that
Monochrome replaces both of them.

Note that this still has to be tested with Monochrome, since I am awaiting a fix
to the Monochrome build system. I will not land it without completing this test.

https://codereview.chromium.org/1890203002/diff/90001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1890203002/diff/90001/components/policy/resou...
components/policy/resources/policy_templates.json:8607: 'tags': [],
On 2016/06/08 12:24:59, Thiemo Nagel wrote:
> Please add 'filtering'.

Done.

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
File components/supervised_user_error_page/DEPS (right):

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
components/supervised_user_error_page/DEPS:2: "+components/web_restrictions",
On 2016/06/07 19:16:18, jochen wrote:
> that includes browser code, please restrict to /renderer

Actually the dependency is on
components/web_restrictions/interfaces/web_restrictions.mojom.h which is common
to the browser and the renderer, so I have changed this to 

"+components/web_restrictions/interfaces",

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
File components/supervised_user_error_page/gin_wrapper.cc (right):

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
components/supervised_user_error_page/gin_wrapper.cc:32: return;
On 2016/06/07 19:16:18, jochen wrote:
> won't "this" leak?

Done.

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
components/supervised_user_error_page/gin_wrapper.cc:37: return;
On 2016/06/07 19:16:18, jochen wrote:
> same here

Done.

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
File components/supervised_user_error_page/gin_wrapper.h (right):

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
components/supervised_user_error_page/gin_wrapper.h:33: class Loader : public
content::RenderFrameObserver {
On 2016/06/07 19:16:18, jochen wrote:
> please add a virtual dtor

Done.

https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
components/supervised_user_error_page/gin_wrapper.h:46: };
On 2016/06/07 19:16:18, jochen wrote:
> disallow copy & assign

Done.

[aberent, 2016-07-14 16:06:53]

On 2016/07/12 18:48:25, aberent wrote:
> To avoid bloating WebView's resources the new patch sets only allow access
> requests in Monochrome. This makes sense since it anyway is only intended for
> Android N or later, and making access requests is only possible if both
WebView
> and Chrome are installed and enabled, which on Android N and later means that
> Monochrome replaces both of them.
> 
> Note that this still has to be tested with Monochrome, since I am awaiting a
fix
> to the Monochrome build system. I will not land it without completing this
test.
> 
>
https://codereview.chromium.org/1890203002/diff/90001/components/policy/resou...
> File components/policy/resources/policy_templates.json (right):
> 
>
https://codereview.chromium.org/1890203002/diff/90001/components/policy/resou...
> components/policy/resources/policy_templates.json:8607: 'tags': [],
> On 2016/06/08 12:24:59, Thiemo Nagel wrote:
> > Please add 'filtering'.
> 
> Done.
> 
>
https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
> File components/supervised_user_error_page/DEPS (right):
> 
>
https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
> components/supervised_user_error_page/DEPS:2: "+components/web_restrictions",
> On 2016/06/07 19:16:18, jochen wrote:
> > that includes browser code, please restrict to /renderer
> 
> Actually the dependency is on
> components/web_restrictions/interfaces/web_restrictions.mojom.h which is
common
> to the browser and the renderer, so I have changed this to 
> 
> "+components/web_restrictions/interfaces",
> 
>
https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
> File components/supervised_user_error_page/gin_wrapper.cc (right):
> 
>
https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
> components/supervised_user_error_page/gin_wrapper.cc:32: return;
> On 2016/06/07 19:16:18, jochen wrote:
> > won't "this" leak?
> 
> Done.
> 
>
https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
> components/supervised_user_error_page/gin_wrapper.cc:37: return;
> On 2016/06/07 19:16:18, jochen wrote:
> > same here
> 
> Done.
> 
>
https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
> File components/supervised_user_error_page/gin_wrapper.h (right):
> 
>
https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
> components/supervised_user_error_page/gin_wrapper.h:33: class Loader : public
> content::RenderFrameObserver {
> On 2016/06/07 19:16:18, jochen wrote:
> > please add a virtual dtor
> 
> Done.
> 
>
https://codereview.chromium.org/1890203002/diff/90001/components/supervised_u...
> components/supervised_user_error_page/gin_wrapper.h:46: };
> On 2016/06/07 19:16:18, jochen wrote:
> > disallow copy & assign
> 
> Done.

There will be a slight delay before I can test this with Monochrome, Monochrome
only works on N, and we can't yet set up child users on N.

[Thiemo Nagel, 2016-07-15 12:17:58]

*/policy/* lgtm

[aberent, 2016-08-02 15:30:25]

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

[commit-bot: I haz the power, 2016-08-02 15:30:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 15:56:30]

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

[commit-bot: I haz the power, 2016-08-02 15:56:32]

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

[aberent, 2016-08-03 13:36:24]

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

[commit-bot: I haz the power, 2016-08-03 13:36:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 14:55:10]

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

[commit-bot: I haz the power, 2016-08-03 14:55:12]

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

[aberent, 2016-08-03 16:51:58]

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

[commit-bot: I haz the power, 2016-08-03 16:52:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aberent, 2016-08-03 17:00:37]

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

[commit-bot: I haz the power, 2016-08-03 18:23:41]

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

[commit-bot: I haz the power, 2016-08-03 18:23:42]

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

[aberent, 2016-08-03 18:30:36]

tsepez@ - accidently sent too soon, security review please of
components/web_restrictions/interfaces/web_restrictions.mojom

[Tom Sepez, 2016-08-03 20:55:41]

Mojom LGTM with nits.

https://codereview.chromium.org/1890203002/diff/160001/components/web_restric...
File components/web_restrictions/interfaces/web_restrictions.mojom (right):

https://codereview.chromium.org/1890203002/diff/160001/components/web_restric...
components/web_restrictions/interfaces/web_restrictions.mojom:13: // Get the
cached result, if any, of requesting access to a URL; including key/value pairs
for
nit: 80 cols.

https://codereview.chromium.org/1890203002/diff/160001/components/web_restric...
components/web_restrictions/interfaces/web_restrictions.mojom:17:
GetResult(string url) => (ClientResult? reply);
nit: does optional buy anything here vs. a result with two empty arrays?

[aberent, 2016-08-04 15:39:36]

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

[commit-bot: I haz the power, 2016-08-04 15:40:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 16:06:34]

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

[commit-bot: I haz the power, 2016-08-04 16:06:36]

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

[aberent, 2016-08-16 16:55:44]

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

[aberent, 2016-08-16 16:55:45]

Now works with Monochrome.

torne@ - PTAL and approve or comment on webview code.

sky@ - please review dependency on mojo, services/shell, and ui.

brettw@ - please review dependency on url.

https://codereview.chromium.org/1890203002/diff/160001/components/web_restric...
File components/web_restrictions/interfaces/web_restrictions.mojom (right):

https://codereview.chromium.org/1890203002/diff/160001/components/web_restric...
components/web_restrictions/interfaces/web_restrictions.mojom:13: // Get the
cached result, if any, of requesting access to a URL; including key/value pairs
for
On 2016/08/03 20:55:41, Tom Sepez wrote:
> nit: 80 cols.

Done.

https://codereview.chromium.org/1890203002/diff/160001/components/web_restric...
components/web_restrictions/interfaces/web_restrictions.mojom:17:
GetResult(string url) => (ClientResult? reply);
On 2016/08/03 20:55:41, Tom Sepez wrote:
> nit: does optional buy anything here vs. a result with two empty arrays?

Done.

[sky, 2016-08-16 17:27:27]

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

https://codereview.chromium.org/1890203002/diff/220001/components/web_restric...
components/web_restrictions/browser/DEPS:5: "+mojo",
Are you sure you need mojo and not a more specific subdir? I looked in
components/web_restrictions/BUILD.gn and didn't see any added deps/public_deps
on mojo. What are you using in mojo?

[aberent, 2016-08-16 17:29:44]

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

[commit-bot: I haz the power, 2016-08-16 17:29:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 17:32:46]

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

[commit-bot: I haz the power, 2016-08-16 17:32:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aberent, 2016-08-16 18:45:49]

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

https://codereview.chromium.org/1890203002/diff/220001/components/web_restric...
components/web_restrictions/browser/DEPS:5: "+mojo",
On 2016/08/16 17:27:27, sky wrote:
> Are you sure you need mojo and not a more specific subdir? I looked in
> components/web_restrictions/BUILD.gn and didn't see any added deps/public_deps
> on mojo. What are you using in mojo?

Done, now dependent on mojo/public/cpp/bindings (I am never sure how specific
DEPS entries should be, is this too specific?). There was also a dependency (a
reference to a header file) missing from BUILD.gn, now fixed.

[aberent, 2016-08-16 19:35:05]

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

[commit-bot: I haz the power, 2016-08-16 19:35:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 19:42:29]

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

[commit-bot: I haz the power, 2016-08-16 19:42:30]

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

[sky, 2016-08-16 19:58:47]

LGTM

[aberent, 2016-08-17 10:25:56]

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

[commit-bot: I haz the power, 2016-08-17 10:26:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 10:33:00]

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

[commit-bot: I haz the power, 2016-08-17 10:33:01]

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

[aberent, 2016-08-17 10:41:02]

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

[commit-bot: I haz the power, 2016-08-17 10:41:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 10:45:24]

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

[commit-bot: I haz the power, 2016-08-17 10:45:26]

Dry run: This issue passed the CQ dry run.

[brettw, 2016-08-17 17:45:44]

url dep LGTM

https://codereview.chromium.org/1890203002/diff/260001/components/supervised_...
File components/supervised_user_error_page/BUILD.gn (right):

https://codereview.chromium.org/1890203002/diff/260001/components/supervised_...
components/supervised_user_error_page/BUILD.gn:36: "//gin:gin",
This should just be "//gin" and below "//url" rather than duplicating the name.

[Torne, 2016-08-17 18:20:56]

https://codereview.chromium.org/1890203002/diff/260001/android_webview/lib/ma...
File android_webview/lib/main/webview_jni_onload.cc (right):

https://codereview.chromium.org/1890203002/diff/260001/android_webview/lib/ma...
android_webview/lib/main/webview_jni_onload.cc:16: #include
"components/web_restrictions/browser/web_restrictions_client.h"
nit: no need to add these now that they don't define JNI registration functions
:)

https://codereview.chromium.org/1890203002/diff/260001/android_webview/render...
File android_webview/renderer/DEPS (right):

https://codereview.chromium.org/1890203002/diff/260001/android_webview/render...
android_webview/renderer/DEPS:20: "+services/shell/public/cpp",
Why is this needed?

[Torne, 2016-08-17 19:57:11]

Oops, also meant to say "android_webview LGTM with nits" :)

On Wed, 17 Aug 2016, 7:20 pm , <torne@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1890203002/diff/260001/android_webview/lib/ma...
> File android_webview/lib/main/webview_jni_onload.cc (right):
>
>
>
https://codereview.chromium.org/1890203002/diff/260001/android_webview/lib/ma...
> android_webview/lib/main/webview_jni_onload.cc:16: #include
> "components/web_restrictions/browser/web_restrictions_client.h"
> nit: no need to add these now that they don't define JNI registration
> functions :)
>
>
>
https://codereview.chromium.org/1890203002/diff/260001/android_webview/render...
> File android_webview/renderer/DEPS (right):
>
>
>
https://codereview.chromium.org/1890203002/diff/260001/android_webview/render...
> android_webview/renderer/DEPS:20: "+services/shell/public/cpp",
> Why is this needed?
>
> https://codereview.chromium.org/1890203002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aberent, 2016-08-18 09:39:59]

https://codereview.chromium.org/1890203002/diff/260001/android_webview/lib/ma...
File android_webview/lib/main/webview_jni_onload.cc (right):

https://codereview.chromium.org/1890203002/diff/260001/android_webview/lib/ma...
android_webview/lib/main/webview_jni_onload.cc:16: #include
"components/web_restrictions/browser/web_restrictions_client.h"
On 2016/08/17 18:20:55, Torne wrote:
> nit: no need to add these now that they don't define JNI registration
functions
> :)

Done.

https://codereview.chromium.org/1890203002/diff/260001/android_webview/render...
File android_webview/renderer/DEPS (right):

https://codereview.chromium.org/1890203002/diff/260001/android_webview/render...
android_webview/renderer/DEPS:20: "+services/shell/public/cpp",
On 2016/08/17 18:20:55, Torne wrote:
> Why is this needed?

Needed by aw_content_renderer_client.cc for
services/shell/public/cpp/interface_provider.h, which in turn is needed to get
the Mojo interface.

https://codereview.chromium.org/1890203002/diff/260001/components/supervised_...
File components/supervised_user_error_page/BUILD.gn (right):

https://codereview.chromium.org/1890203002/diff/260001/components/supervised_...
components/supervised_user_error_page/BUILD.gn:36: "//gin:gin",
On 2016/08/17 17:45:44, brettw (ping on IM after 24h) wrote:
> This should just be "//gin" and below "//url" rather than duplicating the
name.

Done.

[aberent, 2016-08-18 09:40:11]

The CQ bit was checked by aberent@chromium.org

[aberent, 2016-08-18 09:40:11]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
jochen@chromium.org, tnagel@chromium.org, tsepez@chromium.org, sky@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1890203002/#ps280001
(title: "Fix final nits")

[commit-bot: I haz the power, 2016-08-18 09:40:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Torne, 2016-08-18 10:01:47]

actual LGTM as i forgot that email doesn't work any more. :p

[commit-bot: I haz the power, 2016-08-18 10:46:32]

Description was changed from

==========
Implement Web Restrictions in WebView.

This CL:
- Lets WebView ask a content provider (the name of which is set by a
WebView policy) whether a URL should be accessible.
- Implements an error page when a URL is not accessible, that, if it
is supported by the content provider, lets the user request access
- Makes some fixes to the supervised user content provider so that
this works with WebView.

BUG=604330
==========

to

==========
Implement Web Restrictions in WebView.

This CL:
- Lets WebView ask a content provider (the name of which is set by a
WebView policy) whether a URL should be accessible.
- Implements an error page when a URL is not accessible, that, if it
is supported by the content provider, lets the user request access
- Makes some fixes to the supervised user content provider so that
this works with WebView.

BUG=604330
==========

[commit-bot: I haz the power, 2016-08-18 10:46:35]

Committed patchset #16 (id:280001)

[commit-bot: I haz the power, 2016-08-18 10:48:30]

Description was changed from

==========
Implement Web Restrictions in WebView.

This CL:
- Lets WebView ask a content provider (the name of which is set by a
WebView policy) whether a URL should be accessible.
- Implements an error page when a URL is not accessible, that, if it
is supported by the content provider, lets the user request access
- Makes some fixes to the supervised user content provider so that
this works with WebView.

BUG=604330
==========

to

==========
Implement Web Restrictions in WebView.

This CL:
- Lets WebView ask a content provider (the name of which is set by a
WebView policy) whether a URL should be accessible.
- Implements an error page when a URL is not accessible, that, if it
is supported by the content provider, lets the user request access
- Makes some fixes to the supervised user content provider so that
this works with WebView.

BUG=604330

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

[commit-bot: I haz the power, 2016-08-18 10:48:32]

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