Use extension match pattern syntax in content settings extension API

Use extension match pattern syntax in content settings extension API.

This requires adding a port to a URLPattern, but that shouldn't change existing behavior, as we already have a lenient parsing mode there.

BUG=71067
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91099

[Bernhard Bauer, 2011-06-24 15:36:17]

please review.

[Matt Perry, 2011-06-24 18:53:58]

Sam, you did some work with ports in URLPatterns. Do you have an opinion on the
changes to URLPattern here?

http://codereview.chromium.org/7229012/diff/8017/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_content_settings_unittest.cc (right):

http://codereview.chromium.org/7229012/diff/8017/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_unittest.cc:19: {
"file:///foo/bar/baz", "file:///foo/bar/baz" },
could you add a few more cases (test port and https)? and another test for error
cases (http://foo.com/path and file://with/wildcards*)

http://codereview.chromium.org/7229012/diff/8017/chrome/common/extensions/url...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7229012/diff/8017/chrome/common/extensions/url...
chrome/common/extensions/url_pattern.cc:212: if (!SetPort(host_.substr(port_pos
+ 1)))
This does change the behavior slightly. Previously PARSE_LENIENT would just drop
the port. Now it is preserved, and will affect GetAsString(). I'm not sure if
this will break anything. I'd feel safer if we added a separate
"set_allow_ports" switch that only the content settings used.

It's also strange that MatchesURL and OverlapsWith completely ignore the port.
If we're going to allow ports here, it seems like we should have them affect the
functionality (or maybe, they shouldn't affect anything, and remove the changes
to GetAsString).

http://codereview.chromium.org/7229012/diff/8017/chrome/common/extensions/url...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7229012/diff/8017/chrome/common/extensions/url...
chrome/common/extensions/url_pattern.h:19: // <host> := '*' | '*.' <anychar
except '/' and '*'>+
add a mention of the <port> here, that explains when it is allowed

[Bernhard Bauer, 2011-06-24 21:49:53]

http://codereview.chromium.org/7229012/diff/8017/chrome/common/extensions/url...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7229012/diff/8017/chrome/common/extensions/url...
chrome/common/extensions/url_pattern.cc:212: if (!SetPort(host_.substr(port_pos
+ 1)))
On 2011/06/24 18:53:58, Matt Perry wrote:
> This does change the behavior slightly. Previously PARSE_LENIENT would just
drop
> the port. Now it is preserved, and will affect GetAsString(). I'm not sure if
> this will break anything. I'd feel safer if we added a separate
> "set_allow_ports" switch that only the content settings used.
>
> It's also strange that MatchesURL and OverlapsWith completely ignore the port.
> If we're going to allow ports here, it seems like we should have them affect
the
> functionality (or maybe, they shouldn't affect anything, and remove the
changes
> to GetAsString).

I'd like to keep the changes to GetAsString, so we can return content settings
patterns to an extension later.

One thing we could do would be to use a wildcard for the port when
|set_allow_ports| is not set. This would make the current behavior correct w/r/t
the port, and also remove the port again when serializing the pattern to a
string.

[Sam Kerner (Chrome), 2011-06-27 17:58:25]

http://codereview.chromium.org/7229012/diff/12001/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7229012/diff/12001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:24: // * The port is only allowed when
the pattern is parsed with the PARSE_LENIENT
The idea of lenient vs. strict parsing made sense when ports were not part of
any valid pattern.  If ports are not an error for some users, a change in
terminology is needed.


Once upon a time, the parser did not throw an error when it saw a port. 
http://foo.com:123/* would be parsed so that the host was "foo.com:123".  People
put ports in patterns, and wondered why they didn't work.  We wanted the parser
to fail on ports, but this would break existing installs.  So we fail on ports
when loading an unpacked extension (strict mode), and tolerate ports when
loading an installed extension (lenient mode).

How about making ParseOption have a way to request that ports are ignored, and
another option to indicate that ports are an error?  Then PARSE_LENIENT,
PARSE_STRICT, and full support for ports could all be expressed.  Something
like:

enum ParseOption {
  FORBID_PORTS = 1<<0,
  ERROR_ON_PORTS = 1<<1;

  PARSE_MANIFEST_PATTERN_LENIENT = FORBID_PORTS,  // Formerly PARSE_LENIENT
  PARSE_MANIFEST_PATTERN_STRICT = FORBID_PORTS & ERROR_ON_PORTS  // Formerly
PARSE_STRICT
};

[Bernhard Bauer, 2011-06-27 18:05:03]

Okay, but shouldn't we call it IGNORE_PORTS instead of FORBID? I don't
see a big difference between FORBID and ERROR_ON.

I think I already have something pretty similar in the newest
(not-yet-uploaded) version of the CL, I just store the flag whether we
ignore ports in an instance variable in the pattern (like Matt
suggested).

On Mon, Jun 27, 2011 at 19:58,  <skerner@chromium.org> wrote:
>
>
http://codereview.chromium.org/7229012/diff/12001/chrome/common/extensions/ur...
> File chrome/common/extensions/url_pattern.h (right):
>
>
http://codereview.chromium.org/7229012/diff/12001/chrome/common/extensions/ur...
> chrome/common/extensions/url_pattern.h:24: // * The port is only allowed
> when the pattern is parsed with the PARSE_LENIENT
> The idea of lenient vs. strict parsing made sense when ports were not
> part of any valid pattern.  If ports are not an error for some users, a
> change in terminology is needed.
>
>
> Once upon a time, the parser did not throw an error when it saw a port.
> http://foo.com:123/* would be parsed so that the host was "foo.com:123".
>  People put ports in patterns, and wondered why they didn't work.  We
> wanted the parser to fail on ports, but this would break existing
> installs.  So we fail on ports when loading an unpacked extension
> (strict mode), and tolerate ports when loading an installed extension
> (lenient mode).
>
> How about making ParseOption have a way to request that ports are
> ignored, and another option to indicate that ports are an error?  Then
> PARSE_LENIENT, PARSE_STRICT, and full support for ports could all be
> expressed.  Something like:
>
> enum ParseOption {
>  FORBID_PORTS = 1<<0,
>  ERROR_ON_PORTS = 1<<1;
>
>  PARSE_MANIFEST_PATTERN_LENIENT = FORBID_PORTS,  // Formerly
> PARSE_LENIENT
>  PARSE_MANIFEST_PATTERN_STRICT = FORBID_PORTS & ERROR_ON_PORTS  //
> Formerly PARSE_STRICT
> };
>
> http://codereview.chromium.org/7229012/
>

[skerner, 2011-06-27 18:18:21]

On Mon, Jun 27, 2011 at 2:04 PM, Bernhard Bauer <bauerb@chromium.org> wrote:
> Okay, but shouldn't we call it IGNORE_PORTS instead of FORBID? I don't
> see a big difference between FORBID and ERROR_ON.

IGNORE_PORTS makes sense.  Here is the behavior I had in mind:

! FORBID_PORTS && ! ERROR_ON_PORTS: Ports are read and used.
! FORBID_PORTS &&   ERROR_ON_PORTS: CHECK()
  FORBID_PORTS && ! ERROR_ON_PORTS: Like LENIENT
  FORBID_PORTS &&   ERROR_ON_PORTS: Like STRICT


>
> I think I already have something pretty similar in the newest
> (not-yet-uploaded) version of the CL, I just store the flag whether we
> ignore ports in an instance variable in the pattern (like Matt
> suggested).

It seems strange that there are parse options, and also a setter to
control parsing.  I think there is a constructor that takes a pattern,
and unless you add a bool to that constructor you can't use it to
parse ports.  We have a style policy pushing enumerated fields over
boolean arguments to control options, so I would recommend making this
part of ParseOptions.

>
> On Mon, Jun 27, 2011 at 19:58,  <skerner@chromium.org> wrote:
>>
>>
http://codereview.chromium.org/7229012/diff/12001/chrome/common/extensions/ur...
>> File chrome/common/extensions/url_pattern.h (right):
>>
>>
http://codereview.chromium.org/7229012/diff/12001/chrome/common/extensions/ur...
>> chrome/common/extensions/url_pattern.h:24: // * The port is only allowed
>> when the pattern is parsed with the PARSE_LENIENT
>> The idea of lenient vs. strict parsing made sense when ports were not
>> part of any valid pattern.  If ports are not an error for some users, a
>> change in terminology is needed.
>>
>>
>> Once upon a time, the parser did not throw an error when it saw a port.
>> http://foo.com:123/* would be parsed so that the host was "foo.com:123".
>>  People put ports in patterns, and wondered why they didn't work.  We
>> wanted the parser to fail on ports, but this would break existing
>> installs.  So we fail on ports when loading an unpacked extension
>> (strict mode), and tolerate ports when loading an installed extension
>> (lenient mode).
>>
>> How about making ParseOption have a way to request that ports are
>> ignored, and another option to indicate that ports are an error?  Then
>> PARSE_LENIENT, PARSE_STRICT, and full support for ports could all be
>> expressed.  Something like:
>>
>> enum ParseOption {
>>  FORBID_PORTS = 1<<0,
>>  ERROR_ON_PORTS = 1<<1;
>>
>>  PARSE_MANIFEST_PATTERN_LENIENT = FORBID_PORTS,  // Formerly
>> PARSE_LENIENT
>>  PARSE_MANIFEST_PATTERN_STRICT = FORBID_PORTS & ERROR_ON_PORTS  //
>> Formerly PARSE_STRICT
>> };
>>
>> http://codereview.chromium.org/7229012/
>>
>

[Bernhard Bauer, 2011-06-27 18:36:53]

SGTM, although I would prefer an enum (IGNORE_PORTS, ERROR_ON_PORTS,
USE_PORTS?) so we don't have conflicting flag combinations.

On Mon, Jun 27, 2011 at 20:17, Sam Kerner <skerner@google.com> wrote:
> On Mon, Jun 27, 2011 at 2:04 PM, Bernhard Bauer <bauerb@chromium.org> wrote:
>> Okay, but shouldn't we call it IGNORE_PORTS instead of FORBID? I don't
>> see a big difference between FORBID and ERROR_ON.
>
> IGNORE_PORTS makes sense.  Here is the behavior I had in mind:
>
> ! FORBID_PORTS && ! ERROR_ON_PORTS: Ports are read and used.
> ! FORBID_PORTS &&   ERROR_ON_PORTS: CHECK()
>  FORBID_PORTS && ! ERROR_ON_PORTS: Like LENIENT
>  FORBID_PORTS &&   ERROR_ON_PORTS: Like STRICT
>
>
>>
>> I think I already have something pretty similar in the newest
>> (not-yet-uploaded) version of the CL, I just store the flag whether we
>> ignore ports in an instance variable in the pattern (like Matt
>> suggested).
>
> It seems strange that there are parse options, and also a setter to
> control parsing.  I think there is a constructor that takes a pattern,
> and unless you add a bool to that constructor you can't use it to
> parse ports.  We have a style policy pushing enumerated fields over
> boolean arguments to control options, so I would recommend making this
> part of ParseOptions.
>
>>
>> On Mon, Jun 27, 2011 at 19:58,  <skerner@chromium.org> wrote:
>>>
>>>
http://codereview.chromium.org/7229012/diff/12001/chrome/common/extensions/ur...
>>> File chrome/common/extensions/url_pattern.h (right):
>>>
>>>
http://codereview.chromium.org/7229012/diff/12001/chrome/common/extensions/ur...
>>> chrome/common/extensions/url_pattern.h:24: // * The port is only allowed
>>> when the pattern is parsed with the PARSE_LENIENT
>>> The idea of lenient vs. strict parsing made sense when ports were not
>>> part of any valid pattern.  If ports are not an error for some users, a
>>> change in terminology is needed.
>>>
>>>
>>> Once upon a time, the parser did not throw an error when it saw a port.
>>> http://foo.com:123/* would be parsed so that the host was "foo.com:123".
>>>  People put ports in patterns, and wondered why they didn't work.  We
>>> wanted the parser to fail on ports, but this would break existing
>>> installs.  So we fail on ports when loading an unpacked extension
>>> (strict mode), and tolerate ports when loading an installed extension
>>> (lenient mode).
>>>
>>> How about making ParseOption have a way to request that ports are
>>> ignored, and another option to indicate that ports are an error?  Then
>>> PARSE_LENIENT, PARSE_STRICT, and full support for ports could all be
>>> expressed.  Something like:
>>>
>>> enum ParseOption {
>>>  FORBID_PORTS = 1<<0,
>>>  ERROR_ON_PORTS = 1<<1;
>>>
>>>  PARSE_MANIFEST_PATTERN_LENIENT = FORBID_PORTS,  // Formerly
>>> PARSE_LENIENT
>>>  PARSE_MANIFEST_PATTERN_STRICT = FORBID_PORTS & ERROR_ON_PORTS  //
>>> Formerly PARSE_STRICT
>>> };
>>>
>>> http://codereview.chromium.org/7229012/
>>>
>>
>

[skerner, 2011-06-27 19:40:01]

On Mon, Jun 27, 2011 at 2:36 PM, Bernhard Bauer <bauerb@chromium.org> wrote:
> SGTM, although I would prefer an enum (IGNORE_PORTS, ERROR_ON_PORTS,
> USE_PORTS?) so we don't have conflicting flag combinations.

All right, that SGTM.

>
> On Mon, Jun 27, 2011 at 20:17, Sam Kerner <skerner@google.com> wrote:
>> On Mon, Jun 27, 2011 at 2:04 PM, Bernhard Bauer <bauerb@chromium.org> wrote:
>>> Okay, but shouldn't we call it IGNORE_PORTS instead of FORBID? I don't
>>> see a big difference between FORBID and ERROR_ON.
>>
>> IGNORE_PORTS makes sense.  Here is the behavior I had in mind:
>>
>> ! FORBID_PORTS && ! ERROR_ON_PORTS: Ports are read and used.
>> ! FORBID_PORTS &&   ERROR_ON_PORTS: CHECK()
>>  FORBID_PORTS && ! ERROR_ON_PORTS: Like LENIENT
>>  FORBID_PORTS &&   ERROR_ON_PORTS: Like STRICT
>>
>>
>>>
>>> I think I already have something pretty similar in the newest
>>> (not-yet-uploaded) version of the CL, I just store the flag whether we
>>> ignore ports in an instance variable in the pattern (like Matt
>>> suggested).
>>
>> It seems strange that there are parse options, and also a setter to
>> control parsing.  I think there is a constructor that takes a pattern,
>> and unless you add a bool to that constructor you can't use it to
>> parse ports.  We have a style policy pushing enumerated fields over
>> boolean arguments to control options, so I would recommend making this
>> part of ParseOptions.
>>
>>>
>>> On Mon, Jun 27, 2011 at 19:58,  <skerner@chromium.org> wrote:
>>>>
>>>>
http://codereview.chromium.org/7229012/diff/12001/chrome/common/extensions/ur...
>>>> File chrome/common/extensions/url_pattern.h (right):
>>>>
>>>>
http://codereview.chromium.org/7229012/diff/12001/chrome/common/extensions/ur...
>>>> chrome/common/extensions/url_pattern.h:24: // * The port is only allowed
>>>> when the pattern is parsed with the PARSE_LENIENT
>>>> The idea of lenient vs. strict parsing made sense when ports were not
>>>> part of any valid pattern.  If ports are not an error for some users, a
>>>> change in terminology is needed.
>>>>
>>>>
>>>> Once upon a time, the parser did not throw an error when it saw a port.
>>>> http://foo.com:123/* would be parsed so that the host was "foo.com:123".
>>>>  People put ports in patterns, and wondered why they didn't work.  We
>>>> wanted the parser to fail on ports, but this would break existing
>>>> installs.  So we fail on ports when loading an unpacked extension
>>>> (strict mode), and tolerate ports when loading an installed extension
>>>> (lenient mode).
>>>>
>>>> How about making ParseOption have a way to request that ports are
>>>> ignored, and another option to indicate that ports are an error?  Then
>>>> PARSE_LENIENT, PARSE_STRICT, and full support for ports could all be
>>>> expressed.  Something like:
>>>>
>>>> enum ParseOption {
>>>>  FORBID_PORTS = 1<<0,
>>>>  ERROR_ON_PORTS = 1<<1;
>>>>
>>>>  PARSE_MANIFEST_PATTERN_LENIENT = FORBID_PORTS,  // Formerly
>>>> PARSE_LENIENT
>>>>  PARSE_MANIFEST_PATTERN_STRICT = FORBID_PORTS & ERROR_ON_PORTS  //
>>>> Formerly PARSE_STRICT
>>>> };
>>>>
>>>> http://codereview.chromium.org/7229012/
>>>>
>>>
>>
>

[Bernhard Bauer, 2011-06-28 16:46:31]

Ok, added a third ParseOption. PTAL?

http://codereview.chromium.org/7229012/diff/8017/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_content_settings_unittest.cc (right):

http://codereview.chromium.org/7229012/diff/8017/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_content_settings_unittest.cc:19: {
"file:///foo/bar/baz", "file:///foo/bar/baz" },
On 2011/06/24 18:53:58, Matt Perry wrote:
> could you add a few more cases (test port and https)? and another test for
error
> cases (http://foo.com/path and file://with/wildcards*)

Done.

http://codereview.chromium.org/7229012/diff/8017/chrome/common/extensions/url...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7229012/diff/8017/chrome/common/extensions/url...
chrome/common/extensions/url_pattern.h:19: // <host> := '*' | '*.' <anychar
except '/' and '*'>+
On 2011/06/24 18:53:58, Matt Perry wrote:
> add a mention of the <port> here, that explains when it is allowed

Done.

[Sam Kerner (Chrome), 2011-06-28 18:12:39]

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:85: int parsed_port;
I think you need to set an intial value here (int parsed_port = -1) to avoid one
of our builders giving a compile error.

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:203: size_t port_pos = host_.find(':');
Pattern http://foo.com:123/* has a host of "foo.com:123" with PARSE_LENIENT, and
"foo.com" with IGNORE_PORTS.  

Because ':' is not a valid character in a DNS host name, the old behavior means
the pattern never matched any host.  I am nervious about changing behavior,
because it means installed extensions will suddenly start doing things they did
not do before.  On the other hand, any developer who tried to load them in
chrome for the last few releases got an error.

Matt, what do you think?  The case this matters is a pattern in the manifest,
such as a host permission.

I think all the new code uses USE_PORTS, so you could change IGNORE_PORTS to the
old behavior and we could figure this out in a separate change.

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:26: //   ignored for |MatchesURL| and
|OverlapsWith| regardless.
Comment out of date?

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:101: ERROR_ON_PORTS,
Does IGNORE_PORTS mean http://foo.com:1234/* matches "http://foo.com/"?  How
about "http://foo.com:5678/"?  I other words, does "ignore" mean silently drop
the port?

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:115: PARSE_ERROR_HAS_COLON,  // Only
checked when strict checks are enabled.
This comment needs an update: "strict checks" is now ERROR_ON_PORTS.

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern_unittest.cc (right):

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern_unittest.cc:518:
EXPECT_FALSE(pattern2.MatchesURL(GURL("http://www.example.com:1234/foo")));
Consider testing a pattern with a * as the port.

[Matt Perry, 2011-06-28 21:37:07]

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:203: size_t port_pos = host_.find(':');
On 2011/06/28 18:12:39, Sam Kerner (Chrome) wrote:
> Pattern http://foo.com:123/* has a host of "foo.com:123" with PARSE_LENIENT,
and
> "foo.com" with IGNORE_PORTS.  
> 
> Because ':' is not a valid character in a DNS host name, the old behavior
means
> the pattern never matched any host.  I am nervious about changing behavior,
> because it means installed extensions will suddenly start doing things they
did
> not do before.  On the other hand, any developer who tried to load them in
> chrome for the last few releases got an error.
> 
> Matt, what do you think?  The case this matters is a pattern in the manifest,
> such as a host permission.
> 
> I think all the new code uses USE_PORTS, so you could change IGNORE_PORTS to
the
> old behavior and we could figure this out in a separate change.

I agree. It seems unlikely to matter, but to be on the safe side I think we
should keep the behavior of IGNORE_PORTS.

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:277: if (!MatchesPort(test.port()))
do you want to handle matching a "default port"? like if the pattern was
http://google.com:80/foo, does MatchesURL("http://google.com/foo") match it?

If so, you have to check if test.port() == -1 (or use EffectiveIntPort()).

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:337: if (!IsValidPort(test))
this test seems unnecessary. port_ must be a valid port, since we don't allow it
to be set otherwise, so if port_ == test, test is a valid port.

[bauerb at google, 2011-06-28 22:36:52]

Thanks for the review I'll work on the rest of the comments tomorrow.

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:85: int parsed_port;
On 2011/06/28 18:12:39, Sam Kerner (Chrome) wrote:
> I think you need to set an intial value here (int parsed_port = -1) to avoid
one
> of our builders giving a compile error.

No, StringToInt always sets its argument (to zero if there's a parsing error).

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:277: if (!MatchesPort(test.port()))
On 2011/06/28 21:37:07, Matt Perry wrote:
> do you want to handle matching a "default port"? like if the pattern was
> http://google.com:80/foo, does MatchesURL("http://google.com/foo") match it?
> 
> If so, you have to check if test.port() == -1 (or use EffectiveIntPort()).

Ooh! Yes, I want that, thanks!

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:101: ERROR_ON_PORTS,
On 2011/06/28 18:12:39, Sam Kerner (Chrome) wrote:
> Does IGNORE_PORTS mean http://foo.com:1234/* matches "http://foo.com/"?  How
> about "http://foo.com:5678/"?  I other words, does "ignore" mean silently drop
> the port?

Yes, just like it did with PARSE_LENIENT.

[Sam Kerner (Chrome), 2011-06-29 13:36:34]

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:85: int parsed_port;
On 2011/06/28 22:36:52, please use chromium account wrote:
> On 2011/06/28 18:12:39, Sam Kerner (Chrome) wrote:
> > I think you need to set an intial value here (int parsed_port = -1) to avoid
> one
> > of our builders giving a compile error.
> 
> No, StringToInt always sets its argument (to zero if there's a parsing error).

I am not saying your code is wrong.  I am saying that some compilers are not
smart enough to see that, and when you commit you will find that a builder
breaks with an error about parsed_port being used without being initialized.

[Bernhard Bauer, 2011-06-29 13:53:18]

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:85: int parsed_port;
On 2011/06/29 13:36:34, Sam Kerner (Chrome) wrote:
> On 2011/06/28 22:36:52, please use chromium account wrote:
> > On 2011/06/28 18:12:39, Sam Kerner (Chrome) wrote:
> > > I think you need to set an intial value here (int parsed_port = -1) to
avoid
> > one
> > > of our builders giving a compile error.
> > 
> > No, StringToInt always sets its argument (to zero if there's a parsing
error).
> 
> I am not saying your code is wrong.  I am saying that some compilers are not
> smart enough to see that, and when you commit you will find that a builder
> breaks with an error about parsed_port being used without being initialized.

Hm, fair enough.

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:203: size_t port_pos = host_.find(':');
On 2011/06/28 21:37:07, Matt Perry wrote:
> On 2011/06/28 18:12:39, Sam Kerner (Chrome) wrote:
> > Pattern http://foo.com:123/* has a host of "foo.com:123" with PARSE_LENIENT,
> and
> > "foo.com" with IGNORE_PORTS.  
> > 
> > Because ':' is not a valid character in a DNS host name, the old behavior
> means
> > the pattern never matched any host.  I am nervious about changing behavior,
> > because it means installed extensions will suddenly start doing things they
> did
> > not do before.  On the other hand, any developer who tried to load them in
> > chrome for the last few releases got an error.
> > 
> > Matt, what do you think?  The case this matters is a pattern in the
manifest,
> > such as a host permission.
> > 
> > I think all the new code uses USE_PORTS, so you could change IGNORE_PORTS to
> the
> > old behavior and we could figure this out in a separate change.
> 
> I agree. It seems unlikely to matter, but to be on the safe side I think we
> should keep the behavior of IGNORE_PORTS.

Done.

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:337: if (!IsValidPort(test))
On 2011/06/28 21:37:07, Matt Perry wrote:
> this test seems unnecessary. port_ must be a valid port, since we don't allow
it
> to be set otherwise, so if port_ == test, test is a valid port.

But we don't want a port wildcard to match an invalid port?

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:26: //   ignored for |MatchesURL| and
|OverlapsWith| regardless.
On 2011/06/28 18:12:39, Sam Kerner (Chrome) wrote:
> Comment out of date?

Done.

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:115: PARSE_ERROR_HAS_COLON,  // Only
checked when strict checks are enabled.
On 2011/06/28 18:12:39, Sam Kerner (Chrome) wrote:
> This comment needs an update: "strict checks" is now ERROR_ON_PORTS.

Done.

[Matt Perry, 2011-06-29 17:18:47]

LGTM

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7229012/diff/20003/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:337: if (!IsValidPort(test))
On 2011/06/29 13:53:18, Bernhard Bauer wrote:
> On 2011/06/28 21:37:07, Matt Perry wrote:
> > this test seems unnecessary. port_ must be a valid port, since we don't
allow
> it
> > to be set otherwise, so if port_ == test, test is a valid port.
> 
> But we don't want a port wildcard to match an invalid port?

Oh OK, fair enough.

[Sam Kerner (Chrome), 2011-06-30 02:36:52]

LGTM with one nit to add a comment.

http://codereview.chromium.org/7229012/diff/26001/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7229012/diff/26001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:28: //   host, which makes the pattern
effectively never match any URL.
Please point out that this seemingly silly behavior is needed because some users
have to maintain backward compatibility with the behavior patters used to have
before ports were parsed.