Let url filter test the scheme in urlContains/Equals/Prefix/Suffix criteria

Let url filter test the scheme in urlContains/Equals/Prefix/Suffix criteria


BUG=140238
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152067

[battre, 2012-08-14 13:20:17]

Hi.

Could either of you please review this?

Thanks,
Dominic

[Yoyo Zhou, 2012-08-14 15:40:10]

LGTM, for the most part.

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

https://chromiumcodereview.appspot.com/10823313/diff/7001/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher_unittest.cc:329: GURL
gurl("https://www.google.com:1234/webhp?sourceid=chrome-instant&ie=UTF-8"
Hmm, you added a port here, but none of the tests except for the equals test
would be affected by the port being removed or not. How about an
EXPECT_TRUE(...PrefixCondition("https://www.google.com/webhp"))
and
EXPECT_FALSE(...ContainsCondition(":1234"))

https://chromiumcodereview.appspot.com/10823313/diff/7001/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher_unittest.cc:341: EXPECT_FALSE(
Matches(factory.CreateURLPrefixCondition(
nit: extra space after (

[Aaron Boodman, 2012-08-15 04:07:13]

http://codereview.chromium.org/10823313/diff/7001/chrome/common/extensions/ap...
File chrome/common/extensions/api/events.json (right):

http://codereview.chromium.org/10823313/diff/7001/chrome/common/extensions/ap...
chrome/common/extensions/api/events.json:259: "description": "Matches if the URL
(without port and fragment identifier) contains a specified string.",
without port? really?? that is super unexpected.

[Yoyo Zhou, 2012-08-15 17:53:12]

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

https://chromiumcodereview.appspot.com/10823313/diff/7001/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher.cc:317: return kBeginningOfURL +
url.scheme() + content::kStandardSchemeSeparator +
By the way, I just learned about GURL::Replacements yesterday; it could be
useful here.

[battre, 2012-08-16 13:16:25]

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

https://chromiumcodereview.appspot.com/10823313/diff/7001/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher.cc:317: return kBeginningOfURL +
url.scheme() + content::kStandardSchemeSeparator +
On 2012/08/15 17:53:12, Yoyo Zhou wrote:
> By the way, I just learned about GURL::Replacements yesterday; it could be
> useful here.

I was aware of this. I cannot use the GURL::Replacements for
URLMatcherConditionFactory::CanonicalizeURLForComponentSearches (I cannot get
rid of the scheme nor am I sure my host name normalization would be acceptable).
I'd rather have the two URLMatcherConditionFactory::CanonicalizeURLFor...()
functions look similar than use GURL::Replacements here.

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

https://chromiumcodereview.appspot.com/10823313/diff/7001/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher_unittest.cc:329: GURL
gurl("https://www.google.com:1234/webhp?sourceid=chrome-instant&ie=UTF-8"
On 2012/08/14 15:40:11, Yoyo Zhou wrote:
> Hmm, you added a port here, but none of the tests except for the equals test
> would be affected by the port being removed or not. How about an
> EXPECT_TRUE(...PrefixCondition("https://www.google.com/webhp"))
> and
> EXPECT_FALSE(...ContainsCondition(":1234"))

Done.

https://chromiumcodereview.appspot.com/10823313/diff/7001/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher_unittest.cc:341: EXPECT_FALSE(
Matches(factory.CreateURLPrefixCondition(
On 2012/08/14 15:40:11, Yoyo Zhou wrote:
> nit: extra space after (

Done.

[battre, 2012-08-16 13:49:26]

Please have another look. I addressed Aaron's wish to support port numbers. I
wanted to prevent that every user needs to specify a rule to match
http://www.foo.com/bar and http://www.foo.com:80/bar. I have solved this by
normalizing the URLs so that they don't contain implicit port numbers.

Best regards,
Dominic

[Aaron Boodman, 2012-08-16 14:54:45]

api lgtm.

[Yoyo Zhou, 2012-08-16 17:52:32]

LGTM, a few nits.

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

https://chromiumcodereview.appspot.com/10823313/diff/4003/chrome/common/exten...
chrome/common/extensions/api/events.json:259: "description": "Matches if the URL
(without fragment identifier) contains a specified string. Port numbers are
stripped from the URL in case they match the default port number.",
nit: I would write "if" instead of "in case".

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

https://chromiumcodereview.appspot.com/10823313/diff/4003/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher.cc:93: // Step 1: as above, except
that the scheme is not removed.
and port

https://chromiumcodereview.appspot.com/10823313/diff/4003/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher.cc:101: // BU
http://www.example.com/index.html?search=foo EU
This comment seems to be at odds with the port changes.

[battre, 2012-08-16 18:29:16]

Thanks for catching the missing documentation updates.

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

https://chromiumcodereview.appspot.com/10823313/diff/4003/chrome/common/exten...
chrome/common/extensions/api/events.json:259: "description": "Matches if the URL
(without fragment identifier) contains a specified string. Port numbers are
stripped from the URL in case they match the default port number.",
On 2012/08/16 17:52:32, Yoyo Zhou wrote:
> nit: I would write "if" instead of "in case".

Done.

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

https://chromiumcodereview.appspot.com/10823313/diff/4003/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher.cc:93: // Step 1: as above, except
that the scheme is not removed.
On 2012/08/16 17:52:32, Yoyo Zhou wrote:
> and port

Done.

https://chromiumcodereview.appspot.com/10823313/diff/4003/chrome/common/exten...
chrome/common/extensions/matcher/url_matcher.cc:101: // BU
http://www.example.com/index.html?search=foo EU
On 2012/08/16 17:52:32, Yoyo Zhou wrote:
> This comment seems to be at odds with the port changes.

Done.

[commit-bot: I haz the power, 2012-08-16 18:29:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10823313/11003

[commit-bot: I haz the power, 2012-08-16 20:04:29]

Try job failure for 10823313-11003 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-16 21:53:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10823313/11003

[commit-bot: I haz the power, 2012-08-16 23:49:46]

Try job failure for 10823313-11003 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-17 07:55:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10823313/11003

[commit-bot: I haz the power, 2012-08-17 07:55:56]

Failed to apply patch for chrome/common/extensions/docs/samples.json:
While running patch -p1 --forward --force;
patching file chrome/common/extensions/docs/samples.json
Hunk #1 FAILED at 184.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/common/extensions/docs/samples.json.rej