Factor DocumentSubresourceFilter out of SubresourceFilterAgent.

Factor DocumentSubresourceFilter out of SubresourceFilterAgent.

Furthermore, add proper unit tests for both classes, and make the filter keep
count of the outcomes of evaluations, which will be used for histograms in a
later CL.

BUG=609747
Committed: https://crrev.com/4424f552b1a5fdb8471b46c2d351c694908d85da
Cr-Commit-Position: refs/heads/master@{#406536}

[engedy, 2016-07-18 12:16:54]

Description was changed from

==========
Factor DocumentSubresourceFilter out of SubresourceFilterAgent.

Furthermore, add proper unit tests for both classes, and make the filter count
the outcomes of evaluations.

BUG=609747
==========

to

==========
Factor DocumentSubresourceFilter out of SubresourceFilterAgent.

Furthermore, add proper unit tests for both classes, and make the filter count 
the outcomes of evaluations, which will be used for histograms in a later CL.

BUG=609747
==========

[engedy, 2016-07-18 12:16:54]

engedy@chromium.org changed reviewers:
+ melandory@chromium.org, pkalinnikov@chromium.org

[engedy, 2016-07-18 13:10:05]

Patchset #1 (id:1) has been deleted

[engedy, 2016-07-18 13:12:10]

Pavel, could you please take a first look?

[engedy, 2016-07-18 13:17:59]

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

[commit-bot: I haz the power, 2016-07-18 13:18:10]

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-07-18 13:21:53]

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

[commit-bot: I haz the power, 2016-07-18 13:21:53]

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...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[engedy, 2016-07-18 13:46:06]

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

[commit-bot: I haz the power, 2016-07-18 13:46:34]

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-07-18 13:58:37]

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

[commit-bot: I haz the power, 2016-07-18 13:58:38]

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

[engedy, 2016-07-18 14:13:12]

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

[commit-bot: I haz the power, 2016-07-18 14:13:30]

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-07-18 14:46:17]

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

[commit-bot: I haz the power, 2016-07-18 14:46:18]

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

[engedy, 2016-07-19 08:55:39]

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

[commit-bot: I haz the power, 2016-07-19 08:56:00]

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-07-19 10:51:12]

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

[commit-bot: I haz the power, 2016-07-19 10:51:13]

Dry run: This issue passed the CQ dry run.

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

LGTM % comments.

