Supervised user web restrictions content provider

Supervised user web restrictions content provider

The web restrictions content provider lets other apps query
web restrictions. In particular it lets apps using WebView
query Chrome's supervised user web restrictions.

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

[aberent, 2015-11-16 15:52:16]

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

[aberent, 2015-11-16 15:52:16]

Uploaded simply to make my proposed detailed interface visible. Only contains
the abstract class so far, and is a long way from being ready for review.

knn@ - does this match your expectations?

[Bernhard Bauer, 2015-11-18 14:32:23]

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

[Bernhard Bauer, 2015-11-18 14:32:24]

Drive-by nits! 😃

https://codereview.chromium.org/1452603002/diff/1/components/web_restrictions...
File
components/web_restrictions/android/java/src/org/chromium/components/web_restrictions/WebRestrictionsContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/1/components/web_restrictions...
components/web_restrictions/android/java/src/org/chromium/components/web_restrictions/WebRestrictionsContentProvider.java:5:
package org.chromium.components.web_restrictions;
Nit: Underscores in package names are discouraged.

https://codereview.chromium.org/1452603002/diff/1/components/web_restrictions...
components/web_restrictions/android/java/src/org/chromium/components/web_restrictions/WebRestrictionsContentProvider.java:44:
if (!matcher.find()) return null;
If you use .matches() you don't need to manually anchor the pattern.

[aberent, 2015-11-18 15:49:04]

https://codereview.chromium.org/1452603002/diff/1/components/web_restrictions...
File
components/web_restrictions/android/java/src/org/chromium/components/web_restrictions/WebRestrictionsContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/1/components/web_restrictions...
components/web_restrictions/android/java/src/org/chromium/components/web_restrictions/WebRestrictionsContentProvider.java:5:
package org.chromium.components.web_restrictions;
On 2015/11/18 14:32:24, Bernhard Bauer wrote:
> Nit: Underscores in package names are discouraged.

Except for an extra 's', which I will remove, this matches the package name in
knn@'s CL, which, in turn, matches the component name. The package names in
chrome components (even those with underscores) generally match the component
names, although some do remove the underscores.

https://codereview.chromium.org/1452603002/diff/1/components/web_restrictions...
components/web_restrictions/android/java/src/org/chromium/components/web_restrictions/WebRestrictionsContentProvider.java:44:
if (!matcher.find()) return null;
On 2015/11/18 14:32:24, Bernhard Bauer wrote:
> If you use .matches() you don't need to manually anchor the pattern.
Can one extract a group (next line) without anchoring it? I couldn't see that
one could.

[Bernhard Bauer, 2015-11-18 16:40:51]

https://codereview.chromium.org/1452603002/diff/1/components/web_restrictions...
File
components/web_restrictions/android/java/src/org/chromium/components/web_restrictions/WebRestrictionsContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/1/components/web_restrictions...
components/web_restrictions/android/java/src/org/chromium/components/web_restrictions/WebRestrictionsContentProvider.java:5:
package org.chromium.components.web_restrictions;
On 2015/11/18 15:49:04, aberent wrote:
> On 2015/11/18 14:32:24, Bernhard Bauer wrote:
> > Nit: Underscores in package names are discouraged.
> 
> Except for an extra 's', which I will remove, this matches the package name in
> knn@'s CL, which, in turn, matches the component name. The package names in
> chrome components (even those with underscores) generally match the component
> names, although some do remove the underscores.

Yes, the ones that don't remove underscores are wrong 😃 (See
https://codereview.chromium.org/1143313006/diff/1/chrome/android/java_staging...
or the Google Java style guide at
https://google.github.io/styleguide/javaguide.html#s5.2.1-package-names.)

(Also, I would actually keep the plural, as that is more common in Chromium
code, cf. "profiles", "extensions", ...)

https://codereview.chromium.org/1452603002/diff/1/components/web_restrictions...
components/web_restrictions/android/java/src/org/chromium/components/web_restrictions/WebRestrictionsContentProvider.java:44:
if (!matcher.find()) return null;
On 2015/11/18 15:49:04, aberent wrote:
> On 2015/11/18 14:32:24, Bernhard Bauer wrote:
> > If you use .matches() you don't need to manually anchor the pattern.
> Can one extract a group (next line) without anchoring it? I couldn't see that
> one could.

