Event matching by regular expression matching on URLs.

Event matching by regular expression matching on URLs.

Uses FilteredRE2 implementation to more efficiently match lots of regexes at once by prefiltering.

BUG=135609, 112155
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=156693

[battre, 2012-09-11 21:49:48]

Sorry, I did not get to do a real review today. Just sending the what I got
done.

https://chromiumcodereview.appspot.com/10910179/diff/2001/chrome/common/exten...
File chrome/common/extensions/matcher/regex_set_matcher.h (right):

https://chromiumcodereview.appspot.com/10910179/diff/2001/chrome/common/exten...
chrome/common/extensions/matcher/regex_set_matcher.h:34: // patterns at once.
I think for consistency to SubStringMatcher, the ownership of the StringPatterns
should remain with the caller. (or it should be changed there as well)

https://chromiumcodereview.appspot.com/10910179/diff/2001/chrome/common/exten...
chrome/common/extensions/matcher/regex_set_matcher.h:48: std::vector<int>
FindSubstringMatches(const std::string& text) const;
int -> StringPattern::ID? I am not sure whether this is really a
StringPattern::ID. I think it is useful to distinguish the IDs by typedefs.

https://chromiumcodereview.appspot.com/10910179/diff/2001/chrome/common/exten...
chrome/common/extensions/matcher/regex_set_matcher.h:71: // |substring_matcher_|
but whose lifetime is managed here.
doesn't lifetime management imply that we delete the objects?

[Yoyo Zhou, 2012-09-11 22:35:37]

https://chromiumcodereview.appspot.com/10910179/diff/2001/chrome/common/exten...
File chrome/common/extensions/matcher/regex_set_matcher.h (right):

https://chromiumcodereview.appspot.com/10910179/diff/2001/chrome/common/exten...
chrome/common/extensions/matcher/regex_set_matcher.h:34: // patterns at once.
On 2012/09/11 21:49:48, battre wrote:
> I think for consistency to SubStringMatcher, the ownership of the
StringPatterns
> should remain with the caller. (or it should be changed there as well)

It does. This is perhaps confusing -- the regex patterns are managed in
UrlMatcher's regex_pattern_singletons_ -- so I should add a note.

https://chromiumcodereview.appspot.com/10910179/diff/2001/chrome/common/exten...
chrome/common/extensions/matcher/regex_set_matcher.h:48: std::vector<int>
FindSubstringMatches(const std::string& text) const;
On 2012/09/11 21:49:48, battre wrote:
> int -> StringPattern::ID? I am not sure whether this is really a
> StringPattern::ID. I think it is useful to distinguish the IDs by typedefs.

These ints are the FilteredRE2 IDs. I can make a typedef for those.

https://chromiumcodereview.appspot.com/10910179/diff/2001/chrome/common/exten...
chrome/common/extensions/matcher/regex_set_matcher.h:71: // |substring_matcher_|
but whose lifetime is managed here.
On 2012/09/11 21:49:48, battre wrote:
> doesn't lifetime management imply that we delete the objects?

DeleteSubstringPatterns does that.

[battre, 2012-09-12 18:04:50]

LGTM. Great to see how easy it was to integrate this into the existing
architecture.

