Introduce UrlPattern.

Introduce UrlPattern. This is basically me resuming work on
issue 14106, but as it is a complete rewrite, I have started
a new issue.

I also added supporting JoinString() and ReplaceAll()
utility functions.

[Matt Perry, 2009-01-30 19:39:58]

looking good

http://codereview.chromium.org/19704/diff/13/213
File base/string_util.h (right):

http://codereview.chromium.org/19704/diff/13/213#newcode555
Line 555: std::string ReplaceAll(const std::string& in, const std::string &find,
You're in luck!  We have this function already.  See
ReplaceSubstringsAfterOffset

http://codereview.chromium.org/19704/diff/13/214
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/19704/diff/13/214#newcode1304
Line 1304: std::vector<std::string> in;
add two more test cases for an empty vector and a length-1 vector

http://codereview.chromium.org/19704/diff/13/217
File chrome/browser/extensions/url_pattern.cc (right):

http://codereview.chromium.org/19704/diff/13/217#newcode32
Line 32: bool UrlPattern::Parse(const std::string& pattern) {
Have you tried just making a GURL out of this pattern, and grabbing the
scheme(), host(), and path() from that?

http://codereview.chromium.org/19704/diff/13/217#newcode42
Line 42: DCHECK(host_start_pos < pattern.length());
wouldn't an input of "http://" trigger this DCHECK?

http://codereview.chromium.org/19704/diff/13/217#newcode64
Line 64: ++(host_components.begin()));
"host_components.begin() + 1" works.  Incrementing a temporary is interesting :)

