Implement GENERICBLOCK option.

Implement GENERICBLOCK option.

BUG=609747
Committed: https://crrev.com/7c855c7419981aaac5af823d7df2665af9e68bae
Cr-Commit-Position: refs/heads/master@{#426780}

[pkalinnikov, 2016-10-20 14:26:00]

Balazs, please take a first look.

[pkalinnikov, 2016-10-20 14:26:08]

Description was changed from

==========
Implement GENERICBLOCK option.

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

to

==========
Implement GENERICBLOCK option.

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

[pkalinnikov, 2016-10-20 14:26:08]

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

[pkalinnikov, 2016-10-20 14:26:23]

On 2016/10/20 14:26:00, pkalinnikov wrote:
> Balazs, please take a first look.

[engedy, 2016-10-21 09:51:31]

LGTM % comments.

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
File
components/subresource_filter/content/renderer/document_subresource_filter.h
(right):

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/document_subresource_filter.h:88:
// GENERICBLOCK activation type. Defined only if not
nit: Undefined if |filtering_disabled_for_document_|.

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/indexed_ruleset.cc (right):

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset.cc:314: //
Additionally, if |disable_generic_rules|, and the |domains| list corresponds
Phrasing suggestions: A rule whose domain list is empty or contains only
negative domains is still considered a `generic` rule. Therefore, if
|disable_generic_rules| is set, this function will always return false for such
|domains| lists.

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset.cc:431: //
|is_third_party| should reflect relation between |url| and |initiator|.
nit: the relation

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset.cc:432: bool
IsMatch(const flat::UrlPatternIndex* index,
nit: Did you intend to put this into the unnamed namespace?

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/indexed_ruleset_unittest.cc
(right):

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:37:
UrlRuleBuilder(const UrlPattern& url_pattern, bool is_whitelist = false)
nit: explicit

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:63:
domain->set_domain(std::move(domain_pattern));
nit: std::move is kinda pointless, set_domain takes a const string&, so it looks
like you can't avoid the copy. :-(

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:78: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:116:
EXPECT_EQ(expected_success, indexer_.AddUrlRule(rule));
As discussed below, I'd make this:

ASSERT_TRUE(indexer_.AddUrlRule(rule));

If we can somehow make |rule| into a string, then:

ASSERT_TRUE(indexer_.AddUrlRule(rule)) << MAGIC_TOSTR(rule);

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:564:
AddUrlRule(UrlRuleBuilder(UrlPattern("some_text", kSubstring)).rule());
Currently, if any of these calls fails, it is hard to discover which one did.
Printing the rule inside is one solution, another would be encapsulating all of
these calls in ASSERT_NO_FATAL_FAILURE.

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:683:
AddUrlRule(builder.rule(), false);
This is the only location where we pass "false" as the second argument. I would
just keep this line as it was, remove the optional argument, and make the
AddUrlRule always ASSERT that it succeeded.

[pkalinnikov, 2016-10-21 10:50:11]

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
File
components/subresource_filter/content/renderer/document_subresource_filter.h
(right):

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/document_subresource_filter.h:88:
// GENERICBLOCK activation type. Defined only if not
On 2016/10/21 09:51:30, engedy wrote:
> nit: Undefined if |filtering_disabled_for_document_|.

Done.

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/indexed_ruleset.cc (right):

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset.cc:314: //
Additionally, if |disable_generic_rules|, and the |domains| list corresponds
On 2016/10/21 09:51:30, engedy wrote:
> Phrasing suggestions: A rule whose domain list is empty or contains only
> negative domains is still considered a `generic` rule. Therefore, if
> |disable_generic_rules| is set, this function will always return false for
such
> |domains| lists.

Done.

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset.cc:431: //
|is_third_party| should reflect relation between |url| and |initiator|.
On 2016/10/21 09:51:30, engedy wrote:
> nit: the relation

Done.

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset.cc:432: bool
IsMatch(const flat::UrlPatternIndex* index,
On 2016/10/21 09:51:30, engedy wrote:
> nit: Did you intend to put this into the unnamed namespace?

Yep. Done.

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/indexed_ruleset_unittest.cc
(right):

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:37:
UrlRuleBuilder(const UrlPattern& url_pattern, bool is_whitelist = false)
On 2016/10/21 09:51:30, engedy wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:63:
domain->set_domain(std::move(domain_pattern));
On 2016/10/21 09:51:30, engedy wrote:
> nit: std::move is kinda pointless, set_domain takes a const string&, so it
looks
> like you can't avoid the copy. :-(

Those protobufs...

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:78: };
On 2016/10/21 09:51:30, engedy wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:116:
EXPECT_EQ(expected_success, indexer_.AddUrlRule(rule));
On 2016/10/21 09:51:30, engedy wrote:
> As discussed below, I'd make this:
> 
> ASSERT_TRUE(indexer_.AddUrlRule(rule));
> 
> If we can somehow make |rule| into a string, then:
> 
> ASSERT_TRUE(indexer_.AddUrlRule(rule)) << MAGIC_TOSTR(rule);

How about this?

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:564:
AddUrlRule(UrlRuleBuilder(UrlPattern("some_text", kSubstring)).rule());
On 2016/10/21 09:51:30, engedy wrote:
> Currently, if any of these calls fails, it is hard to discover which one did.
> Printing the rule inside is one solution, another would be encapsulating all
of
> these calls in ASSERT_NO_FATAL_FAILURE.

Done.

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:683:
AddUrlRule(builder.rule(), false);
On 2016/10/21 09:51:30, engedy wrote:
> This is the only location where we pass "false" as the second argument. I
would
> just keep this line as it was, remove the optional argument, and make the
> AddUrlRule always ASSERT that it succeeded.

Done.

[engedy, 2016-10-21 11:32:15]

LGTM.

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/indexed_ruleset_unittest.cc
(right):

https://codereview.chromium.org/2438743003/diff/1/components/subresource_filt...
components/subresource_filter/core/common/indexed_ruleset_unittest.cc:116:
EXPECT_EQ(expected_success, indexer_.AddUrlRule(rule));
On 2016/10/21 10:50:10, pkalinnikov wrote:
> On 2016/10/21 09:51:30, engedy wrote:
> > As discussed below, I'd make this:
> > 
> > ASSERT_TRUE(indexer_.AddUrlRule(rule));
> > 
> > If we can somehow make |rule| into a string, then:
> > 
> > ASSERT_TRUE(indexer_.AddUrlRule(rule)) << MAGIC_TOSTR(rule);
> 
> How about this?

Looks good.

[pkalinnikov, 2016-10-21 11:46:54]

The CQ bit was checked by pkalinnikov@chromium.org

[commit-bot: I haz the power, 2016-10-21 11:47:07]

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-10-21 12:55:03]

Description was changed from

==========
Implement GENERICBLOCK option.

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

to

==========
Implement GENERICBLOCK option.

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

[commit-bot: I haz the power, 2016-10-21 12:55:04]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-21 13:29:05]

Description was changed from

==========
Implement GENERICBLOCK option.

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

to

==========
Implement GENERICBLOCK option.

BUG=609747

Committed: https://crrev.com/7c855c7419981aaac5af823d7df2665af9e68bae
Cr-Commit-Position: refs/heads/master@{#426780}
==========

[commit-bot: I haz the power, 2016-10-21 13:29:06]

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