Integrate IndexedRuleset into DocumentSubresourceFilter.

components/subresource_filter/content/renderer/document_subresource_filter.cc

Index: components/subresource_filter/content/renderer/document_subresource_filter.cc
diff --git a/components/subresource_filter/content/renderer/document_subresource_filter.cc b/components/subresource_filter/content/renderer/document_subresource_filter.cc
index f1e4fcebce86070a5773899b2e311ad1a8191a18..a8e0c78645e32878ce15c947d4a46cf0ffd78659 100644
--- a/components/subresource_filter/content/renderer/document_subresource_filter.cc
+++ b/components/subresource_filter/content/renderer/document_subresource_filter.cc
@@ -12,15 +12,53 @@
 #include "base/trace_event/trace_event.h"
 #include "components/subresource_filter/core/common/memory_mapped_ruleset.h"
 #include "third_party/WebKit/public/platform/WebURL.h"
-#include "url/gurl.h"
 
 namespace subresource_filter {
 
+namespace {
+
+proto::ElementType ToElementType(

[pkalinnikov, 2016/07/22 13:20:03]

Please review this conversion carefully.

+    blink::WebURLRequest::RequestContext request_context) {
+  switch (request_context) {
+    case blink::WebURLRequest::RequestContextAudio:
+    case blink::WebURLRequest::RequestContextVideo:

[engedy, 2016/07/22 14:19:58]

Also Track

[pkalinnikov, 2016/07/25 11:41:11]

Done.

+      return proto::ELEMENT_TYPE_MEDIA;
+    case blink::WebURLRequest::RequestContextFont:
+      return proto::ELEMENT_TYPE_FONT;
+    case blink::WebURLRequest::RequestContextFrame:
+    case blink::WebURLRequest::RequestContextIframe:
+      return proto::ELEMENT_TYPE_SUBDOCUMENT;
+    case blink::WebURLRequest::RequestContextImage:

[engedy, 2016/07/22 14:19:58]

Also ImageSet

[pkalinnikov, 2016/07/25 11:41:11]

Done.

+      return proto::ELEMENT_TYPE_IMAGE;
+    case blink::WebURLRequest::RequestContextObject:
+    case blink::WebURLRequest::RequestContextPlugin:

[engedy, 2016/07/22 14:19:58]

According to web_url_request_util.cc:68, Embed and Object correspond to OBJECT,
and Plugin to OBJECT_SUBREQUEST. This contradicts my understanding. We should
verify this.

[pkalinnikov, 2016/07/25 11:41:11]

Done.

+      return proto::ELEMENT_TYPE_OBJECT;
+    case blink::WebURLRequest::RequestContextPing:

[engedy, 2016/07/22 14:19:58]

Also Beacon.

[pkalinnikov, 2016/07/25 11:41:11]

Done.

+      return proto::ELEMENT_TYPE_PING;
+    case blink::WebURLRequest::RequestContextScript:
+      return proto::ELEMENT_TYPE_SCRIPT;
+    case blink::WebURLRequest::RequestContextStyle:
+      return proto::ELEMENT_TYPE_STYLESHEET;

[engedy, 2016/07/22 14:19:58]

Let's classify XSLT as a stylesheet too.

[pkalinnikov, 2016/07/25 11:41:11]

Done.

+    case blink::WebURLRequest::RequestContextXMLHttpRequest:

[engedy, 2016/07/22 14:19:58]

EventSource and Fetch should probably be considered XHR too.

[pkalinnikov, 2016/07/25 11:41:11]

Done.

+      return proto::ELEMENT_TYPE_XMLHTTPREQUEST;
+
+    case blink::WebURLRequest::RequestContextUnspecified:
+      return proto::ELEMENT_TYPE_UNSPECIFIED;
+    default:
+      return proto::ELEMENT_TYPE_OTHER;
+  }

[engedy, 2016/07/22 14:19:58]

We'll have to carefully look into CSPReport, Favicon, Prefetch, Download,
Manifest, Subresource. Potentially, we won't even consult the filter for these.

Then Form, Hyperlink, Location and Internal correspond to various navigation
types of main document loads into frames, so we will have to look at them too.

+}
+
+}  // namespace
+
 DocumentSubresourceFilter::DocumentSubresourceFilter(
     ActivationState activation_state,
     const scoped_refptr<const MemoryMappedRuleset>& ruleset,
-    std::vector<GURL> /* ignored_ancestor_document_urls */)
-    : activation_state_(activation_state), ruleset_(ruleset) {
+    std::vector<GURL> ancestor_document_urls)
+    : activation_state_(activation_state),
+      ruleset_(ruleset),
+      matcher_(ruleset_->data(), ruleset_->length()),
+      ancestor_document_urls_(std::move(ancestor_document_urls)) {
   DCHECK_NE(activation_state_, ActivationState::DISABLED);
   DCHECK(ruleset);
 }
@@ -29,11 +67,20 @@ DocumentSubresourceFilter::~DocumentSubresourceFilter() = default;
 
 bool DocumentSubresourceFilter::allowLoad(
     const blink::WebURL& resourceUrl,
-    blink::WebURLRequest::RequestContext /* ignored */) {
+    blink::WebURLRequest::RequestContext request_context) {
   TRACE_EVENT1("loader", "DocumentSubresourceFilter::allowLoad", "url",
                resourceUrl.string().utf8());
+
+  // TODO(pkalinnikov): Check all |ancestor_document_urls| for activation bit.

[pkalinnikov, 2016/07/22 13:20:03]

Or check |ancestor_document_rules| once during construction and remember the
activation bit. It depends on whether this bit should be computed with the
request in account.

[pkalinnikov, 2016/07/25 11:47:31]

Added comment.

   ++num_loads_evaluated_;
-  if (DoesLoadMatchFilteringRules(GURL(resourceUrl))) {
+
+  url::Origin initiator;
+  if (!ancestor_document_urls_.empty())
+    initiator = url::Origin(ancestor_document_urls_.front());
+  const bool load_is_allowed = matcher_.IsAllowed(

[engedy, 2016/07/22 14:19:58]

nit: How about just inlining this?

[pkalinnikov, 2016/07/25 11:41:11]

Done.

+      GURL(resourceUrl), initiator, ToElementType(request_context));

[engedy, 2016/07/22 14:19:58]

Either need to handle invalid URLs here, or verify that call sites provide a
valid URL + add comment and a DCHECK.

[pkalinnikov, 2016/07/25 11:47:31]

The |matcher_| already takes care of invalid GURLs.

[engedy, 2016/07/25 14:06:38]

Yes, it has a DCHECK, but then we still need to verify that the DCHECK codifies
a precondition that is actually met. As a third option, we can remove the DCHECK
from there, and declare that the method is completely happy to filter invalid
URLs.

[pkalinnikov, 2016/07/25 17:34:54]

Wait, I'm talking about IndexedRulesetMatcher::IsAllowed. It has no a DCHECK,
the first 2 lines are:

  if (!url.is_valid())
    return true;

[engedy, 2016/07/25 19:47:00]

Oh, right, I didn't noticed that. Ack.

+
+  if (!load_is_allowed) {
     ++num_loads_matching_rules_;
     if (activation_state_ == ActivationState::ENABLED) {
       ++num_loads_disallowed_;
@@ -43,14 +90,4 @@ bool DocumentSubresourceFilter::allowLoad(
   return true;
 }
 
-bool DocumentSubresourceFilter::DoesLoadMatchFilteringRules(
-    const GURL& resource_url) {
-  static_assert(CHAR_BIT == 8, "Assumed char was 8 bits.");
-  base::StringPiece disallowed_suffix = base::StringPiece(
-      reinterpret_cast<const char*>(ruleset_->data()), ruleset_->length());
-  return !disallowed_suffix.empty() &&
-         base::EndsWith(resource_url.path_piece(), disallowed_suffix,
-                        base::CompareCase::SENSITIVE);
-}
-
 }  // namespace subresource_filter