Also match against the query string if present.

Also match against the query string if present.

BUG=none
TEST=BlacklistTest.QueryStringMatch

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

[Paweł Hajdan Jr., 2010-01-08 09:22:23]

http://codereview.chromium.org/523137/diff/1/2
File chrome/browser/privacy_blacklist/blacklist.cc (right):

http://codereview.chromium.org/523137/diff/1/2#newcode191
chrome/browser/privacy_blacklist/blacklist.cc:191: if (Matches((*i)->pattern(),
url_spec)) {
Can we just make it possible to pass url.spec() here? Or let Matches take GURL?

http://codereview.chromium.org/523137/diff/1/3
File chrome/browser/privacy_blacklist/blacklist_unittest.cc (right):

http://codereview.chromium.org/523137/diff/1/3#newcode172
chrome/browser/privacy_blacklist/blacklist_unittest.cc:172: TEST(BlacklistTest,
QueryStringMatch) {
Could you just merge this test into the test below?

[jochen (gone - plz use gerrit), 2010-01-08 10:35:28]

I've merged the unit test. This required changing sample test data and adopting
the BlacklistIO unit test.

http://codereview.chromium.org/523137/diff/1/2
File chrome/browser/privacy_blacklist/blacklist.cc (right):

http://codereview.chromium.org/523137/diff/1/2#newcode191
chrome/browser/privacy_blacklist/blacklist.cc:191: if (Matches((*i)->pattern(),
url_spec)) {
On 2010/01/08 09:22:23, Paweł Hajdan Jr. wrote:
> Can we just make it possible to pass url.spec() here? Or let Matches take
GURL?

Currently, we don't want to match against the protocol, login information
embedded in the url, or the port. Of course Matches() could take the GURL, but
then these string operations have to be repeated for every match.

http://codereview.chromium.org/523137/diff/1/3
File chrome/browser/privacy_blacklist/blacklist_unittest.cc (right):

http://codereview.chromium.org/523137/diff/1/3#newcode172
chrome/browser/privacy_blacklist/blacklist_unittest.cc:172: TEST(BlacklistTest,
QueryStringMatch) {
On 2010/01/08 09:22:23, Paweł Hajdan Jr. wrote:
> Could you just merge this test into the test below?

I'd rather merge it in the test above, since the test below tests
Blacklist::Entry::Match and not Blacklist::findMatch.

Done.

[Paweł Hajdan Jr., 2010-01-08 10:47:41]

+brettw for comments about how to use GURL the best way here

Brett, could you take a look at changes in blacklist.cc and suggest
improvements?

LGTM

[jochen (gone - plz use gerrit), 2010-01-08 12:42:37]

There was an unused variable "input2" in BlacklistTest.Generic which I removed.
Rest unchanged.

On 2010/01/08 10:47:41, Paweł Hajdan Jr. wrote:
> +brettw for comments about how to use GURL the best way here
> 
> Brett, could you take a look at changes in blacklist.cc and suggest
> improvements?
> 
> LGTM

[brettw, 2010-01-08 17:42:57]

http://codereview.chromium.org/523137/diff/1/2
File chrome/browser/privacy_blacklist/blacklist.cc (right):

http://codereview.chromium.org/523137/diff/1/2#newcode177
chrome/browser/privacy_blacklist/blacklist.cc:177: Blacklist::Match*
Blacklist::findMatch(const GURL& url) const {
Can you fix the style of this function name (should start with capital)?

http://codereview.chromium.org/523137/diff/1/2#newcode185
chrome/browser/privacy_blacklist/blacklist.cc:185: std::string url_spec =
url.host() + url.path();
I'm not entirely sure what you need here. This will produce something like
"google.com/foo?query" If that's what you need, then this code is probably best.

Normally, though, you would use ReplaceComponents for removing the things you
don't want.

I might suggest, however, having a separate function convert a GURL into the
type of string you want, which will make the conversion easier to re-use in the
future and also probably more clear in this code.

[jochen (gone - plz use gerrit), 2010-01-11 10:21:49]

I've renamed the findMatch function and moved the GURL to string conversion to a
separate function. Please review.

http://codereview.chromium.org/523137/diff/1/2
File chrome/browser/privacy_blacklist/blacklist.cc (right):

http://codereview.chromium.org/523137/diff/1/2#newcode177
chrome/browser/privacy_blacklist/blacklist.cc:177: Blacklist::Match*
Blacklist::findMatch(const GURL& url) const {
On 2010/01/08 17:42:58, brettw wrote:
> Can you fix the style of this function name (should start with capital)?

Done.

http://codereview.chromium.org/523137/diff/1/2#newcode185
chrome/browser/privacy_blacklist/blacklist.cc:185: std::string url_spec =
url.host() + url.path();
On 2010/01/08 17:42:58, brettw wrote:
> I'm not entirely sure what you need here. This will produce something like
> "google.com/foo?query" If that's what you need, then this code is probably
best.
> 
> Normally, though, you would use ReplaceComponents for removing the things you
> don't want.
> 
> I might suggest, however, having a separate function convert a GURL into the
> type of string you want, which will make the conversion easier to re-use in
the
> future and also probably more clear in this code.

Yes, that's exactly what we want. I've moved the code to a separate function as
suggested.

Done.

[Paweł Hajdan Jr., 2010-01-11 10:28:32]

Minor things. Also, please either update the .pbr file, or remove it from the
repo.

http://codereview.chromium.org/523137/diff/7001/7004
File chrome/browser/privacy_blacklist/blacklist.h (right):

http://codereview.chromium.org/523137/diff/7001/7004#newcode186
chrome/browser/privacy_blacklist/blacklist.h:186: // match against the host, the
path, and, if present, the query string.
I'm not sure if saying what we match against here is a good idea. It's likely to
get out of sync.

http://codereview.chromium.org/523137/diff/7001/7004#newcode187
chrome/browser/privacy_blacklist/blacklist.h:187: // Used only internally but
made static so that access can be given to
IMHO the comment from this point on is useless.

http://codereview.chromium.org/523137/diff/7001/7010
File chrome/browser/privacy_blacklist/blacklist_unittest.cc (right):

http://codereview.chromium.org/523137/diff/7001/7010#newcode228
chrome/browser/privacy_blacklist/blacklist_unittest.cc:228:
GURL("http://example.com/some/path")));
nit: please fix alignment in this line (the "G" in GURL should start right after
the "(" from the line above (i.e. right below "u" from "url_spec1").

http://codereview.chromium.org/523137/diff/7001/7010#newcode238
chrome/browser/privacy_blacklist/blacklist_unittest.cc:238:
GURL("http://example.com/"
nit: same for all of the above

[jochen (gone - plz use gerrit), 2010-01-11 10:46:38]

what do you mean by update the .pbr? It should reflect the changes to the .pbl
(otherwise the unit_tests would fail).

http://codereview.chromium.org/523137/diff/7001/7004
File chrome/browser/privacy_blacklist/blacklist.h (right):

http://codereview.chromium.org/523137/diff/7001/7004#newcode186
chrome/browser/privacy_blacklist/blacklist.h:186: // match against the host, the
path, and, if present, the query string.
On 2010/01/11 10:28:32, Paweł Hajdan Jr. wrote:
> I'm not sure if saying what we match against here is a good idea. It's likely
to
> get out of sync.

Done.

http://codereview.chromium.org/523137/diff/7001/7004#newcode187
chrome/browser/privacy_blacklist/blacklist.h:187: // Used only internally but
made static so that access can be given to
On 2010/01/11 10:28:32, Paweł Hajdan Jr. wrote:
> IMHO the comment from this point on is useless.

Done.

http://codereview.chromium.org/523137/diff/7001/7010
File chrome/browser/privacy_blacklist/blacklist_unittest.cc (right):

http://codereview.chromium.org/523137/diff/7001/7010#newcode228
chrome/browser/privacy_blacklist/blacklist_unittest.cc:228:
GURL("http://example.com/some/path")));
On 2010/01/11 10:28:32, Paweł Hajdan Jr. wrote:
> nit: please fix alignment in this line (the "G" in GURL should start right
after
> the "(" from the line above (i.e. right below "u" from "url_spec1").

Done.

http://codereview.chromium.org/523137/diff/7001/7010#newcode238
chrome/browser/privacy_blacklist/blacklist_unittest.cc:238:
GURL("http://example.com/"
On 2010/01/11 10:28:32, Paweł Hajdan Jr. wrote:
> nit: same for all of the above

Done.

[Paweł Hajdan Jr., 2010-01-11 10:54:18]

LGTM

On 2010/01/11 10:46:38, Jochen Eisinger wrote:
> what do you mean by update the .pbr? It should reflect the changes to the .pbl
> (otherwise the unit_tests would fail).

Oh, you indeed updated it. I was looking at the Rietveld's line-by-line diff and
it didn't show any differences, but the raw diff confirms that the file has been
updated. Sorry for the confusion.

[brettw, 2010-01-12 15:53:51]

LGTM, sorry I missed the reply.

http://codereview.chromium.org/523137/diff/12001/12004
File chrome/browser/privacy_blacklist/blacklist.h (right):

http://codereview.chromium.org/523137/diff/12001/12004#newcode186
chrome/browser/privacy_blacklist/blacklist.h:186: static std::string
GetURLAsString(const GURL& url);
I'd suggest a name a little more specific. The problem with this name is that it
seems like it just converts the URL to a string, which it doesn't. It's specific
to this component.

Maybe GetURLAsLookupString or maybe ...BlacklistString

[jochen (gone - plz use gerrit), 2010-01-12 16:30:19]

http://codereview.chromium.org/523137/diff/12001/12004
File chrome/browser/privacy_blacklist/blacklist.h (right):

http://codereview.chromium.org/523137/diff/12001/12004#newcode186
chrome/browser/privacy_blacklist/blacklist.h:186: static std::string
GetURLAsString(const GURL& url);
On 2010/01/12 15:53:51, brettw wrote:
> I'd suggest a name a little more specific. The problem with this name is that
it
> seems like it just converts the URL to a string, which it doesn't. It's
specific
> to this component.
> 
> Maybe GetURLAsLookupString or maybe ...BlacklistString

Done.