Load Multiple Blacklists...

Load Multiple Blacklists

Add the ability to load multiple text blacklists into the binary blacklist representation.

NOTE: Included Binary file means this change expects to fail on the trybots
in blacklist_io_test.cc:103. All other checks must pass.

BUG=16932
TEST=BlacklistIO*


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

[brettw, 2009-07-23 23:26:59]

http://codereview.chromium.org/159199/diff/18/2007
File chrome/browser/privacy_blacklist/blacklist.cc (right):

http://codereview.chromium.org/159199/diff/18/2007#newcode29
Line 29: const unsigned int Blacklist::kBlockAll;
We normally define the values in the .cc file only, to avoid this issue. I think
you should do that rather than this kind of confusing thing.

http://codereview.chromium.org/159199/diff/18/2005
File chrome/browser/privacy_blacklist/blacklist_io_unittest.cc (right):

http://codereview.chromium.org/159199/diff/18/2005#newcode52
Line 52: PathService::Get(chrome::DIR_TEST_DATA, &data_dir);
Seems like you should be using FilePath here. The wstring path functions are
dodgy on Linux.

[idanan, 2009-07-24 14:52:15]

Thanks for the quick review.

- itai

http://codereview.chromium.org/159199/diff/18/2007
File chrome/browser/privacy_blacklist/blacklist.cc (right):

http://codereview.chromium.org/159199/diff/18/2007#newcode29
Line 29: const unsigned int Blacklist::kBlockAll;
If it is OK with you, I'd rather keep it this way because it affects
performance. Constants in the declaration are treated as compile-time constants
and allow much more optimization including usage in constructs where constants
are required.

http://codereview.chromium.org/159199/diff/18/2005
File chrome/browser/privacy_blacklist/blacklist_io_unittest.cc (right):

http://codereview.chromium.org/159199/diff/18/2005#newcode52
Line 52: PathService::Get(chrome::DIR_TEST_DATA, &data_dir);
On 2009/07/23 23:26:59, brettw wrote:
> Seems like you should be using FilePath here. The wstring path functions are
> dodgy on Linux.

Done.

[brettw, 2009-07-24 16:07:22]

http://codereview.chromium.org/159199/diff/18/2007
File chrome/browser/privacy_blacklist/blacklist.cc (right):

http://codereview.chromium.org/159199/diff/18/2007#newcode29
Line 29: const unsigned int Blacklist::kBlockAll;
I have a hard time believing inlining these will have any different in
performance. Have you considered an enum instead? You could define it in the
header, and it would do better typechecking for you (plus you're not supposed to
use "unsigned").

[idanan_google.com, 2009-07-24 17:09:13]

Well, yes. It can affect performance. Of course, the impact depends on usage but
the compiler can do much more with a constant than an address (which is what
you get when it is not defined in the header).

Also from a C++ Standard perspective there are constructs which
require compile-time
constants which these qualify as.

An enum is also a compile time constants but I do not like they way
they are typed
and when using enums in bit-fields, one needs a different type to hold
the built field,
which is why unsigned int is the preferred way to store bit-field
attributes such as
those here or in std::ios for example.

- Itai

On Fri, Jul 24, 2009 at 12:07, <brettw@chromium.org> wrote:
>
> http://codereview.chromium.org/159199/diff/18/2007
> File chrome/browser/privacy_blacklist/blacklist.cc (right):
>
> http://codereview.chromium.org/159199/diff/18/2007#newcode29
> Line 29: const unsigned int Blacklist::kBlockAll;
> I have a hard time believing inlining these will have any different in
> performance. Have you considered an enum instead? You could define it in
> the header, and it would do better typechecking for you (plus you're not
> supposed to use "unsigned").
>
> http://codereview.chromium.org/159199
>

[brettw, 2009-07-24 19:47:07]

LGTM