Readability review for joaodasilva

Readability review for joaodasilva

Added the URLBlacklistManager and the URLBlacklist filter matcher

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

[Joao da Silva, 2011-10-18 10:02:51]

Hi Yonatan,

This is related to http://b/5335748, and is a chromium CL.

Let me know if you can do this readability review. Thanks!

-Joao

[zunger, 2011-10-20 22:24:03]

Overall very clean.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
File chrome/browser/policy/url_blacklist_manager.cc (right):

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.cc:37: if (!list)
Better to say 'list == NULL' explicitly.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.cc:75:
blacklist_manager->SetBlacklist(blacklist);
Use explicit { }'s when the composite if statement takes up more than two lines.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.cc:94: SchemeFlag flag;
Declare variables immediately before first use.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.cc:126: it->path_prefix == path)
Use explicit { }'s when the hypothesis takes up more than 1 line.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.cc:137: if (block)
&c

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.cc:168: for (;;) {
while (1) is a bit more common in google3 code.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.cc:241: if (scheme.empty())
{ }'s

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
File chrome/browser/policy/url_blacklist_manager.h (right):

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.h:33: // against this set. The
filters are currently kept in memory.
What's the thread-safety of this class?

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.h:39: // URLs matching |filter| will
be blocked.
Are these exact URLs? Is any normalization done?

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.h:76: struct PathFilter {
You can move this entire declaration into the .cc file and just forward-declare
here.

[Joao da Silva, 2011-10-21 10:19:47]

Thanks for the review, please have another look.

The CL has been rebased against chromium's trunk. Tasks* have been replaced with
base::Bind, type declarations were moved a bit around, and methods were
re-ordered in the .cc file to follow declaration order in the .h file.

I've removed the extra newlines so that the CL is commit-able; feel free to add
comments to the unit test file in patch set 1.

All the comments were still relevant and have been addressed.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
File chrome/browser/policy/url_blacklist_manager.cc (right):

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.cc:37: if (!list)
On 2011/10/20 22:24:03, zunger wrote:
> Better to say 'list == NULL' explicitly.

Done.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.cc:75:
blacklist_manager->SetBlacklist(blacklist);
On 2011/10/20 22:24:03, zunger wrote:
> Use explicit { }'s when the composite if statement takes up more than two
lines.

Done.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.cc:94: SchemeFlag flag;
On 2011/10/20 22:24:03, zunger wrote:
> Declare variables immediately before first use.

Done.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.cc:126: it->path_prefix == path)
On 2011/10/20 22:24:03, zunger wrote:
> Use explicit { }'s when the hypothesis takes up more than 1 line.

Done.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.cc:137: if (block)
On 2011/10/20 22:24:03, zunger wrote:
> &c

Not clear what is meant here; I've added explicit { } since the composite if
statement takes more than two lines, as mentioned above.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.cc:168: for (;;) {
On 2011/10/20 22:24:03, zunger wrote:
> while (1) is a bit more common in google3 code.

Done.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.cc:241: if (scheme.empty())
On 2011/10/20 22:24:03, zunger wrote:
> { }'s

Done.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
File chrome/browser/policy/url_blacklist_manager.h (right):

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.h:33: // against this set. The
filters are currently kept in memory.
On 2011/10/20 22:24:03, zunger wrote:
> What's the thread-safety of this class?

It's not thread safe. After setting up all the filters with Allow and Block,
concurrent calls to IsURLBlocked are safe though.

Instances are currently only owned by a URLBlacklistManager on a specific thread
(IO). This class should be private and in the .cc file, but the declaration is
here for testing.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.h:39: // URLs matching |filter| will
be blocked.
On 2011/10/20 22:24:03, zunger wrote:
> Are these exact URLs? Is any normalization done?

Most URLs are valid filters, with a couple of exceptions. The format is
documented for administrators at
http://www.chromium.org/administrators/url-blacklist-filter-format ; I've left a
comment here pointing to that document.

http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_black...
chrome/browser/policy/url_blacklist_manager.h:76: struct PathFilter {
On 2011/10/20 22:24:03, zunger wrote:
> You can move this entire declaration into the .cc file and just
forward-declare
> here.

Done.

[zunger, 2011-10-21 17:02:10]

So I have no idea how to use this damned Chromium review tool. What do I
need to do in order to see the latest
changes you've made? All I can see are your new comments, against the old
source file.

On Fri, Oct 21, 2011 at 03:19, <joaodasilva@chromium.org> wrote:

> Thanks for the review, please have another look.
>
> The CL has been rebased against chromium's trunk. Tasks* have been replaced
> with
> base::Bind, type declarations were moved a bit around, and methods were
> re-ordered in the .cc file to follow declaration order in the .h file.
>
> I've removed the extra newlines so that the CL is commit-able; feel free to
> add
> comments to the unit test file in patch set 1.
>
> All the comments were still relevant and have been addressed.
>
>
>
> http://codereview.chromium.**org/7930011/diff/1/chrome/**
>
browser/policy/url_blacklist_**manager.cc<http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_blacklist_manager.cc>
> File chrome/browser/policy/url_**blacklist_manager.cc (right):
>
> http://codereview.chromium.**org/7930011/diff/1/chrome/**
>
browser/policy/url_blacklist_**manager.cc#newcode37<http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_blacklist_manager.cc#newcode37>
> chrome/browser/policy/url_**blacklist_manager.cc:37: if (!list)
> On 2011/10/20 22:24:03, zunger wrote:
>
>> Better to say 'list == NULL' explicitly.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7930011/diff/1/chrome/**
>
browser/policy/url_blacklist_**manager.cc#newcode75<http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_blacklist_manager.cc#newcode75>
> chrome/browser/policy/url_**blacklist_manager.cc:75:
> blacklist_manager->**SetBlacklist(blacklist);
> On 2011/10/20 22:24:03, zunger wrote:
>
>> Use explicit { }'s when the composite if statement takes up more than
>>
> two lines.
>
> Done.
>
>
> http://codereview.chromium.**org/7930011/diff/1/chrome/**
>
browser/policy/url_blacklist_**manager.cc#newcode94<http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_blacklist_manager.cc#newcode94>
> chrome/browser/policy/url_**blacklist_manager.cc:94: SchemeFlag flag;
> On 2011/10/20 22:24:03, zunger wrote:
>
>> Declare variables immediately before first use.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7930011/diff/1/chrome/**
>
browser/policy/url_blacklist_**manager.cc#newcode126<http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_blacklist_manager.cc#newcode126>
> chrome/browser/policy/url_**blacklist_manager.cc:126: it->path_prefix ==
> path)
> On 2011/10/20 22:24:03, zunger wrote:
>
>> Use explicit { }'s when the hypothesis takes up more than 1 line.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7930011/diff/1/chrome/**
>
browser/policy/url_blacklist_**manager.cc#newcode137<http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_blacklist_manager.cc#newcode137>
> chrome/browser/policy/url_**blacklist_manager.cc:137: if (block)
> On 2011/10/20 22:24:03, zunger wrote:
>
>> &c
>>
>
> Not clear what is meant here; I've added explicit { } since the
> composite if statement takes more than two lines, as mentioned above.
>
>
> http://codereview.chromium.**org/7930011/diff/1/chrome/**
>
browser/policy/url_blacklist_**manager.cc#newcode168<http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_blacklist_manager.cc#newcode168>
> chrome/browser/policy/url_**blacklist_manager.cc:168: for (;;) {
> On 2011/10/20 22:24:03, zunger wrote:
>
>> while (1) is a bit more common in google3 code.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7930011/diff/1/chrome/**
>
browser/policy/url_blacklist_**manager.cc#newcode241<http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_blacklist_manager.cc#newcode241>
> chrome/browser/policy/url_**blacklist_manager.cc:241: if (scheme.empty())
> On 2011/10/20 22:24:03, zunger wrote:
>
>> { }'s
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7930011/diff/1/chrome/**
>
browser/policy/url_blacklist_**manager.h<http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_blacklist_manager.h>
> File chrome/browser/policy/url_**blacklist_manager.h (right):
>
> http://codereview.chromium.**org/7930011/diff/1/chrome/**
>
browser/policy/url_blacklist_**manager.h#newcode33<http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_blacklist_manager.h#newcode33>
> chrome/browser/policy/url_**blacklist_manager.h:33: // against this set.
> The filters are currently kept in memory.
> On 2011/10/20 22:24:03, zunger wrote:
>
>> What's the thread-safety of this class?
>>
>
> It's not thread safe. After setting up all the filters with Allow and
> Block, concurrent calls to IsURLBlocked are safe though.
>
> Instances are currently only owned by a URLBlacklistManager on a
> specific thread (IO). This class should be private and in the .cc file,
> but the declaration is here for testing.
>
>
> http://codereview.chromium.**org/7930011/diff/1/chrome/**
>
browser/policy/url_blacklist_**manager.h#newcode39<http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_blacklist_manager.h#newcode39>
> chrome/browser/policy/url_**blacklist_manager.h:39: // URLs matching
> |filter| will be blocked.
> On 2011/10/20 22:24:03, zunger wrote:
>
>> Are these exact URLs? Is any normalization done?
>>
>
> Most URLs are valid filters, with a couple of exceptions. The format is
> documented for administrators at
>
http://www.chromium.org/**administrators/url-blacklist-**filter-format<http:/...;
> I've left a comment here pointing to that document.
>
>
> http://codereview.chromium.**org/7930011/diff/1/chrome/**
>
browser/policy/url_blacklist_**manager.h#newcode76<http://codereview.chromium.org/7930011/diff/1/chrome/browser/policy/url_blacklist_manager.h#newcode76>
> chrome/browser/policy/url_**blacklist_manager.h:76: struct PathFilter {
> On 2011/10/20 22:24:03, zunger wrote:
>
>> You can move this entire declaration into the .cc file and just
>>
> forward-declare
>
>> here.
>>
>
> Done.
>
>
http://codereview.chromium.**org/7930011/<http://codereview.chromium.org/7930...
>

[Joao da Silva, 2011-10-21 18:43:23]

The two patch sets can be seen at http://codereview.chromium.org/7930011/ . The
email only shows the comments added as replies.

Due to the rebase, there are some changes unrelated to this review that make the
"Delta from patch set" view a bit confusing for now. The "Side by side diff" of
the 2nd patch set shows the latest changes, and I think it's the easiest to see
now.

[zunger, 2011-10-21 19:38:41]

lgtm

[commit-bot: I haz the power, 2011-10-22 10:42:25]

No LGTM from valid reviewers yet.

[Joao da Silva, 2011-10-22 10:48:11]

Wondering if I can lgtm a CL authored by me.