http://codereview.chromium.org/19704/diff/13/217#newcode107
Line 107: if (dot_prefixed_host_.empty()) {
You could do away with this special variable by just matching against host_, and
checking that test.host()[test.host.length() - host_.length() - 1] == '.'

http://codereview.chromium.org/19704/diff/13/218
File chrome/browser/extensions/url_pattern.h (right):

http://codereview.chromium.org/19704/diff/13/218#newcode12
Line 12: // <url-pattern> := <scheme>://<host><path>
nit: is it worth noting that "host" doesn't apply to all schemes?

http://codereview.chromium.org/19704/diff/13/218#newcode69
Line 69: class UrlPattern {
Consistency wants this to be named URLPattern (we have lots of URL* classes,
like URLRequest, URLFetcher, etc).

http://codereview.chromium.org/19704/diff/13/218#newcode97
Line 97: static bool IsValidScheme(const std::string& scheme);
I'm gonna get all philosophical here, so feel free to ignore this.

Generally I prefer file-level non-member statics over member statics.  It's an
unfortunate consequence of C++ design that we have to declare this
implementation-specific junk in the header.  Someone looking at this header
doesn't care about the various helper functions and private constants you have.

The rule of thumb I follow is that any private method that can be a non-member
function, should be.  (For a more authoritative source of this opinion, see
Effective C++ by Scott Meyers.)

http://codereview.chromium.org/19704/diff/13/218#newcode103
Line 103: bool MatchesPath(const GURL& path);
nit: s/path/test

http://codereview.chromium.org/19704/diff/13/219
File chrome/browser/extensions/url_pattern_unittest.cc (right):

http://codereview.chromium.org/19704/diff/13/219#newcode11
Line 11: const char* kInvalidPatterns[] = {
add "http://"

http://codereview.chromium.org/19704/diff/13/219#newcode27
Line 27: UrlPattern pattern1;
should we break each of these patterns into a separate test?  i could see
arguments for either way, so it's your call.

[Aaron Boodman, 2009-01-30 21:44:29]

Thanks for the detailed feedback. New snapshot available.

Also, I moved url_pattern to common/ because it will be used in the renderer
too.

http://codereview.chromium.org/19704/diff/13/213
File base/string_util.h (right):

http://codereview.chromium.org/19704/diff/13/213#newcode555
Line 555: std::string ReplaceAll(const std::string& in, const std::string &find,
On 2009/01/30 19:39:58, Matt Perry wrote:
> You're in luck!  We have this function already.  See
> ReplaceSubstringsAfterOffset

Heh, whoops. Removed.

http://codereview.chromium.org/19704/diff/13/214
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/19704/diff/13/214#newcode1304
Line 1304: std::vector<std::string> in;
On 2009/01/30 19:39:58, Matt Perry wrote:
> add two more test cases for an empty vector and a length-1 vector

Done.

http://codereview.chromium.org/19704/diff/13/217
File chrome/browser/extensions/url_pattern.cc (right):

http://codereview.chromium.org/19704/diff/13/217#newcode32
Line 32: bool UrlPattern::Parse(const std::string& pattern) {
On 2009/01/30 19:39:58, Matt Perry wrote:
> Have you tried just making a GURL out of this pattern, and grabbing the
> scheme(), host(), and path() from that?

I actually did try that, first, and unfortunately it just ends up being more
work. Here are some of the issues I can remember:

- GURL escaped "*" when it is used in the host
- Doesn't know what to do with things like http://*.
- Normalizing when that isn't actually what we want (http://www.google.com
should be an error, not normalized).

In the end I decided that it just wasn't designed to do this, and it was going
to be more pain that it was worth to try and make it work.

http://codereview.chromium.org/19704/diff/13/217#newcode42
Line 42: DCHECK(host_start_pos < pattern.length());
On 2009/01/30 19:39:58, Matt Perry wrote:
> wouldn't an input of "http://" trigger this DCHECK?

Yes, good point. Fixed.

http://codereview.chromium.org/19704/diff/13/217#newcode64
Line 64: ++(host_components.begin()));
On 2009/01/30 19:39:58, Matt Perry wrote:
> "host_components.begin() + 1" works.  Incrementing a temporary is interesting
:)

Ok, thanks. I wasn't sure what the conventional way was to do this.

http://codereview.chromium.org/19704/diff/13/217#newcode107
Line 107: if (dot_prefixed_host_.empty()) {
On 2009/01/30 19:39:58, Matt Perry wrote:
> You could do away with this special variable by just matching against host_,
and
> checking that test.host()[test.host.length() - host_.length() - 1] == '.'

Done. FWIW, I did this because in the last version of this code, I did the check
similar to how you suggest here, and you thought it would be more readable to
create the temporary :).

http://codereview.chromium.org/19704/diff/13/218
File chrome/browser/extensions/url_pattern.h (right):

http://codereview.chromium.org/19704/diff/13/218#newcode12
Line 12: // <url-pattern> := <scheme>://<host><path>
On 2009/01/30 19:39:58, Matt Perry wrote:
> nit: is it worth noting that "host" doesn't apply to all schemes?

It is noted, below. I could do a real BNF, but it is more complex to understand
at a glance I felt like this was better for a comment.

http://codereview.chromium.org/19704/diff/13/218#newcode69
Line 69: class UrlPattern {
On 2009/01/30 19:39:58, Matt Perry wrote:
> Consistency wants this to be named URLPattern (we have lots of URL* classes,
> like URLRequest, URLFetcher, etc).

Done.

http://codereview.chromium.org/19704/diff/13/218#newcode97
Line 97: static bool IsValidScheme(const std::string& scheme);
On 2009/01/30 19:39:58, Matt Perry wrote:
> I'm gonna get all philosophical here, so feel free to ignore this.
> 
> Generally I prefer file-level non-member statics over member statics.  It's an
> unfortunate consequence of C++ design that we have to declare this
> implementation-specific junk in the header.  Someone looking at this header
> doesn't care about the various helper functions and private constants you
have.
> 
> The rule of thumb I follow is that any private method that can be a non-member
> function, should be.  (For a more authoritative source of this opinion, see
> Effective C++ by Scott Meyers.)

Done. Rules of thumb very much appreciated. I am new around here.

http://codereview.chromium.org/19704/diff/13/218#newcode103
Line 103: bool MatchesPath(const GURL& path);
On 2009/01/30 19:39:58, Matt Perry wrote:
> nit: s/path/test

Done.

http://codereview.chromium.org/19704/diff/13/219
File chrome/browser/extensions/url_pattern_unittest.cc (right):

http://codereview.chromium.org/19704/diff/13/219#newcode11
Line 11: const char* kInvalidPatterns[] = {
On 2009/01/30 19:39:58, Matt Perry wrote:
> add "http://"

Done.

http://codereview.chromium.org/19704/diff/13/219#newcode27
Line 27: UrlPattern pattern1;
On 2009/01/30 19:39:58, Matt Perry wrote:
> should we break each of these patterns into a separate test?  i could see
> arguments for either way, so it's your call.

I did have some paste-o type errors when writing this due to  pattern1,
pattern2, etc, so I think it's a good idea to break them apart.

Done.

[Matt Perry, 2009-01-30 23:33:14]

LGTM

http://codereview.chromium.org/19704/diff/13/217
File chrome/browser/extensions/url_pattern.cc (right):

http://codereview.chromium.org/19704/diff/13/217#newcode32
Line 32: bool UrlPattern::Parse(const std::string& pattern) {
On 2009/01/30 21:44:29, Aaron Boodman wrote:
> On 2009/01/30 19:39:58, Matt Perry wrote:
> > Have you tried just making a GURL out of this pattern, and grabbing the
> > scheme(), host(), and path() from that?
> 
> I actually did try that, first, and unfortunately it just ends up being more
> work.

Fair enough.  The pattern is simple enough that we probably don't need parsing
as robust as GURL.

Though, now I'm wondering.. what about non-ASCII URLs?  I guess it isn't
possible to match them with these patterns.

http://codereview.chromium.org/19704/diff/13/217#newcode107
Line 107: if (dot_prefixed_host_.empty()) {
On 2009/01/30 21:44:29, Aaron Boodman wrote:
> On 2009/01/30 19:39:58, Matt Perry wrote:
> > You could do away with this special variable by just matching against host_,
> and
> > checking that test.host()[test.host.length() - host_.length() - 1] == '.'
> 
> Done. FWIW, I did this because in the last version of this code, I did the
check
> similar to how you suggest here, and you thought it would be more readable to
> create the temporary :).

Ha!  I have to work on being more consistent :).

http://codereview.chromium.org/19704/diff/13/218
File chrome/browser/extensions/url_pattern.h (right):

http://codereview.chromium.org/19704/diff/13/218#newcode97
Line 97: static bool IsValidScheme(const std::string& scheme);
On 2009/01/30 21:44:29, Aaron Boodman wrote:
> On 2009/01/30 19:39:58, Matt Perry wrote:
> > I'm gonna get all philosophical here, so feel free to ignore this.
> > 
> > Generally I prefer file-level non-member statics over member statics.  It's
an
> > unfortunate consequence of C++ design that we have to declare this
> > implementation-specific junk in the header.  Someone looking at this header
> > doesn't care about the various helper functions and private constants you
> have.
> > 
> > The rule of thumb I follow is that any private method that can be a
non-member
> > function, should be.  (For a more authoritative source of this opinion, see
> > Effective C++ by Scott Meyers.)
> 
> Done. Rules of thumb very much appreciated. I am new around here.

Just to be clear, this isn't a team style issue at all, just my personal
preference :).

[Matt Perry, 2009-01-31 01:29:14]

lgtm

http://codereview.chromium.org/19704/diff/41/51
File chrome/common/extensions/url_pattern_unittest.cc (right):

http://codereview.chromium.org/19704/diff/41/51#newcode131
Line 131: GURL("http://\xe1\x80\xbf/a\xc2\x81\xe1\xe1")));
interesting.  what does
pattern.MatchesUrl(GURL("http://foo.xn--gkd/a\xc2\x81\xel\xel")) return?  we
should specify either way.