Web restrictions component only.

components/web_restriction/content_resolver_web_restriction_provider.h

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_WEB_RESTRICTION_CONTENT_RESOLVER_WEB_RESTRICTION_PROVIDER_H_
+#define COMPONENTS_WEB_RESTRICTION_CONTENT_RESOLVER_WEB_RESTRICTION_PROVIDER_H_
+
+#include <jni.h>
+#include <list>
+#include <map>
+
+#include "base/android/jni_android.h"
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/task_runner.h"
+#include "components/web_restriction/web_restriction_provider.h"
+
+namespace web_restriction {
+
+class ContentResolverWebRestrictionProvider;
+
+namespace android {
+
+// A wrapper to the callback class to facilitate getting a callback from java
+// into C++. Objects of this class delete itself only when they are called back
+// so we must ensure that happens even for error cases.
+class SelfDeletingCallback {

[Bernhard Bauer, 2016/01/26 11:09:03]

Why is this in the header? We should be able to move it to the implementation
file into an anonymous namespace.

[knn, 2016/01/26 14:37:43]

We need this class to be declared before the jni header include in the cc file.
It is either this or have a separate h/cc file for the callback which in turn
doesn't make sense as it is very tightly coupled with the provider class and we
will have include ordering issues.

[Bernhard Bauer, 2016/01/27 14:24:01]

Hm. What we really need is just a declaration of the JNI method in the header,
and that could live in the ContentResolverWebRestrictionProvider class and
forward to the callback. Basically, I'd like to avoid having this class itself
in the header, because it shows up in the namespace for anyone who includes this
header.

[knn, 2016/01/28 11:43:25]

Resolved the issue by casting to the callback myself instead of letting JNI do
it.

+ public:
+  SelfDeletingCallback(const GURL& url,
+                       const base::Callback<void(bool)>& callback,
+                       const scoped_refptr<base::TaskRunner>& callback_runner,
+                       ContentResolverWebRestrictionProvider* provider);
+
+  void RequestSuccess(JNIEnv* env,
+                      const base::android::JavaParamRef<jobject>& obj,
+                      jboolean request_success);
+  void ShouldProceed(JNIEnv* env,
+                     const base::android::JavaParamRef<jobject>& obj,
+                     jboolean should_proceed,
+                     const base::android::JavaParamRef<jstring>& error_page);
+
+ private:
+  GURL url_;
+  base::Callback<void(bool)> callback_;
+  scoped_refptr<base::TaskRunner> callback_runner_;
+  ContentResolverWebRestrictionProvider* provider_;
+
+  // Only the callback can delete itself. We must ensure it is indeed
+  // called back.
+  ~SelfDeletingCallback();

[Bernhard Bauer, 2016/01/26 11:09:03]

Methods come before members.

[knn, 2016/01/26 14:37:43]

But this is a destructor and seems appropriate to put it with the constructor
below which in turn always shows up in the end.

[Bernhard Bauer, 2016/01/27 14:24:01]

The macro below technically defines a constructor, but conceptually it is just a
declaration that this class does not have a copy constructor 😃 The style guide
does say that methods come before members, and it has an explicit exception for
DISALLOW_COPY_AND_ASSIGN
(https://google.github.io/styleguide/cppguide.html#Declaration_Order).

[knn, 2016/01/28 11:43:25]

Done.

+  DISALLOW_COPY_AND_ASSIGN(SelfDeletingCallback);
+};
+
+}  // namespace android
+
+class ContentResolverWebRestrictionProvider : public WebRestrictionProvider {

[Bernhard Bauer, 2016/01/27 14:24:01]

Why is this not in the android namespace, BTW?

[knn, 2016/01/28 11:43:25]

web_restrictions is implicitly android at the moment, got rid of the android
namespace for the callback.

+ public:
+  ContentResolverWebRestrictionProvider();
+  ~ContentResolverWebRestrictionProvider() override;
+
+  // Register JNI methods.
+  static bool Register(JNIEnv* env);
+
+  // Verify the content provider and Setup.

[Bernhard Bauer, 2016/01/26 11:09:03]

Does this method verify the setup (i.e. is "setup" a noun here)? If not, and it
sets [something] up, split it into two words. Also, no need to capitalize.

[knn, 2016/01/26 14:37:44]

Done.

+  void Initialize(const std::string& content_provider_authority);
+
+  // WebRestrictionProvider:
+  UrlAccess ShouldProceed(bool is_main_frame,
+                          const GURL& url,
+                          const base::Callback<void(bool)>& callback) override;
+
+  bool SupportsRequest() const override;
+
+  bool GetErrorHtml(const GURL& url, std::string* error_html) const override;

[Bernhard Bauer, 2016/01/26 11:09:03]

Identifying the error page by its URL isn't particularly pretty (for example,
this implicitly contains the assumption that the error page would only depend on
the URL). Is there some other identifier that we have when creating the throttle
that we could use instead?

[knn, 2016/01/26 14:37:44]

That is the only information available to the provider right now (other than
intrinsic factors like time I guess). Do you foresee major issues with this?

+
+  void RequestPermission(const GURL& url,
+                         const base::Callback<void(bool)>& callback) override;
+
+ private:
+  friend class android::SelfDeletingCallback;  // For updating the cache.
+
+  bool initialized_;
+  // Setup during Initialize.

[Bernhard Bauer, 2016/01/26 11:09:03]

Also, "Set up [...]".

[knn, 2016/01/26 14:37:43]

Done.

+  bool supports_request_;
+  base::android::ScopedJavaGlobalRef<jobject> java_provider_;
+
+  std::map<GURL, std::string> error_page_cache_;
+  std::map<GURL, bool> url_access_cache_;

[Bernhard Bauer, 2016/01/26 11:09:03]

How are we dealing with cache invalidation here? Also, you could use
base/containers/mru_cache.h for this.

[knn, 2016/01/26 14:37:43]

We aren't. I was banking on a webview app's lifetime being limited usually.
Reason for not using MRUCache was we want to cache the 2 values in sync which
leads to a MRUCache<GURL, pair<bool, string>> construct with special casing for
string.empty() -> absent. This just seemed cleaner to me, any thoughts?

[Bernhard Bauer, 2016/01/27 14:24:01]

Hm, I could see various instances where that isn't the case, and then we get bug
reports filed because OMG the filter doesn't work 😕

Do we support listening for changes to the filter? ContentProvider does offer
that. We could also land this without caching as a first step, and then add
caching when we have figured out how to invalidate it (e.g. by listening for
changes).
Could we just use two MRU caches?

[knn, 2016/01/28 11:43:25]

Clearing the cache on update now. Hope the cache helper methods look good.

+  std::list<GURL> recent_urls_;
+
+  DISALLOW_COPY_AND_ASSIGN(ContentResolverWebRestrictionProvider);
+};
+
+}  // namespace web_restriction
+
+#endif  // COMPONENTS_WEB_RESTRICTION_CONTENT_RESOLVER_WEB_RESTRICTION_PROVIDER_H_