Design changes in browser/privacy_blacklist needed to integrate

Design changes in browser/privacy_blacklist needed to integrate
privacy blacklists with extensions.

- change I/O routines interface to be more clear
- distinguish between transient and permanent blacklists
- remove obsolete code (pbl_tool and blacklist error dialog)

BlacklistManager will be responsible for recompiling the persistent
blacklists. Errors will be reported via infobars.

TEST=Covered by unit_tests.
BUG=21541
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30449

[Paweł Hajdan Jr., 2009-10-26 18:03:10]

Please review. I can explain the rationale for the changes if needed.

The short story is that I decided that instead of keeping track of text
blacklists that are compiled in the binary blacklist in a file, I will only have
one BlacklistPathProvider, and distinguish between transient blacklists (for
extensions loaded via "Load extension") and permanent blacklists (for extensions
installed from a .crx).

The next steps are to use BlacklistManager in Profile, pause requests until the
blacklists have loaded (and handle errors), and make ExtensionsService a
BlacklistPathProvider.

[Aaron Boodman, 2009-10-26 21:18:49]

Seems OK to me. I did not review the parsing code in detail as it looks like it
is old.

Also I put some nits in the review regarding code that was already there. You
can ignore those if you want.

It looks like to integrate this with EM, it will require EM to dispatch
BM-specific notifications. I would prefer this worked the other way round, that
BM listed to EM-specific notifications (specifically: EXTENSION_LOADED,
EXTENSION_UNLOADED). This would make it more similar to other extensions
modules.

http://codereview.chromium.org/337025/diff/1/4
File chrome/browser/privacy_blacklist/blacklist.h (right):

http://codereview.chromium.org/337025/diff/1/4#newcode26
Line 26: // TODO(idanan): Implement this efficiently.
This comment looks like it is maybe out of date now? At least, there are lots of
comments sprinkled throughout the implementation talking about efficiency.

http://codereview.chromium.org/337025/diff/1/4#newcode37
Line 37: typedef std::vector<Entry*> EntryList;
It would be better to use linked_ptr here.

http://codereview.chromium.org/337025/diff/1/4#newcode61
Line 61: static unsigned int String2Attribute(const std::string&);
FYI, it isn't clear to me from the header what this method does at all.

http://codereview.chromium.org/337025/diff/1/4#newcode70
Line 70: void set_name(const std::string& name) { name_ = name; }
Each setter should go after its getter.

http://codereview.chromium.org/337025/diff/1/4#newcode72
Line 72: private:
Blank line between public/protected/private.

http://codereview.chromium.org/337025/diff/1/4#newcode82
Line 82: explicit Entry(const std::string& pattern, const Provider* provider);
explicit keyword only needed for single-argument constructors.

http://codereview.chromium.org/337025/diff/1/4#newcode97
Line 97: bool MatchType(const std::string&) const;
Nit: method names should be MatchesType()

http://codereview.chromium.org/337025/diff/1/4#newcode107
Line 107: void SwapTypes(std::vector<std::string>* types);
Again, not clear what this does from header.

http://codereview.chromium.org/337025/diff/1/5
File chrome/browser/privacy_blacklist/blacklist_io.cc (right):

http://codereview.chromium.org/337025/diff/1/5#newcode18
Line 18: 
whitespace

http://codereview.chromium.org/337025/diff/1/5#newcode44
Line 44: class TextBlacklistParser {
Class needs header comments.

http://codereview.chromium.org/337025/diff/1/5#newcode62
Line 62: error_string_ = ASCIIToUTF16("File I/O error. Check path and
permissions.");
80 cols

http://codereview.chromium.org/337025/diff/1/5#newcode62
Line 62: error_string_ = ASCIIToUTF16("File I/O error. Check path and
permissions.");
Why not just use std::string for this error?

http://codereview.chromium.org/337025/diff/1/5#newcode81
Line 81: while (cur < end) {
It would help to have a link to the format somewhere as a comment.

http://codereview.chromium.org/337025/diff/1/6
File chrome/browser/privacy_blacklist/blacklist_io.h (right):

http://codereview.chromium.org/337025/diff/1/6#newcode13
Line 13: class BlacklistIO {
Class needs header  comment.

[Paweł Hajdan Jr., 2009-10-27 17:26:19]

Patch updated.

[brettw, 2009-10-27 20:30:22]

http://codereview.chromium.org/337025/diff/5001/2006
File chrome/browser/privacy_blacklist/blacklist_io.cc (right):

http://codereview.chromium.org/337025/diff/5001/2006#newcode188
Line 188: // Read the providers
Check that your comments end in periods.

http://codereview.chromium.org/337025/diff/5001/2006#newcode195
Line 195: std::map<uint32, Blacklist::Provider*> provider_map;
This should be size_t to match the type of the loop below.

http://codereview.chromium.org/337025/diff/5001/2006#newcode214
Line 214: for (unsigned int i = 0; i < n; ++i) {
This should be size_t to match the type of 'n'. Can you also rename 'n' to
num_providers and num_entries (it's used for two different things).

http://codereview.chromium.org/337025/diff/5001/2009
File chrome/browser/privacy_blacklist/blacklist_manager.cc (right):

http://codereview.chromium.org/337025/diff/5001/2009#newcode110
Line 110: blacklist_manager()->OnBlacklistReadFinished(NULL);
Doesn't this get run on a background thread? It seems like you can't use the
blacklist object directly in this case. Am I understanding that correctly?

[Paweł Hajdan Jr., 2009-10-28 16:28:31]

Patch updated, please review.

[brettw, 2009-10-28 17:49:11]

LGTM

[Aaron Boodman, 2009-10-28 20:54:07]

lgtm