I have mostly nits. Let me know if you want me to have another look.

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/api/events.json (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/api/events.json:269: "description": "Matches if the URL
matches a specified regular expression. The regular expressions use the <a
href=\"http://code.google.com/p/re2/wiki/Syntax\">RE2 syntax</a>.",
nit: add "(without fragment identifier)"? Add that port numbers are stripped
from the URL if they match the default port number.

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/matcher/regex_set_matcher.cc (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/regex_set_matcher.cc:87: <<
it->second->pattern() << ")";
I think it would be great if we could report such errors in some way so that
users are aware that their RegEx were invalid.

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/regex_set_matcher.cc:93:
DeleteSubstringPatterns();
Here you delete objects, that substring_matcher_ still refers to. I think this
is brittle and would recommend to reset the SubstringSetMatcher before deleting
the old references even though the code in this class will become more
complicated. WDYT?

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/matcher/regex_set_matcher.h (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/regex_set_matcher.h:66: // Mapping of IDs from
FilteredRE2 (which are assigned in order)
nit: Mapping of RE2IDs from ...

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/matcher/string_pattern.cc (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/string_pattern.cc:17: if (id_ > rhs.id_) return
false;
as we are toughing the code anyway, I would recommend to replace the previous
lines with
if (id_ != rhs_.id_) return id_ < rhs.id_;

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/matcher/url_matcher.cc (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher.cc:24: // ----------------------   
                --------------------
nit: adapt ASCII drawing

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher.cc:133: // against full URLs, which
are transformed as in case 2.
I think we want to point this out in the documentation.

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher.cc:208: if
(!ContainsKey(matching_patterns, string_pattern_->id()))
Thanks for introducing me to this.

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/matcher/url_matcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher_unittest.cc:207:
factory.CreateURLEqualsCondition("foo").criterion());
Can you add one more test for the regex matching here?

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher_unittest.cc:224: //
ForgetUnusedPatterns.
can you test that this also holds for regex singletons?

[Yoyo Zhou, 2012-09-12 20:25:56]

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/api/events.json (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/api/events.json:269: "description": "Matches if the URL
matches a specified regular expression. The regular expressions use the <a
href=\"http://code.google.com/p/re2/wiki/Syntax\">RE2 syntax</a>.",
On 2012/09/12 18:04:50, battre wrote:
> nit: add "(without fragment identifier)"? Add that port numbers are stripped
> from the URL if they match the default port number.

Done.

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/matcher/regex_set_matcher.cc (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/regex_set_matcher.cc:87: <<
it->second->pattern() << ")";
On 2012/09/12 18:04:50, battre wrote:
> I think it would be great if we could report such errors in some way so that
> users are aware that their RegEx were invalid.

Yes, I looked into this and it seemed like it would be a somewhat hairier
change, so I left it as a TODO (actually the next thing I'll be doing). I think
it'd be better to test for regex validity earlier, so that by the time the
regexes get here we know they're good.

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/regex_set_matcher.cc:93:
DeleteSubstringPatterns();
On 2012/09/12 18:04:50, battre wrote:
> Here you delete objects, that substring_matcher_ still refers to. I think this
> is brittle and would recommend to reset the SubstringSetMatcher before
deleting
> the old references even though the code in this class will become more
> complicated. WDYT?

I just moved the substring_matcher_.reset line to above this one.

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/matcher/regex_set_matcher.h (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/regex_set_matcher.h:66: // Mapping of IDs from
FilteredRE2 (which are assigned in order)
On 2012/09/12 18:04:50, battre wrote:
> nit: Mapping of RE2IDs from ...

Done. Expanded "regex IDs" to "regex StringPattern::IDs".

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/matcher/string_pattern.cc (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/string_pattern.cc:17: if (id_ > rhs.id_) return
false;
On 2012/09/12 18:04:50, battre wrote:
> as we are toughing the code anyway, I would recommend to replace the previous
> lines with
> if (id_ != rhs_.id_) return id_ < rhs.id_;

Done.

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/matcher/url_matcher.cc (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher.cc:24: // ----------------------   
                --------------------
On 2012/09/12 18:04:50, battre wrote:
> nit: adapt ASCII drawing

I'm not sure this helps for regular expressions; they're not really transformed
in any meaningful way. I added more comments though.

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher.cc:133: // against full URLs, which
are transformed as in case 2.
On 2012/09/12 18:04:50, battre wrote:
> I think we want to point this out in the documentation.

Do you mean the comments at the top of this file? Or the user-facing
documentation (intros/events.html or intros/declarativeWebRequest.html)? I was
not sure where in the docs it would be good to point out.

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/matcher/url_matcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher_unittest.cc:207:
factory.CreateURLEqualsCondition("foo").criterion());
On 2012/09/12 18:04:50, battre wrote:
> Can you add one more test for the regex matching here?

Done.

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher_unittest.cc:224: //
ForgetUnusedPatterns.
On 2012/09/12 18:04:50, battre wrote:
> can you test that this also holds for regex singletons?

Yes. I also made this test check that regex patterns don't share IDs with
substring patterns.

[battre, 2012-09-12 20:50:39]

LGTM, thx

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/matcher/regex_set_matcher.cc (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/regex_set_matcher.cc:87: <<
it->second->pattern() << ")";
On 2012/09/12 20:25:56, Yoyo Zhou wrote:
> On 2012/09/12 18:04:50, battre wrote:
> > I think it would be great if we could report such errors in some way so that
> > users are aware that their RegEx were invalid.
> 
> Yes, I looked into this and it seemed like it would be a somewhat hairier
> change, so I left it as a TODO (actually the next thing I'll be doing). I
think
> it'd be better to test for regex validity earlier, so that by the time the
> regexes get here we know they're good.

SGTM

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/matcher/url_matcher.cc (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher.cc:24: // ----------------------   
                --------------------
On 2012/09/12 20:25:56, Yoyo Zhou wrote:
> On 2012/09/12 18:04:50, battre wrote:
> > nit: adapt ASCII drawing
> 
> I'm not sure this helps for regular expressions; they're not really
transformed
> in any meaningful way. I added more comments though.

Oh, I was just referring to the point that the ------- lines were a little bit
too long.

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher.cc:133: // against full URLs, which
are transformed as in case 2.
On 2012/09/12 20:25:56, Yoyo Zhou wrote:
> On 2012/09/12 18:04:50, battre wrote:
> > I think we want to point this out in the documentation.
> 
> Do you mean the comments at the top of this file? Or the user-facing
> documentation (intros/events.html or intros/declarativeWebRequest.html)? I was
> not sure where in the docs it would be good to point out.

I think you fixed this by addressing my comment to the json file.

[Yoyo Zhou, 2012-09-12 21:58:06]

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
File chrome/common/extensions/matcher/url_matcher.cc (right):

https://chromiumcodereview.appspot.com/10910179/diff/1021/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher.cc:24: // ----------------------   
                --------------------
On 2012/09/12 20:50:39, battre wrote:
> On 2012/09/12 20:25:56, Yoyo Zhou wrote:
> > On 2012/09/12 18:04:50, battre wrote:
> > > nit: adapt ASCII drawing
> > 
> > I'm not sure this helps for regular expressions; they're not really
> transformed
> > in any meaningful way. I added more comments though.
> 
> Oh, I was just referring to the point that the ------- lines were a little bit
> too long.

Ah, it's that the art is ugly. Fixed.

[commit-bot: I haz the power, 2012-09-13 00:11:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/10910179/3028

[commit-bot: I haz the power, 2012-09-13 00:11:36]

Presubmit check for 10910179-3028 failed and returned exit status 1.


All integration tests passed!
Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 4.0s to calculate.

[Yoyo Zhou, 2012-09-13 00:13:49]

+ben for chrome/

[Ben Goodger (Google), 2012-09-13 15:40:20]

lgtm

[commit-bot: I haz the power, 2012-09-13 15:42:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/10910179/3028