Correct sqlite wrapper behavior on systems where wchar_t is UTF-32,

Correct sqlite wrapper behavior on systems where wchar_t is UTF-32,
for example Linux.

The problem was that old code assumed wstring is UTF-16, which resulted
in string corruption on Linux. I actually tested it on browser/history
unit tests, see http://codereview.chromium.org/18758.

[Evan Martin, 2009-01-26 18:24:11]

This looks great to me, but please wait for Brett to sign off on it because he
is the sqlite master.

http://codereview.chromium.org/18805/diff/1/9
File chrome/common/sqlite_utils.cc (right):

http://codereview.chromium.org/18805/diff/1/9#newcode401
Line 401: str->assign(s ? UTF16ToWide(s) : std::wstring(L""));
Should this test WCHAR_IS_UTF32 and such to avoid this conversion on Windows?

http://codereview.chromium.org/18805/diff/1/10
File chrome/common/sqlite_utils.h (right):

http://codereview.chromium.org/18805/diff/1/10#newcode259
Line 259: static_cast<int>(in_utf16.length()), SQLITE_TRANSIENT);
If we're going to convert, should use UTF-8 strings on Linux, I think.  SQLite
supports databases in any encoding, and does string conversions on the other
side of these function calls.

http://www.sqlite.org/version3.html -- discussing of utf-16

http://www.sqlite.org/pragma.html -- you can use "PRAGMA encoding;" to check
what the native encoding is.  I believe we use UTF-8 in Chrome databases; Brett
would know for sure.

[brettw, 2009-01-26 19:00:34]

http://codereview.chromium.org/18805/diff/1/9
File chrome/common/sqlite_utils.cc (right):

http://codereview.chromium.org/18805/diff/1/9#newcode199
Line 199: int SQLStatement::prepare16(sqlite3* db, const char16* sql, int
sql_len) {
Nobody calls this function, I would just delete it. It would be difficult to
call in cross platform code since we usually use literals for SQL.

http://codereview.chromium.org/18805/diff/1/9#newcode307
Line 307: const char16* SQLStatement::column_name16(int index) {
Nobody uses this either.

http://codereview.chromium.org/18805/diff/1/9#newcode401
Line 401: str->assign(s ? UTF16ToWide(s) : std::wstring(L""));
This function just returns its parameter on Windows, so I think it's OK.

http://codereview.chromium.org/18805/diff/1/10
File chrome/common/sqlite_utils.h (right):

http://codereview.chromium.org/18805/diff/1/10#newcode259
Line 259: static_cast<int>(in_utf16.length()), SQLITE_TRANSIENT);
I agree with Evan. Our databases are actually all in UTF-8, so even the Windows
one converts to UTF-8 after the call.

[Paweł Hajdan Jr., 2009-01-30 10:13:18]

I removed unused functions, switched to UTF-8, and applied Brett's fix for
crashing windows unit_tests.exe (verified on trybot).

Please look.

[Dean McNamee, 2009-01-30 10:48:35]

I have a few random comments, that aren't really part of your review, but I just
noticed it while we're talking about this code.  I think some of them are small
style / comment things that would be easy to clean up in this review.  Others
are probably bigger and not important.

I'll wait for Brett to give the word on this review.

Thanks

http://codereview.chromium.org/18805/diff/605/423
File chrome/browser/history/url_database.cc (right):

http://codereview.chromium.org/18805/diff/605/423#newcode244
Line 244: // strings.
I didn't understand this new comment.

http://codereview.chromium.org/18805/diff/605/423#newcode246
Line 246: end_query.push_back(std::numeric_limits<unsigned char>::max());
Brett:  Random thought.  Does this actually work for all input?  If my prefix
was a\xff, wouldn't the end key be a\xff\xff, meaning it wouldn't find something
like a\xff\xff\xff as a prefix?  Anyway, not a problem in our case, but it
wouldn't be too much work to actually do it completely correctly?

http://codereview.chromium.org/18805/diff/605/423#newcode248
Line 248: statement->bind_string(0, WideToUTF8(prefix));
Not a huge deal, buy maybe just WideToUTF8 prefix once?

http://codereview.chromium.org/18805/diff/605/428
File chrome/common/sqlite_utils.cc (right):

http://codereview.chromium.org/18805/diff/605/428#newcode375
Line 375: str->assign(s ? s : std::string(""));
You can just do std::string() instead of std::string("").  It will save on a
strlen("") and some other bits of code.

http://codereview.chromium.org/18805/diff/605/428#newcode387
Line 387: return static_cast<const char16*>( sqlite3_column_text16(stmt_, index)
);
no spacing around these ( )

http://codereview.chromium.org/18805/diff/605/428#newcode394
Line 394: str->assign(s ? UTF8ToWide(s) : std::wstring(L""));
same here, std::wstring()

http://codereview.chromium.org/18805/diff/605/429
File chrome/common/sqlite_utils.h (right):

http://codereview.chromium.org/18805/diff/605/429#newcode246
Line 246: // (sqlite just uses the length)
Brett: This comment kinda seems weird to me.  In the STLs I've looked at,
c_str() and data() are always the same.  I agree in using data() here, but
because it's the logical thing to do, not because it's an optimization...

[brettw, 2009-01-30 16:01:31]

LGTM if dean is happy with my suggestions.

http://codereview.chromium.org/18805/diff/605/423
File chrome/browser/history/url_database.cc (right):

http://codereview.chromium.org/18805/diff/605/423#newcode244
Line 244: // strings.
How about this: Use 8-bit strings for everything so we can be sure sqlite is
comparing everything in 8-bit mode. Otherwise, it will have to convert strings
either to UTF-8 or UTF-16 for comparison.

http://codereview.chromium.org/18805/diff/605/423#newcode246
Line 246: end_query.push_back(std::numeric_limits<unsigned char>::max());
The biggest value a UTF-8 leading byte can have is 11111110 (and in practice
it's 11110xxx). And the data in the database and the user prefix should always
be valid UTF-8. Does this make sense?

http://codereview.chromium.org/18805/diff/605/423#newcode248
Line 248: statement->bind_string(0, WideToUTF8(prefix));
Probably a good idea. How about making a new variable above for the utf-8 prefix
and using that in both places we do WideToUTF8(prefix).

http://codereview.chromium.org/18805/diff/605/429
File chrome/common/sqlite_utils.h (right):

http://codereview.chromium.org/18805/diff/605/429#newcode246
Line 246: // (sqlite just uses the length)
Yes, you're right. I don't think Pawel needs to fix it for this patch though.

[Paweł Hajdan Jr., 2009-01-30 18:59:15]

I think I fixed these issues. Evan, what do you think?

[Evan Martin, 2009-01-30 19:01:48]

LGTM