The .matches() method checks the full string, so you don't need the zero-width
assertions that you're at the beginning or the end of the string. I don't think
groups have anything to do with that at all.

[aberent, 2015-11-27 15:52:53]

Description was changed from

==========
Web restrictions content provider

The web restrictions content provider lets other apps query web
restrictions. In particular it lets apps using WebView query
Chrome's supervised user web restrictions.

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

to

==========
Supervised user web restrictions content provider

The web restrictions content provider lets other apps query
web restrictions. In particular it lets apps using WebView query Chrome's
supervised user web restrictions.

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

[aberent, 2015-11-27 15:53:10]

Description was changed from

==========
Supervised user web restrictions content provider

The web restrictions content provider lets other apps query
web restrictions. In particular it lets apps using WebView query Chrome's
supervised user web restrictions.

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

to

==========
Supervised user web restrictions content provider

The web restrictions content provider lets other apps query
web restrictions. In particular it lets apps using WebView
query Chrome's supervised user web restrictions.

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

[aberent, 2015-12-03 19:08:37]

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

[commit-bot: I haz the power, 2015-12-03 19:13:35]

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

[commit-bot: I haz the power, 2015-12-03 19:22:00]

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

[commit-bot: I haz the power, 2015-12-03 19:22:01]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[aberent, 2015-12-14 12:42:56]

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

[commit-bot: I haz the power, 2015-12-14 12:43:01]

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

[commit-bot: I haz the power, 2015-12-14 12:48:54]

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

[commit-bot: I haz the power, 2015-12-14 12:48:55]

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

[aberent, 2015-12-14 16:04:40]

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

[commit-bot: I haz the power, 2015-12-14 16:05:59]

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

[aberent, 2015-12-14 16:14:35]

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

[aberent, 2015-12-14 16:14:35]

bauerb@chromium.org: now ready for full review. PTAL.

jochen@chromium.org: Please review changes, as owner, in chrome.gyp, and
addition of new web_restriction component.

