Make the subresource filter support subdomain anchor matching.

Make the subresource filter support subdomain anchor matching.

BUG=609747
Committed: https://crrev.com/854818d6eaaff10ee5273e82e517aef473eda9f1
Cr-Commit-Position: refs/heads/master@{#407135}

[pkalinnikov, 2016-07-20 12:32:07]

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

[pkalinnikov, 2016-07-20 12:32:08]

PTAL.

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

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/fuzzy_pattern_matching.cc:27: bool
StartsWithFuzzy(base::StringPiece text, base::StringPiece subpattern) {
Simplified these two functions to consistently *not* support the '^' matching
end-of-string. This will be returned back in another CL implementing this
feature in all the other places.

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/string_splitter.h (right):

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/string_splitter.h:70: const
StringSplitter* splitter_;
This is to make the Iterator assignable.

[engedy, 2016-07-20 15:09:40]

LGTM % comments.

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

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/fuzzy_pattern_matching.cc:27: bool
StartsWithFuzzy(base::StringPiece text, base::StringPiece subpattern) {
On 2016/07/20 12:32:08, pkalinnikov wrote:
> Simplified these two functions to consistently *not* support the '^' matching
> end-of-string. This will be returned back in another CL implementing this
> feature in all the other places.

Sounds good. I think we can even down-prioritize that CL in favor of integrating
this logic into DocumentSubresourceFilter.

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

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/fuzzy_pattern_matching_unittest.cc:22:
{"abc", "abcd", false}, /* {"abc", "abc^", true}, */
For the sake of clean code, could you please just remove the superfluous test
cases instead of commenting them out? You can always just look at git history to
retrieve them.

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/string_splitter.h (right):

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/string_splitter.h:70: const
StringSplitter* splitter_;
On 2016/07/20 12:32:08, pkalinnikov wrote:
> This is to make the Iterator assignable.

Sounds good. Should we make it take a pointer in the constructor too?

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/url_pattern_matching.h (right):

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching.h:82:
DCHECK(url.is_valid() && url.has_host());
Hmm, where do we ensure that this will be satisfied?

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching.h:85: const char*
begin = url.spec().data() + begin_position;
What do you think about not passing down a GURL to these functions, instead just
a StringPiece with the URL's spec, plus a url::Component indicating where the
host part is located within the URL? Then we could avoid the pointer arithmetics
and the data() calls, which I find a bit untidy.

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching.h:100: // Returns
the leftmost occurrence of |subpattern| in the |url|, such that it
nit: the position just beyond the leftmost...

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching.h:108: [&url,
subpattern](size_t position) {
nit: match_end_position, same below.

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching.h:190:
DCHECK_LE(subpattern.size(), spec.size());
This precondition is satisfied because the EndsWithFuzzy above, right?

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

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching_unittest.cc:116:
{{"examp", kSubdomain, kAnchorNone}, "http://example.com/", true},
Could you please add a test cases for:

 -- the full match:
      http://example.com/, example.com/

 -- the full non-match:
      http://example.com/, https://example.com/

 -- where the first match candidate is not good, but the second is:
      http://rex.com.ex.com/, ex.com
      http://http.com/, http

 -- where the candidate match overlaps the start of the host:
      http://example.com/, /example.com
      http://example.com/, /example.com/

[engedy, 2016-07-20 15:10:23]

Description was changed from

==========
Implement SUBDOMAIN anchor matching.

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

to

==========
Make the subresource filter support subdomain anchor matching.

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

[engedy, 2016-07-20 15:11:23]

Also, I lowercased subdomain in the CL description. :-)

[pkalinnikov, 2016-07-21 15:07:41]

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

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/fuzzy_pattern_matching_unittest.cc:22:
{"abc", "abcd", false}, /* {"abc", "abc^", true}, */
On 2016/07/20 15:09:39, engedy wrote:
> For the sake of clean code, could you please just remove the superfluous test
> cases instead of commenting them out? You can always just look at git history
to
> retrieve them.

Done.

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/string_splitter.h (right):

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/string_splitter.h:70: const
StringSplitter* splitter_;
On 2016/07/20 15:09:40, engedy wrote:
> On 2016/07/20 12:32:08, pkalinnikov wrote:
> > This is to make the Iterator assignable.
> 
> Sounds good. Should we make it take a pointer in the constructor too?

A pointer in the constructor allows to pass nullptr, which adds extra checks in
all methods using the |splitter_|. This is why I decided to pass it by
reference.

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/url_pattern_matching.h (right):

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching.h:82:
DCHECK(url.is_valid() && url.has_host());
On 2016/07/20 15:09:40, engedy wrote:
> Hmm, where do we ensure that this will be satisfied?

Added checks below in IsMatch.

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching.h:85: const char*
begin = url.spec().data() + begin_position;
On 2016/07/20 15:09:40, engedy wrote:
> What do you think about not passing down a GURL to these functions, instead
just
> a StringPiece with the URL's spec, plus a url::Component indicating where the
> host part is located within the URL? Then we could avoid the pointer
arithmetics
> and the data() calls, which I find a bit untidy.

How about still passing GURL, but using url::Component inside? I don't see a
necessity to pass spec+url::Component, because they both can be extracted from
the GURL.

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching.h:100: // Returns
the leftmost occurrence of |subpattern| in the |url|, such that it
On 2016/07/20 15:09:40, engedy wrote:
> nit: the position just beyond the leftmost...

Done.

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching.h:108: [&url,
subpattern](size_t position) {
On 2016/07/20 15:09:40, engedy wrote:
> nit: match_end_position, same below.

Done.

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching.h:190:
DCHECK_LE(subpattern.size(), spec.size());
On 2016/07/20 15:09:40, engedy wrote:
> This precondition is satisfied because the EndsWithFuzzy above, right?

Well, yes, unless we implement end-of-string matching. In this case the
|subpattern| can be 1 character longer. How about keeping this just in case?

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

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching_unittest.cc:116:
{{"examp", kSubdomain, kAnchorNone}, "http://example.com/", true},
On 2016/07/20 15:09:40, engedy wrote:
> Could you please add a test cases for:
> 
>  -- the full match:
>       http://example.com/, example.com/
> 
>  -- the full non-match:
>       http://example.com/, https://example.com/
> 
>  -- where the first match candidate is not good, but the second is:
>       http://rex.com.ex.com/, http://ex.com
>       http://http.com/, http
> 
>  -- where the candidate match overlaps the start of the host:
>       http://example.com/, /example.com
>       http://example.com/, /example.com/
> 

Done.

[engedy, 2016-07-22 09:09:08]

LGTM % comments.

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/string_splitter.h (right):

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/string_splitter.h:70: const
StringSplitter* splitter_;
On 2016/07/21 15:07:41, pkalinnikov wrote:
> On 2016/07/20 15:09:40, engedy wrote:
> > On 2016/07/20 12:32:08, pkalinnikov wrote:
> > > This is to make the Iterator assignable.
> > 
> > Sounds good. Should we make it take a pointer in the constructor too?
> 
> A pointer in the constructor allows to pass nullptr, which adds extra checks
in
> all methods using the |splitter_|. This is why I decided to pass it by
> reference.

Makes sense.

Given that we only use |splitter_| to access the predicate and to extract the
end of the string in the constructor, what do you think about explicitly passing
in the predicate and the end iterator? Just an idea, I'm fine with doing it
either way.

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/url_pattern_matching.h (right):

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching.h:85: const char*
begin = url.spec().data() + begin_position;
On 2016/07/21 15:07:41, pkalinnikov wrote:
> On 2016/07/20 15:09:40, engedy wrote:
> > What do you think about not passing down a GURL to these functions, instead
> just
> > a StringPiece with the URL's spec, plus a url::Component indicating where
the
> > host part is located within the URL? Then we could avoid the pointer
> arithmetics
> > and the data() calls, which I find a bit untidy.
> 
> How about still passing GURL, but using url::Component inside? I don't see a
> necessity to pass spec+url::Component, because they both can be extracted from
> the GURL.

Yes, however, I'd it would be somewhat cleaner to extract the spec string of the
GURL exactly once (in line 186), right after that line extract the boundaries of
the host component, then not touch the GURL ever again.

This would make it very clear that all indices in the rest of the file are
understood to be in the context single string, and would avoid even the
suspicion that we calculate some indexes based on string A, and then we used
them to index into string B. (Where, e.g., A could be url.spec(), where B could
be url.possibly_invalid_spec()).

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching.h:190:
DCHECK_LE(subpattern.size(), spec.size());
On 2016/07/21 15:07:41, pkalinnikov wrote:
> On 2016/07/20 15:09:40, engedy wrote:
> > This precondition is satisfied because the EndsWithFuzzy above, right?
> 
> Well, yes, unless we implement end-of-string matching. In this case the
> |subpattern| can be 1 character longer. How about keeping this just in case?

I just wanted to double-check that I understood the code correctly. DCHECKs are
exactly for declaring preconditions, so it is correct to keep it.

https://codereview.chromium.org/2167653002/diff/20001/components/subresource_...
File components/subresource_filter/core/common/url_pattern_matching.h (right):

https://codereview.chromium.org/2167653002/diff/20001/components/subresource_...
components/subresource_filter/core/common/url_pattern_matching.h:87: const int
begin_int = static_cast<int>(begin_position);
I'd suggest doing this conversion the other way around.

Because we explicitly check that the host is present, the host.begin/end()
should be non-negative (I am going out on a limb here and assume for a second
that GURL does the right thing in case of size_t->int overflows). Therefore,
size_t will always be able to accommodate a non-negative int value that
represents an index in an array.

There is no guarantee that the size of |url| will fit into an int. (Yes,
url::Component shouldn't use int's either, but that's a different story.)

https://codereview.chromium.org/2167653002/diff/20001/components/subresource_...
components/subresource_filter/core/common/url_pattern_matching.h:168: bool
IsMatch(const GURL& url,
Is this also the declaration? If so, we should add comments to document the
method.

https://codereview.chromium.org/2167653002/diff/20001/components/subresource_...
components/subresource_filter/core/common/url_pattern_matching.h:173: if
(pattern.anchor_left == proto::ANCHOR_TYPE_SUBDOMAIN && !url.has_host())
I'd suggest moving this to down below where IsSubdomain methods are called. It
will be less useful as documentation, but won't be forgotten that easily if we
refactor stuff.

[pkalinnikov, 2016-07-22 10:46:16]

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

[commit-bot: I haz the power, 2016-07-22 10:46:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pkalinnikov, 2016-07-22 10:46:49]

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
File components/subresource_filter/core/common/url_pattern_matching.h (right):

https://codereview.chromium.org/2167653002/diff/1/components/subresource_filt...
components/subresource_filter/core/common/url_pattern_matching.h:85: const char*
begin = url.spec().data() + begin_position;
On 2016/07/22 09:09:07, engedy wrote:
> On 2016/07/21 15:07:41, pkalinnikov wrote:
> > On 2016/07/20 15:09:40, engedy wrote:
> > > What do you think about not passing down a GURL to these functions,
instead
> > just
> > > a StringPiece with the URL's spec, plus a url::Component indicating where
> the
> > > host part is located within the URL? Then we could avoid the pointer
> > arithmetics
> > > and the data() calls, which I find a bit untidy.
> > 
> > How about still passing GURL, but using url::Component inside? I don't see a
> > necessity to pass spec+url::Component, because they both can be extracted
from
> > the GURL.
> 
> Yes, however, I'd it would be somewhat cleaner to extract the spec string of
the
> GURL exactly once (in line 186), right after that line extract the boundaries
of
> the host component, then not touch the GURL ever again.
> 
> This would make it very clear that all indices in the rest of the file are
> understood to be in the context single string, and would avoid even the
> suspicion that we calculate some indexes based on string A, and then we used
> them to index into string B. (Where, e.g., A could be url.spec(), where B
could
> be url.possibly_invalid_spec()).

Done.

https://codereview.chromium.org/2167653002/diff/20001/components/subresource_...
File components/subresource_filter/core/common/url_pattern_matching.h (right):

https://codereview.chromium.org/2167653002/diff/20001/components/subresource_...
components/subresource_filter/core/common/url_pattern_matching.h:87: const int
begin_int = static_cast<int>(begin_position);
On 2016/07/22 09:09:07, engedy wrote:
> I'd suggest doing this conversion the other way around.
> 
> Because we explicitly check that the host is present, the host.begin/end()
> should be non-negative (I am going out on a limb here and assume for a second
> that GURL does the right thing in case of size_t->int overflows). Therefore,
> size_t will always be able to accommodate a non-negative int value that
> represents an index in an array.
> 
> There is no guarantee that the size of |url| will fit into an int. (Yes,
> url::Component shouldn't use int's either, but that's a different story.)

Done.

https://codereview.chromium.org/2167653002/diff/20001/components/subresource_...
components/subresource_filter/core/common/url_pattern_matching.h:168: bool
IsMatch(const GURL& url,
On 2016/07/22 09:09:07, engedy wrote:
> Is this also the declaration? If so, we should add comments to document the
> method.

The declaration is above in the "Public interface declaration" section.

https://codereview.chromium.org/2167653002/diff/20001/components/subresource_...
components/subresource_filter/core/common/url_pattern_matching.h:173: if
(pattern.anchor_left == proto::ANCHOR_TYPE_SUBDOMAIN && !url.has_host())
On 2016/07/22 09:09:07, engedy wrote:
> I'd suggest moving this to down below where IsSubdomain methods are called. It
> will be less useful as documentation, but won't be forgotten that easily if we
> refactor stuff.

Done.

[commit-bot: I haz the power, 2016-07-22 11:23:50]

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

[commit-bot: I haz the power, 2016-07-22 11:23:51]

Dry run: This issue passed the CQ dry run.

[engedy, 2016-07-22 11:51:06]

I think this is very clean now, thank you! Still LGTM.

[pkalinnikov, 2016-07-22 11:51:37]

The CQ bit was checked by pkalinnikov@chromium.org

[commit-bot: I haz the power, 2016-07-22 11:51:46]

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-22 11:55:31]

Description was changed from

==========
Make the subresource filter support subdomain anchor matching.

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

to

==========
Make the subresource filter support subdomain anchor matching.

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

[commit-bot: I haz the power, 2016-07-22 11:55:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-22 11:57:03]

Description was changed from

==========
Make the subresource filter support subdomain anchor matching.

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

to

==========
Make the subresource filter support subdomain anchor matching.

BUG=609747

Committed: https://crrev.com/854818d6eaaff10ee5273e82e517aef473eda9f1
Cr-Commit-Position: refs/heads/master@{#407135}
==========

[commit-bot: I haz the power, 2016-07-22 11:57:04]

Patchset 3 (id:??) landed as
https://crrev.com/854818d6eaaff10ee5273e82e517aef473eda9f1
Cr-Commit-Position: refs/heads/master@{#407135}