Safe browsing cleanup:...

Safe browsing cleanup:

* wstring -> FilePath
* create versions of WriteFile/ReadFile with FilePath signatures


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

[Evan Stade, 2009-01-28 01:26:44]

note addition of ToString() function in file_path. I'm guessing there's a good
reason it doesn't already exist, but I don't know what it is.

[Evan Martin, 2009-01-28 01:41:15]

The reason we don't have ToString() that returns UTF-8 is that Windows normally
doesn't handle UTF-8 in paths.  So I doubt any other code would use it.

If you want to avoid string conversions in the places where you used ToString, I
would suggest making a wrapper for sqlite3_open that calls sqlite3_open or
sqlite3_open16 for Windows.  I think we already have a file with sqlite-related
utilities.

http://codereview.chromium.org/19610/diff/47/61
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

http://codereview.chromium.org/19610/diff/47/61#newcode90
Line 90: virtual FilePath filename() const { return filename_; }
could be const FilePath& maybe?  I guess if it's overridden it can't.  Hrm, this
is a weird prototype.

http://codereview.chromium.org/19610/diff/47/56
File chrome/browser/safe_browsing/safe_browsing_database_bloom.cc (right):

http://codereview.chromium.org/19610/diff/47/56#newcode115
Line 115: if (sqlite3_open(filename_.ToString().c_str(), &db_) != SQLITE_OK) {
The original code normally wouldn't have worked on Windows, except that sqlite
specially handles it:

"Note to Windows users: The encoding used for the filename argument of
sqlite3_open() and sqlite3_open_v2() must be UTF-8, not whatever codepage is
currently defined. Filenames containing international characters must be
converted to UTF-8 prior to passing them into sqlite3_open() or
sqlite3_open_v2()."

http://codereview.chromium.org/19610/diff/47/63
File chrome/common/chrome_constants.cc (right):

http://codereview.chromium.org/19610/diff/47/63#newcode9
Line 9: #define FPL FILE_PATH_LITERAL
I appreciate the idea, but we don't do this elsewhere because it makes code hard
to read.

[Evan Martin, 2009-01-28 01:41:35]

PS, relevant docs: http://www.sqlite.org/capi3ref.html#sqlite3_open

[Evan Stade, 2009-01-28 02:06:38]

Aha, thanks for the link.

For the future though, there are many places where filepaths or parts thereof
need to be used as URLs (net/, browser/url_fixer_upper/, browser/autocomplete/,
etc.). Currently we sprinkle ifdefs... but is this really the best solution?

http://codereview.chromium.org/19610/diff/47/63
File chrome/common/chrome_constants.cc (right):

http://codereview.chromium.org/19610/diff/47/63#newcode9
Line 9: #define FPL FILE_PATH_LITERAL
On 2009/01/28 01:41:15, Evan Martin wrote:
> I appreciate the idea, but we don't do this elsewhere because it makes code
hard
> to read.

There are actually some places where we do this (mainly unit tests that have
long lists of test cases/expected results). I personally think it's worth it
since otherwise this file will end up with about twice as many lines. But if you
feel strongly I'll change it.

[Evan Stade, 2009-01-28 02:33:14]

updated.

http://codereview.chromium.org/19610/diff/270/90
File chrome/common/sqlite_utils.h (right):

http://codereview.chromium.org/19610/diff/270/90#newcode315
Line 315: // TODO(estade): wrap the following in namespace.
comment should read:

TODO(estade): wrap the following static functions in a namespace.

[Evan Martin, 2009-01-28 02:41:00]

cool, lgtm

http://codereview.chromium.org/19610/diff/270/90
File chrome/common/sqlite_utils.h (right):

http://codereview.chromium.org/19610/diff/270/90#newcode317
Line 317: int OpenSqliteDb(const FilePath& filepath, sqlite3** database);
Can you document this function, and in particular the return value?  (Just
saying "see sqlite3_open docs" is sufficient for the latter.)