Factor DocumentSubresourceFilter out of SubresourceFilterAgent.

components/subresource_filter/content/renderer/document_subresource_filter.h

Index: components/subresource_filter/content/renderer/document_subresource_filter.h
diff --git a/components/subresource_filter/content/renderer/document_subresource_filter.h b/components/subresource_filter/content/renderer/document_subresource_filter.h
new file mode 100644
index 0000000000000000000000000000000000000000..c53c8761368a7a9b379695e7633b50027e6e082d
--- /dev/null
+++ b/components/subresource_filter/content/renderer/document_subresource_filter.h
@@ -0,0 +1,68 @@
+// Copyright 2016 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_SUBRESOURCE_FILTER_CONTENT_RENDERER_DOCUMENT_SUBRESOURCE_FILTER_H_
+#define COMPONENTS_SUBRESOURCE_FILTER_CONTENT_RENDERER_DOCUMENT_SUBRESOURCE_FILTER_H_
+
+#include <stddef.h>
+
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "components/subresource_filter/core/common/activation_state.h"
+#include "third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h"
+
+class GURL;
+
+namespace subresource_filter {
+
+class MemoryMappedRuleset;
+
+// Performs filtering of subresource loads in the scope of a given document.
+//
+// TODO(engedy): This is a placeholder implementation that treats the entire
+// ruleset as the UTF-8 representation of a string, and will disallow
+// subresource loads from URL paths having this string as a suffix if it is
+// non-empty. This enables exercising the feature end-to-end in a browsertest.
+// Replace this with the actual filtering logic.
+class DocumentSubresourceFilter : public blink::WebDocumentSubresourceFilter {

[pkalinnikov, 2016/07/19 11:57:32]

Do you think it makes sense to call this TestDocumentSubresourceFilter, to be
consistent with TestRulesetCreator?
Even when we replace the logic, you will still need the simple logic for
testing, right?

[engedy, 2016/07/19 12:57:29]

I'd really like to have and use only a single implementation of
DocumentSubresourceFilter everywhere, with a simple ruleset in tests that is
created using the TestRulesetCreator.

The reason for this is that I current don't see an easy way to inject a testing
filter implementation in some tests, e.g., in multi-process browser_tests where
the SubresourceFilterAgent lives in a different process than where the main body
of the test executes (basically we would need IPC messages just for testing).
Even in single-process tests we would need to create/inject factories just for
this purpose.

Therefore it seems simpler to create a convenient method to define simple
rulesets to be used with the production filter implementation, which is what the
TestRulesetCreator does.

[pkalinnikov, 2016/07/19 13:45:19]

Acknowledged.

+ public:
+  // Constructs a new filter that will:
+  //  -- Operate at the prescribed |activation_state|, which must be either
+  //     ActivationState::DRYRUN or ActivationState::ENABLED. In the former
+  //     case filtering will be performed but no loads will be disallowed.
+  //  -- Hold a reference to and use |ruleset| for its entire lifetime.
+  //  -- Expect |ancestor_document_urls| to be the URLs of documents loaded into
+  //     nested frames, starting with the current frame and ending with the main
+  //     frame. This provides the context for evaluating domain-specific rules.
+  DocumentSubresourceFilter(ActivationState activation_state,
+                            const scoped_refptr<MemoryMappedRuleset>& ruleset,
+                            const std::vector<GURL>& ancestor_document_urls);

[pkalinnikov, 2016/07/19 11:57:33]

Would it be better to pass the vector by value? To make it possible std::moving
to the constructor.

[engedy, 2016/07/19 12:57:28]

Hmm, yes, it would be a temporary list in most cases anyway. Done.

+  ~DocumentSubresourceFilter() override;
+
+  // blink::WebDocumentSubresourceFilter:

[pkalinnikov, 2016/07/19 11:57:32]

Does colon at the end of the comment mean something?

[engedy, 2016/07/19 12:57:29]

This is the most widely used way of indicating that the following lines are the
inherited methods from that interface. Some folks do omit the colon, though, but
using it is the most common. For example:

https://cs.chromium.org/search/?q=%5C/%5C/%5C+content::WebContentsObserver$&s...
https://cs.chromium.org/search/?q=%5C/%5C/%5C+content::WebContentsObserver%5C...

[pkalinnikov, 2016/07/19 13:45:19]

Acknowledged.

+  bool allowLoad(const blink::WebURL& resourceUrl,
+                 blink::WebURLRequest::RequestContext) override;
+
+  size_t num_loads_evaluated() const { return num_loads_evaluated_; }

[pkalinnikov, 2016/07/19 11:57:33]

Maybe add a comment that these 3 are monitoring counters?

I was also thinking whether this is the right place for counters. We'll have
another DocumentSubresourceFilter (non-testing) with the same counters, right?

[engedy, 2016/07/19 12:57:29]

Added comments.

If at all possible, I'd like have only a single implementation, see my
explanation above.

+  size_t num_loads_matching_rules() const { return num_loads_matching_rules_; }
+  size_t num_loads_disallowed() const { return num_loads_disallowed_; }
+
+ private:
+  bool DoesLoadMatchFilteringRules(const GURL& resource_url);
+
+  ActivationState activation_state_;
+  scoped_refptr<MemoryMappedRuleset> ruleset_;

[pkalinnikov, 2016/07/19 11:57:32]

What do you think about making this scoped_refptr<const MemoryMappedRuleset>
here and everywhere else?

[engedy, 2016/07/19 12:57:29]

Done.

+
+  size_t num_loads_evaluated_ = 0;
+  size_t num_loads_matching_rules_ = 0;
+  size_t num_loads_disallowed_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(DocumentSubresourceFilter);
+};
+
+}  // namespace subresource_filter
+
+#endif  // COMPONENTS_SUBRESOURCE_FILTER_CONTENT_RENDERER_DOCUMENT_SUBRESOURCE_FILTER_H_