https://codereview.chromium.org/1452603002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:124:
@VisibleForTesting native long nativeCreateSupervisedUserContentProvider();
Line break removed by git cl format :-(

[aberent, 2015-12-14 17:29:11]

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

[commit-bot: I haz the power, 2015-12-14 17:29:48]

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

[Bernhard Bauer, 2015-12-14 17:58:38]

Woo-hoo!

Can you add a BUG= ID for this?

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:26:
private SynchronousQueue<Pair<Boolean, String>> mResultQueue;
Can you make this final?

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:29:
super();
This can be removed, right?

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:30:
mResultQueue = new SynchronousQueue<Pair<Boolean, String>>();
Initialize this where it's declared?

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:51:
protected synchronized Pair<Boolean, String> shouldProceed(final String url) {
Synchronize on a member variable of type Object. Doing this will cause a
FindBugs error.

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:63:
return mResultQueue.take();
Can you add a comment that this will block until another thread puts something
in, just so that a casual reader can understand the flow of control?

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderTest.java
(right):

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderTest.java:41:
// TODO Auto-generated constructor stub
😃

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderTest.java
(right):

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderTest.java:28:
* Tests of SupervisedUserContentProvider This is tested as a simple class, not
as a content
This seems to be missing some punctuation.

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderTest.java:74:
// https://groups.google.com/forum/#!topic/robolectric/mlXNko2NFE0.
Urrr... Note
https://groups.google.com/d/msg/robolectric/mlXNko2NFE0/cvHWx02R9e8J 😃

What is the mechanism that doesn't work here? RunOnUiThread() should work...

https://codereview.chromium.org/1452603002/diff/160001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_content_provider_android.cc
(right):

https://codereview.chromium.org/1452603002/diff/160001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.cc:39:
void DoNothingCallback(bool /*b*/) {}
This is available as base::DoNothing() (in base/bind_helpers.h).

https://codereview.chromium.org/1452603002/diff/160001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.cc:97:
base::Bind(DoNothingCallback));
Hm... in principle we could return an error if creating the permission request
fails.

https://codereview.chromium.org/1452603002/diff/160001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.cc:119:
if (!profile_->IsSupervised()) {
Leave out the braces.

https://codereview.chromium.org/1452603002/diff/160001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_content_provider_android.h
(right):

https://codereview.chromium.org/1452603002/diff/160001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.h:44:
bool RegisterSupervisedUserContentProvider(JNIEnv* env);
Make this a static method on SupervisedUserContentProvider (possibly shortened
to Register())?

https://codereview.chromium.org/1452603002/diff/160001/components/web_restric...
File
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/160001/components/web_restric...
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java:24:
// This class heavily distorts the documented intention of content providers as
database
One way to improve on this would be to have the "insertions" be insertions into
a different "table" than the one we read restrictions from (where the tables are
identified by URI, right?). Then there would be no expectation that query()
after an insert() would return something different.

Also, at some point in the future we could allow querying the set of requests
(to show different UI when the user has already requested access to a site), so
at that point we could model that as a query() on the other "table". We just
don't currently have the backend for that, I think.

https://codereview.chromium.org/1452603002/diff/160001/components/web_restric...
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java:141:
protected abstract Pair<Boolean, String> shouldProceed(final String url);
We could also model this via a class hierarchy, i.e. one subclass that is
"proceed" and one that is "fail with this error page".

https://codereview.chromium.org/1452603002/diff/160001/components/web_restric...
File
components/web_restriction/junit/src/org/chromium/components/webrestriction/WebRestrictionsContentProviderTest.java
(right):

https://codereview.chromium.org/1452603002/diff/160001/components/web_restric...
components/web_restriction/junit/src/org/chromium/components/webrestriction/WebRestrictionsContentProviderTest.java:41:
// TODO Auto-generated method stub
Pls clean up 😃

[commit-bot: I haz the power, 2015-12-14 18:29:20]

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

[commit-bot: I haz the power, 2015-12-14 18:29:21]

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

[aberent, 2015-12-15 11:58:17]

Description was changed from

==========
Supervised user web restrictions content provider

The web restrictions content provider lets other apps query
web restrictions. In particular it lets apps using WebView
query Chrome's supervised user web restrictions.

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

to

==========
Supervised user web restrictions content provider

The web restrictions content provider lets other apps query
web restrictions. In particular it lets apps using WebView
query Chrome's supervised user web restrictions.

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

[jochen (gone - plz use gerrit), 2015-12-16 09:00:37]

gyp stuff and such lgtm

[aberent, 2015-12-16 10:31:19]

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

[aberent, 2015-12-16 15:20:37]

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:26:
private SynchronousQueue<Pair<Boolean, String>> mResultQueue;
On 2015/12/14 17:58:37, Bernhard Bauer wrote:
> Can you make this final?

Done.

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:29:
super();
On 2015/12/14 17:58:37, Bernhard Bauer wrote:
> This can be removed, right?

Done.

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:30:
mResultQueue = new SynchronousQueue<Pair<Boolean, String>>();
On 2015/12/14 17:58:37, Bernhard Bauer wrote:
> Initialize this where it's declared?

Done.

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:51:
protected synchronized Pair<Boolean, String> shouldProceed(final String url) {
On 2015/12/14 17:58:37, Bernhard Bauer wrote:
> Synchronize on a member variable of type Object. Doing this will cause a
> FindBugs error.

Done.

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:63:
return mResultQueue.take();
On 2015/12/14 17:58:37, Bernhard Bauer wrote:
> Can you add a comment that this will block until another thread puts something
> in, just so that a casual reader can understand the flow of control?

Done.

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderTest.java
(right):

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderTest.java:41:
// TODO Auto-generated constructor stub
On 2015/12/14 17:58:37, Bernhard Bauer wrote:
> 😃

Done.

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderTest.java
(right):

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderTest.java:28:
* Tests of SupervisedUserContentProvider This is tested as a simple class, not
as a content
On 2015/12/14 17:58:37, Bernhard Bauer wrote:
> This seems to be missing some punctuation.

Done.

https://codereview.chromium.org/1452603002/diff/160001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderTest.java:74:
// https://groups.google.com/forum/#!topic/robolectric/mlXNko2NFE0.
On 2015/12/14 17:58:37, Bernhard Bauer wrote:
> Urrr... Note
> https://groups.google.com/d/msg/robolectric/mlXNko2NFE0/cvHWx02R9e8J 😃
> 
> What is the mechanism that doesn't work here? RunOnUiThread() should work...

Done. As discussed.

https://codereview.chromium.org/1452603002/diff/160001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_content_provider_android.cc
(right):

https://codereview.chromium.org/1452603002/diff/160001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.cc:39:
void DoNothingCallback(bool /*b*/) {}
On 2015/12/14 17:58:38, Bernhard Bauer wrote:
> This is available as base::DoNothing() (in base/bind_helpers.h).

No it isn't. Base::DoNothing() doesn't take an argument. I need a function with
a single boolean argument.

https://codereview.chromium.org/1452603002/diff/160001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.cc:97:
base::Bind(DoNothingCallback));
On 2015/12/14 17:58:37, Bernhard Bauer wrote:
> Hm... in principle we could return an error if creating the permission request
> fails.

Done.

https://codereview.chromium.org/1452603002/diff/160001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.cc:119:
if (!profile_->IsSupervised()) {
On 2015/12/14 17:58:37, Bernhard Bauer wrote:
> Leave out the braces.

Done.

https://codereview.chromium.org/1452603002/diff/160001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_content_provider_android.h
(right):

https://codereview.chromium.org/1452603002/diff/160001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.h:44:
bool RegisterSupervisedUserContentProvider(JNIEnv* env);
On 2015/12/14 17:58:38, Bernhard Bauer wrote:
> Make this a static method on SupervisedUserContentProvider (possibly shortened
> to Register())?

Done.

https://codereview.chromium.org/1452603002/diff/160001/components/web_restric...
File
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/160001/components/web_restric...
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java:24:
// This class heavily distorts the documented intention of content providers as
database
On 2015/12/14 17:58:38, Bernhard Bauer wrote:
> One way to improve on this would be to have the "insertions" be insertions
into
> a different "table" than the one we read restrictions from (where the tables
are
> identified by URI, right?). Then there would be no expectation that query()
> after an insert() would return something different.
> 
> Also, at some point in the future we could allow querying the set of requests
> (to show different UI when the user has already requested access to a site),
so
> at that point we could model that as a query() on the other "table". We just
> don't currently have the backend for that, I think.

Done.

https://codereview.chromium.org/1452603002/diff/160001/components/web_restric...
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java:141:
protected abstract Pair<Boolean, String> shouldProceed(final String url);
On 2015/12/14 17:58:38, Bernhard Bauer wrote:
> We could also model this via a class hierarchy, i.e. one subclass that is
> "proceed" and one that is "fail with this error page".

I think that would add a (relatively) a lot of code for not much benefit. Unless
I am misunderstanding we would define an interface and two classes, and then
using the result would be quite messy, involving casting the result to the
success and failure classes.

https://codereview.chromium.org/1452603002/diff/160001/components/web_restric...
File
components/web_restriction/junit/src/org/chromium/components/webrestriction/WebRestrictionsContentProviderTest.java
(right):

https://codereview.chromium.org/1452603002/diff/160001/components/web_restric...
components/web_restriction/junit/src/org/chromium/components/webrestriction/WebRestrictionsContentProviderTest.java:41:
// TODO Auto-generated method stub
On 2015/12/14 17:58:38, Bernhard Bauer wrote:
> Pls clean up 😃

Done.

[aberent, 2015-12-16 15:22:54]

https://codereview.chromium.org/1452603002/diff/160001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_content_provider_android.cc
(right):

https://codereview.chromium.org/1452603002/diff/160001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.cc:39:
void DoNothingCallback(bool /*b*/) {}
On 2015/12/16 15:20:37, aberent wrote:
> On 2015/12/14 17:58:38, Bernhard Bauer wrote:
> > This is available as base::DoNothing() (in base/bind_helpers.h).
> 
> No it isn't. Base::DoNothing() doesn't take an argument. I need a function
with
> a single boolean argument.

Actually no longer does nothing (hence renamed), so no longer relevant.

[Bernhard Bauer, 2015-12-16 17:52:10]

Mostly nits, but also a couple of issues that I hadn't found on my first look;
sorry about that.

https://codereview.chromium.org/1452603002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:56:
// See
http://developer.android.com/guide/components/processes-and-threads.html#Thre....
This also means that all requests will effectively be serialized, right? It
should be possible in principle to allow this from multiple threads at the same
time, but it would mean more bookkeeping. Can we add a TODO for that?

https://codereview.chromium.org/1452603002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:80:
// Chromium always allows insertion requests.
Nit: Chromium is just the name of the project. If you are talking about the
piece of software, it's Chrome.

https://codereview.chromium.org/1452603002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:131:
mQueryResultQueue.add(new Pair<Boolean, String>(result, errorMessage));
Does add() actually work if the other thread isn't waiting yet? If I read
https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/BlockingQueue....
correctly, add() would throw an exception if the queue is over capacity, and you
should probably use put() instead.

OTOH, we could make this e.g. a LinkedBlockingQueue with capacity 1, then we
could actually insert the element without blocking, even if the other thread
isn't waiting yet.

https://codereview.chromium.org/1452603002/diff/180001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java
(right):

https://codereview.chromium.org/1452603002/diff/180001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java:98:
// Because supervisedUserContentProvider uses a SynchronousQueue (which is a
java.util
Nit: SupervisedUserContentProvider with capital letters.

https://codereview.chromium.org/1452603002/diff/180001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_content_provider_android.h
(right):

https://codereview.chromium.org/1452603002/diff/180001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.h:24:
jobject caller,
This should also be const base::android::JavaParamRef<jobject>&. torne@ has been
working on getting rid of raw jobject's in JNI code.

https://codereview.chromium.org/1452603002/diff/180001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.h:38:
void OnInsertRequestSendComplete(bool sentOk);
Nit: |sent_ok| for native code.

https://codereview.chromium.org/1452603002/diff/180001/components/web_restric...
File
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/180001/components/web_restric...
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java:30:
private static final int WEB_RESTICTIONS = 0;
WEB_RESTRICTIONS (missing an R)

https://codereview.chromium.org/1452603002/diff/180001/components/web_restric...
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java:53:
mContentUriMatcher = new UriMatcher(WEB_RESTICTIONS);
Actually, this doesn't really seem to be used?

https://codereview.chromium.org/1452603002/diff/180001/components/web_restric...
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java:153:
* @return a pair containing the result and the HTML error message. result is
true if safe to
Nit: Capitalize Result and Error message at the beginning of the sentence.

[aberent, 2015-12-16 22:15:18]

PTAL

https://codereview.chromium.org/1452603002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:56:
// See
http://developer.android.com/guide/components/processes-and-threads.html#Thre....
On 2015/12/16 17:52:09, Bernhard Bauer wrote:
> This also means that all requests will effectively be serialized, right? It
> should be possible in principle to allow this from multiple threads at the
same
> time, but it would mean more bookkeeping. Can we add a TODO for that?

This, together with your comment about add vs. put, got me thinking about
alternatives. I have re-written this to be multithreaded (using countdown
latches instead of queues). Please check that you agree with my new logic.

https://codereview.chromium.org/1452603002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:80:
// Chromium always allows insertion requests.
On 2015/12/16 17:52:09, Bernhard Bauer wrote:
> Nit: Chromium is just the name of the project. If you are talking about the
> piece of software, it's Chrome.

Done.

https://codereview.chromium.org/1452603002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:131:
mQueryResultQueue.add(new Pair<Boolean, String>(result, errorMessage));
On 2015/12/16 17:52:09, Bernhard Bauer wrote:
> Does add() actually work if the other thread isn't waiting yet? If I read
>
https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/BlockingQueue....
> correctly, add() would throw an exception if the queue is over capacity, and
you
> should probably use put() instead.
> 
> OTOH, we could make this e.g. a LinkedBlockingQueue with capacity 1, then we
> could actually insert the element without blocking, even if the other thread
> isn't waiting yet.

See above. put() turned out to be unusable, because it can throw
InterruptedException, which meant that there was no way of guaranteeing that
anything would be put on the queue (hence potentially blocking requests for
ever). My new version will never block the thread providing the reply (which
might be the UI thread), or fail to deliver its reply, and also allows multiple
simultaneous queries.

https://codereview.chromium.org/1452603002/diff/180001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java
(right):

https://codereview.chromium.org/1452603002/diff/180001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java:98:
// Because supervisedUserContentProvider uses a SynchronousQueue (which is a
java.util
On 2015/12/16 17:52:09, Bernhard Bauer wrote:
> Nit: SupervisedUserContentProvider with capital letters.

Done.

https://codereview.chromium.org/1452603002/diff/180001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_content_provider_android.h
(right):

https://codereview.chromium.org/1452603002/diff/180001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.h:24:
jobject caller,
On 2015/12/16 17:52:09, Bernhard Bauer wrote:
> This should also be const base::android::JavaParamRef<jobject>&. torne@ has
been
> working on getting rid of raw jobject's in JNI code.

Done.

https://codereview.chromium.org/1452603002/diff/180001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.h:38:
void OnInsertRequestSendComplete(bool sentOk);
On 2015/12/16 17:52:09, Bernhard Bauer wrote:
> Nit: |sent_ok| for native code.

Done.

https://codereview.chromium.org/1452603002/diff/180001/components/web_restric...
File
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/180001/components/web_restric...
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java:30:
private static final int WEB_RESTICTIONS = 0;
On 2015/12/16 17:52:09, Bernhard Bauer wrote:
> WEB_RESTRICTIONS (missing an R)

Done.

https://codereview.chromium.org/1452603002/diff/180001/components/web_restric...
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java:53:
mContentUriMatcher = new UriMatcher(WEB_RESTICTIONS);
On 2015/12/16 17:52:09, Bernhard Bauer wrote:
> Actually, this doesn't really seem to be used?

No, but the documentation says that one needs a complete tree, and the
implementation (although I didn't look at it closely) seems to assume that there
is a matcher for the base of the tree.

https://codereview.chromium.org/1452603002/diff/180001/components/web_restric...
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java:153:
* @return a pair containing the result and the HTML error message. result is
true if safe to
On 2015/12/16 17:52:09, Bernhard Bauer wrote:
> Nit: Capitalize Result and Error message at the beginning of the sentence.

Done.

[aberent, 2015-12-16 22:16:01]

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

[commit-bot: I haz the power, 2015-12-16 22:16:26]

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

[commit-bot: I haz the power, 2015-12-17 02:44:20]

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

[commit-bot: I haz the power, 2015-12-17 02:44:21]

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

[Bernhard Bauer, 2015-12-17 11:13:21]

https://codereview.chromium.org/1452603002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:131:
mQueryResultQueue.add(new Pair<Boolean, String>(result, errorMessage));
On 2015/12/16 22:15:17, aberent wrote:
> On 2015/12/16 17:52:09, Bernhard Bauer wrote:
> > Does add() actually work if the other thread isn't waiting yet? If I read
> >
>
https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/BlockingQueue....
> > correctly, add() would throw an exception if the queue is over capacity, and
> you
> > should probably use put() instead.
> > 
> > OTOH, we could make this e.g. a LinkedBlockingQueue with capacity 1, then we
> > could actually insert the element without blocking, even if the other thread
> > isn't waiting yet.
> 
> See above. put() turned out to be unusable, because it can throw
> InterruptedException, which meant that there was no way of guaranteeing that
> anything would be put on the queue (hence potentially blocking requests for
> ever). My new version will never block the thread providing the reply (which
> might be the UI thread), or fail to deliver its reply, and also allows
multiple
> simultaneous queries.

Cool! FTR, I don't think we actually have to care about InterruptedException,
because we don't have code that interrupts a thread, so if it does happen, we
could just crash, but yeah, non-blocking completion is best completion.

https://codereview.chromium.org/1452603002/diff/180001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_content_provider_android.h
(right):

https://codereview.chromium.org/1452603002/diff/180001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.h:38:
void OnInsertRequestSendComplete(bool sentOk);
On 2015/12/16 22:15:17, aberent wrote:
> On 2015/12/16 17:52:09, Bernhard Bauer wrote:
> > Nit: |sent_ok| for native code.
> 
> Done.

Not quite 😃

https://codereview.chromium.org/1452603002/diff/180001/components/web_restric...
File
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/180001/components/web_restric...
components/web_restriction/java/src/org/chromium/components/webrestriction/WebRestrictionsContentProvider.java:153:
* @return a pair containing the result and the HTML error message. result is
true if safe to
On 2015/12/16 17:52:09, Bernhard Bauer wrote:
> Nit: Capitalize Result and Error message at the beginning of the sentence.

Sorry, I meant at the beginning of the following sentences, not in the middle.

https://codereview.chromium.org/1452603002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:50:
// This must be called precisely once per query.
Assert that the |mResult| is null then?

(I know that asserts aren't actually working, but we can still use them for
informative purposes, and then rewrite them to something else later.)

https://codereview.chromium.org/1452603002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:95:
static class SupervisedUserInsertReply {
You could make this a generic class and use it for the other reply as well (with
a type parameter of Boolean or Pair<Boolean, String>, respectively).

https://codereview.chromium.org/1452603002/diff/200001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java
(right):

https://codereview.chromium.org/1452603002/diff/200001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java:107:
// on the main thread.
We could actually make this single-threaded now. We'd just have to modify the
shadow Handler to immediately run its Runnable, then the mocks would put a value
into the query reply (non-blocking), so we could then read it out again without
blocking either. We could even assert that the CountdownLatch is at zero before
trying to read the result out, so if this test should regress, it would fail
straight away instead of deadlocking and timing out.

https://codereview.chromium.org/1452603002/diff/200001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_content_provider_android.cc
(right):

https://codereview.chromium.org/1452603002/diff/200001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.cc:106:
ScopedJavaGlobalRef<jobject>* query_reply_jobj,
You can just copy a ScopedJavaGlobalRef.

https://codereview.chromium.org/1452603002/diff/200001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.cc:136:
ScopedJavaGlobalRef<jobject>* insert_reply_jobj,
This would leak (but as mentioned above, you can just copy ScopedJavaGlobalRef).

[aberent, 2015-12-17 14:59:09]

https://codereview.chromium.org/1452603002/diff/180001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_content_provider_android.h
(right):

https://codereview.chromium.org/1452603002/diff/180001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.h:38:
void OnInsertRequestSendComplete(bool sentOk);
On 2015/12/17 11:13:21, Bernhard Bauer wrote:
> On 2015/12/16 22:15:17, aberent wrote:
> > On 2015/12/16 17:52:09, Bernhard Bauer wrote:
> > > Nit: |sent_ok| for native code.
> > 
> > Done.
> 
> Not quite 😃

Done.

https://codereview.chromium.org/1452603002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
(right):

https://codereview.chromium.org/1452603002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:50:
// This must be called precisely once per query.
On 2015/12/17 11:13:21, Bernhard Bauer wrote:
> Assert that the |mResult| is null then?
> 
> (I know that asserts aren't actually working, but we can still use them for
> informative purposes, and then rewrite them to something else later.)

Done. Also inserted some other asserts.

https://codereview.chromium.org/1452603002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java:95:
static class SupervisedUserInsertReply {
On 2015/12/17 11:13:21, Bernhard Bauer wrote:
> You could make this a generic class and use it for the other reply as well
(with
> a type parameter of Boolean or Pair<Boolean, String>, respectively).

I considered this. The problem is that the two onQueryComplete methods are
different. There would be various ways of working round this, but none of them
seem nice to me.

https://codereview.chromium.org/1452603002/diff/200001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java
(right):

https://codereview.chromium.org/1452603002/diff/200001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java:107:
// on the main thread.
On 2015/12/17 11:13:21, Bernhard Bauer wrote:
> We could actually make this single-threaded now. We'd just have to modify the
> shadow Handler to immediately run its Runnable, then the mocks would put a
value
> into the query reply (non-blocking), so we could then read it out again
without
> blocking either. We could even assert that the CountdownLatch is at zero
before
> trying to read the result out, so if this test should regress, it would fail
> straight away instead of deadlocking and timing out.

Doesn't this just move the complexity around? Anyway I have now put "not on UI
thread" asserts into the code. I think those would fail if we did this (once we
have working asserts).

https://codereview.chromium.org/1452603002/diff/200001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_content_provider_android.cc
(right):

https://codereview.chromium.org/1452603002/diff/200001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.cc:106:
ScopedJavaGlobalRef<jobject>* query_reply_jobj,
On 2015/12/17 11:13:21, Bernhard Bauer wrote:
> You can just copy a ScopedJavaGlobalRef.

Done. As discussed.

https://codereview.chromium.org/1452603002/diff/200001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_content_provider_android.cc:136:
ScopedJavaGlobalRef<jobject>* insert_reply_jobj,
On 2015/12/17 11:13:21, Bernhard Bauer wrote:
> This would leak (but as mentioned above, you can just copy
ScopedJavaGlobalRef).

Done.

[Bernhard Bauer, 2015-12-17 15:18:51]

LGTM

https://codereview.chromium.org/1452603002/diff/200001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java
(right):

https://codereview.chromium.org/1452603002/diff/200001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java:107:
// on the main thread.
On 2015/12/17 14:59:08, aberent wrote:
> On 2015/12/17 11:13:21, Bernhard Bauer wrote:
> > We could actually make this single-threaded now. We'd just have to modify
the
> > shadow Handler to immediately run its Runnable, then the mocks would put a
> value
> > into the query reply (non-blocking), so we could then read it out again
> without
> > blocking either. We could even assert that the CountdownLatch is at zero
> before
> > trying to read the result out, so if this test should regress, it would fail
> > straight away instead of deadlocking and timing out.
> 
> Doesn't this just move the complexity around? Anyway I have now put "not on UI
> thread" asserts into the code. I think those would fail if we did this (once
we
> have working asserts).

Well, it would remove the complexity of having to deal with Real Threads(tm) in
an environment that isn't really set up to deal with them (as you point out in
the comment). But I don't really feel strongly about it, just thought it might
be nice to get rid of the threading code.

[aberent, 2015-12-17 15:28:39]

The CQ bit was checked by aberent@chromium.org

[aberent, 2015-12-17 15:28:40]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1452603002/#ps220001
(title: "Respond to more comments")

[commit-bot: I haz the power, 2015-12-17 15:28:58]

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

[commit-bot: I haz the power, 2015-12-17 16:12:34]

Description was changed from

==========
Supervised user web restrictions content provider

The web restrictions content provider lets other apps query
web restrictions. In particular it lets apps using WebView
query Chrome's supervised user web restrictions.

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

to

==========
Supervised user web restrictions content provider

The web restrictions content provider lets other apps query
web restrictions. In particular it lets apps using WebView
query Chrome's supervised user web restrictions.

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

[commit-bot: I haz the power, 2015-12-17 16:12:35]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-12-17 16:13:22]

Description was changed from

==========
Supervised user web restrictions content provider

The web restrictions content provider lets other apps query
web restrictions. In particular it lets apps using WebView
query Chrome's supervised user web restrictions.

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

to

==========
Supervised user web restrictions content provider

The web restrictions content provider lets other apps query
web restrictions. In particular it lets apps using WebView
query Chrome's supervised user web restrictions.

BUG=569879

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

[commit-bot: I haz the power, 2015-12-17 16:13:23]

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

[jam, 2015-12-17 19:57:16]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/1539653002/ by jam@chromium.org.

The reason for reverting is: The new test is failing on CQ

BUG=570768.