https://codereview.chromium.org/2158833002/diff/120001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2158833002/diff/120001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:517: new
subresource_filter::SubresourceFilterAgent(
How does this object not leak?

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
File
components/subresource_filter/content/renderer/document_subresource_filter.cc
(right):

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.cc:27:
DocumentSubresourceFilter::~DocumentSubresourceFilter() {}
= default;

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.cc:49:
base::EndsWith(resource_url.path(), disallowed_suffix,
s/path/path_piece/, no need for allocating std::string.

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
File
components/subresource_filter/content/renderer/document_subresource_filter.h
(right):

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.h:30:
class DocumentSubresourceFilter : public blink::WebDocumentSubresourceFilter {
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?

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.h:42:
const std::vector<GURL>& ancestor_document_urls);
Would it be better to pass the vector by value? To make it possible std::moving
to the constructor.

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.h:45:
// blink::WebDocumentSubresourceFilter:
Does colon at the end of the comment mean something?

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.h:49:
size_t num_loads_evaluated() const { return num_loads_evaluated_; }
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?

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.h:57:
scoped_refptr<MemoryMappedRuleset> ruleset_;
What do you think about making this scoped_refptr<const MemoryMappedRuleset>
here and everywhere else?

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
File
components/subresource_filter/content/renderer/document_subresource_filter_unittest.cc
(right):

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter_unittest.cc:41:
void SetTestRulesetToDisallowURLsWithPathSuffix(const std::string& suffix) {
base::StringPiece

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter_unittest.cc:84:
SetTestRulesetToDisallowURLsWithPathSuffix(std::string()));
base::StringPiece()

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
File components/subresource_filter/content/renderer/subresource_filter_agent.cc
(right):

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/subresource_filter_agent.cc:70:
ancestor_document_urls));
std::move(ancestor_document_urls) or inline GetAncestorDocumentURLs()

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
File
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc
(right):

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc:41:
~SubresourceFilterAgentUnderTest() {}
= default?

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
File components/subresource_filter/core/common/test_ruleset_creator.h (right):

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/core/common/test_ruleset_creator.h:33: void
CreateRulesetToDisallowURLsWithPathSuffix(const std::string& suffix,
How about base::StringPiece?

[engedy, 2016-07-19 12:52:46]

Description was changed from

==========
Factor DocumentSubresourceFilter out of SubresourceFilterAgent.

Furthermore, add proper unit tests for both classes, and make the filter count 
the outcomes of evaluations, which will be used for histograms in a later CL.

BUG=609747
==========

to

==========
Factor DocumentSubresourceFilter out of SubresourceFilterAgent.

Furthermore, add proper unit tests for both classes, and make the filter keep 
count of the outcomes of evaluations, which will be used for histograms in a
later CL.

BUG=609747
==========

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

https://codereview.chromium.org/2158833002/diff/120001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2158833002/diff/120001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:517: new
subresource_filter::SubresourceFilterAgent(
On 2016/07/19 11:57:32, pkalinnikov wrote:
> How does this object not leak?

As a RenderFrameObserver it is owned by the |render_frame|, which takes care of
destruction.

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
File
components/subresource_filter/content/renderer/document_subresource_filter.cc
(right):

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.cc:27:
DocumentSubresourceFilter::~DocumentSubresourceFilter() {}
On 2016/07/19 11:57:32, pkalinnikov wrote:
> = default;

Done.

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.cc:49:
base::EndsWith(resource_url.path(), disallowed_suffix,
On 2016/07/19 11:57:32, pkalinnikov wrote:
> s/path/path_piece/, no need for allocating std::string.

Nice. Done.

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
File
components/subresource_filter/content/renderer/document_subresource_filter.h
(right):

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.h:30:
class DocumentSubresourceFilter : public blink::WebDocumentSubresourceFilter {
On 2016/07/19 11:57:32, pkalinnikov wrote:
> 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?

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.

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.h:42:
const std::vector<GURL>& ancestor_document_urls);
On 2016/07/19 11:57:33, pkalinnikov wrote:
> Would it be better to pass the vector by value? To make it possible
std::moving
> to the constructor.

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

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.h:45:
// blink::WebDocumentSubresourceFilter:
On 2016/07/19 11:57:32, pkalinnikov wrote:
> Does colon at the end of the comment mean something?

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...

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.h:49:
size_t num_loads_evaluated() const { return num_loads_evaluated_; }
On 2016/07/19 11:57:33, pkalinnikov wrote:
> 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?

Added comments.

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

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.h:57:
scoped_refptr<MemoryMappedRuleset> ruleset_;
On 2016/07/19 11:57:32, pkalinnikov wrote:
> What do you think about making this scoped_refptr<const MemoryMappedRuleset>
> here and everywhere else?

Done.

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
File
components/subresource_filter/content/renderer/document_subresource_filter_unittest.cc
(right):

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter_unittest.cc:41:
void SetTestRulesetToDisallowURLsWithPathSuffix(const std::string& suffix) {
On 2016/07/19 11:57:33, pkalinnikov wrote:
> base::StringPiece

Done everywhere.

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter_unittest.cc:84:
SetTestRulesetToDisallowURLsWithPathSuffix(std::string()));
On 2016/07/19 11:57:33, pkalinnikov wrote:
> base::StringPiece()

Done.

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
File components/subresource_filter/content/renderer/subresource_filter_agent.cc
(right):

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/subresource_filter_agent.cc:70:
ancestor_document_urls));
On 2016/07/19 11:57:33, pkalinnikov wrote:
> std::move(ancestor_document_urls) or inline GetAncestorDocumentURLs()

Done.

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
File
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc
(right):

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc:41:
~SubresourceFilterAgentUnderTest() {}
On 2016/07/19 11:57:33, pkalinnikov wrote:
> = default?

Done.

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
File components/subresource_filter/core/common/test_ruleset_creator.h (right):

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/core/common/test_ruleset_creator.h:33: void
CreateRulesetToDisallowURLsWithPathSuffix(const std::string& suffix,
On 2016/07/19 11:57:33, pkalinnikov wrote:
> How about base::StringPiece?

Done.

[engedy, 2016-07-19 13:09:27]

engedy@chromium.org changed reviewers:
+ caitkp@chromium.org, mkwst@chromium.org, thestig@chromium.org

[engedy, 2016-07-19 13:09:28]

Dear OWNERs, could you please review:

@Tanja --> components/subresource_filter/
@Mike  --> components/test_runner/ (used for blink::WebString and co.)
@Cait  --> components/BUILD.gn
@Lei   --> chrome/renderer/chrome_content_renderer_client.cc

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

Still LGTM.

https://codereview.chromium.org/2158833002/diff/120001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2158833002/diff/120001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:517: new
subresource_filter::SubresourceFilterAgent(
On 2016/07/19 12:57:28, engedy wrote:
> On 2016/07/19 11:57:32, pkalinnikov wrote:
> > How does this object not leak?
> 
> As a RenderFrameObserver it is owned by the |render_frame|, which takes care
of
> destruction.

Acknowledged.

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
File
components/subresource_filter/content/renderer/document_subresource_filter.h
(right):

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.h:30:
class DocumentSubresourceFilter : public blink::WebDocumentSubresourceFilter {
On 2016/07/19 12:57:29, engedy wrote:
> On 2016/07/19 11:57:32, pkalinnikov wrote:
> > 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?
> 
> 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.

Acknowledged.

https://codereview.chromium.org/2158833002/diff/120001/components/subresource...
components/subresource_filter/content/renderer/document_subresource_filter.h:45:
// blink::WebDocumentSubresourceFilter:
On 2016/07/19 12:57:29, engedy wrote:
> On 2016/07/19 11:57:32, pkalinnikov wrote:
> > Does colon at the end of the comment mean something?
> 
> 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...

Acknowledged.

[Mike West, 2016-07-19 13:45:57]

Using //components/test_runner for unit test LGTM.

[Cait (Slow), 2016-07-19 18:26:03]

lgtm

[melandory, 2016-07-20 08:39:56]

On 2016/07/19 18:26:03, Cait (slow) wrote:
> lgtm

lgtm

[Lei Zhang, 2016-07-20 09:34:29]

lgtm, did not forget about this CL.

[engedy, 2016-07-20 09:41:48]

On 2016/07/20 09:34:29, Lei Zhang wrote:
> lgtm, did not forget about this CL.

Wow, thanks! :-)

[engedy, 2016-07-20 09:45:17]

To clarify, the `wow` part meaning that "I sincerely hope that it is just your
calendar being out of date and you are not actually in PDT."

[engedy, 2016-07-20 09:45:27]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2016-07-20 09:45:54]

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-07-20 13:00:59]

Description was changed from

==========
Factor DocumentSubresourceFilter out of SubresourceFilterAgent.

Furthermore, add proper unit tests for both classes, and make the filter keep 
count of the outcomes of evaluations, which will be used for histograms in a
later CL.

BUG=609747
==========

to

==========
Factor DocumentSubresourceFilter out of SubresourceFilterAgent.

Furthermore, add proper unit tests for both classes, and make the filter keep 
count of the outcomes of evaluations, which will be used for histograms in a
later CL.

BUG=609747
==========

[commit-bot: I haz the power, 2016-07-20 13:01:01]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-07-20 13:03:35]

Description was changed from

==========
Factor DocumentSubresourceFilter out of SubresourceFilterAgent.

Furthermore, add proper unit tests for both classes, and make the filter keep 
count of the outcomes of evaluations, which will be used for histograms in a
later CL.

BUG=609747
==========

to

==========
Factor DocumentSubresourceFilter out of SubresourceFilterAgent.

Furthermore, add proper unit tests for both classes, and make the filter keep
count of the outcomes of evaluations, which will be used for histograms in a
later CL.

BUG=609747

Committed: https://crrev.com/4424f552b1a5fdb8471b46c2d351c694908d85da
Cr-Commit-Position: refs/heads/master@{#406536}
==========

[commit-bot: I haz the power, 2016-07-20 13:03:37]

Patchset 7 (id:??) landed as
https://crrev.com/4424f552b1a5fdb8471b46c2d351c694908d85da
Cr-Commit-Position: refs/heads/master@{#406536}