String cleanup in sync code

Clean up the strings for sync code:
- Use FilePath for real file paths.
- Use std::string for PathString on Windows as well

TODO in a separate CL:
- Get rid of PathString typedef completely and directly use std::string everywhere.
- Use wchar_t in syncapi.h/.cc and get rid of sync_char16.

TEST=Existing tests are sufficient since this CL does a lot of code refactoring.
BUG=26342
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30970

[Munjal (Google), 2009-11-01 23:05:59]

Others on sync please feel free to code review too.

[ncarter (slow), 2009-11-02 23:44:45]

I <3 this changelist.

http://codereview.chromium.org/340055/diff/7019/184
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/340055/diff/7019/184#newcode247
Line 247: const SyncEntity& entry) {
AppendUTF8ToPathString did a bit of UTF-8 validation, and I think we want to do
that before trusting the byte sequence from the protobuf.  I dug into the
generated .pb.cc code, and while there are some references to how string fields
can only contain UTF-8, it looks like protobuf-lite doesn't validate this.  I
think it's an option if we use the full (much bigger) protobufs library; but
even then, it seems it's only done by default on debug builds.

This change reduces the amount of validation we do on Windows, but it does seem
there were some places where we didn't validate before.

Given the scope, I'd be fine with deferring this, or trying to upstream a patch
to protobuf that would make validation an option in protobuf-lite.  WDYT?

http://codereview.chromium.org/340055/diff/7019/173
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/340055/diff/7019/173#newcode100
Line 100: int result = base::strncasecmp(reinterpret_cast<const char *>(a),
Should we just go ahead and use this linux version everywhere, or does this have
to match some expectation elsewhere?

http://codereview.chromium.org/340055/diff/7019/173#newcode280
Line 280: return ::PathMatchSpecA(name_subst.c_str(), spec_subst.c_str());
Wow, this function is terrible.  I expect it won't survive the week.

http://codereview.chromium.org/340055/diff/7019/172
File chrome/browser/sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/340055/diff/7019/172#newcode214
Line 214: dir_.reset();
The goal here was to remove the file on TearDown after the test runs.  Did you
intend to change that?

http://codereview.chromium.org/340055/diff/7019/172#newcode901
Line 901: file_util::Delete(directory_manager_->GetSyncDataDatabasePath(),
true);
80 chars.

http://codereview.chromium.org/340055/diff/7019/195
File chrome/browser/sync/util/character_set_converters.cc (right):

http://codereview.chromium.org/340055/diff/7019/195#newcode9
Line 9: void TrimPathStringToValidCharacter(PathString* string) {
I see that this is only used for the conflict resolver, code that chron hoped to
remove as part of the fixit, so I'm fine with whatever for now.

http://codereview.chromium.org/340055/diff/7019/198
File chrome/browser/sync/util/character_set_converters_win.cc (right):

http://codereview.chromium.org/340055/diff/7019/198#newcode11
Line 11: std::string FilePathToUTF8(const FilePath& file_path) {
Should these live in base/string_util or base/file_path?

http://codereview.chromium.org/340055/diff/7019/194
File chrome/browser/sync/util/path_helpers.h (right):

http://codereview.chromium.org/340055/diff/7019/194#newcode15
Line 15: extern const char* const kPathSeparator;
More typical would be:

extern const char kPathSeparator[]

http://codereview.chromium.org/340055/diff/7019/201
File chrome/browser/sync/util/path_helpers_posix.cc (right):

http://codereview.chromium.org/340055/diff/7019/201#newcode30
Line 30: std::string get_cache_dir() {
Codesearch says that these (*_cache_dir) are unused.

http://codereview.chromium.org/340055/diff/7019/207
File chrome/browser/sync/util/query_helpers.cc (right):

http://codereview.chromium.org/340055/diff/7019/207#newcode58
Line 58: int SqliteOpen(const FilePath &filename, sqlite3** db) {
Swap '&' and ' '.

http://codereview.chromium.org/340055/diff/7019/207#newcode64
Line 64: #endif
#include "chrome/common/sqlite_utils.h" and use OpenSqliteDb.

http://codereview.chromium.org/340055/diff/7019/207#newcode167
Line 167: static_cast<const wchar_t*>(sqlite3_column_text16(statement, index)),
wchar_t is not UTF-16 on Mac/Linux, so this seems pretty unsafe.  Would it be
better either to remove this function, or make it string16?

http://codereview.chromium.org/340055/diff/7019/208
File chrome/browser/sync/util/query_helpers.h (right):

http://codereview.chromium.org/340055/diff/7019/208#newcode37
Line 37: void GetColumn(sqlite3_stmt*, int index, std::wstring* value);
Probably drop this instead.

http://codereview.chromium.org/340055/diff/7019/202
File chrome/browser/sync/util/sync_types.h (right):

http://codereview.chromium.org/340055/diff/7019/202#newcode27
Line 27: #define PSTR(s) PSTR_UGLY_DOUBLE_DEFINE_HACK(s)
We don't need the double-define hack -- that's only to delay preprocessor
evaluation of the stringification operator (##) in the original version.  All
platforms should be able to just do:

#define PSTR(s) (s)

http://codereview.chromium.org/340055/diff/7019/191
File chrome/browser/sync/util/user_settings.cc (right):

http://codereview.chromium.org/340055/diff/7019/191#newcode186
Line 186: const DWORD attrs = GetFileAttributesW(settings_path.value().c_str());
Is it necessary to use the W version?  UNICODE is defined for all of Chrome on
Windows, so I think it's equivalent to just doing GetFileAttributes, right? 
(I'm fuzzy here and I bet your MSFT foo is greater than mine)

[Munjal (Google), 2009-11-03 01:58:23]

http://codereview.chromium.org/340055/diff/7019/184
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/340055/diff/7019/184#newcode247
Line 247: const SyncEntity& entry) {
On 2009/11/02 23:44:45, nick wrote:
> AppendUTF8ToPathString did a bit of UTF-8 validation, and I think we want to
do
> that before trusting the byte sequence from the protobuf.  I dug into the
> generated .pb.cc code, and while there are some references to how string
fields
> can only contain UTF-8, it looks like protobuf-lite doesn't validate this.  I
> think it's an option if we use the full (much bigger) protobufs library; but
> even then, it seems it's only done by default on debug builds.
> 
> This change reduces the amount of validation we do on Windows, but it does
seem
> there were some places where we didn't validate before.
> 
> Given the scope, I'd be fine with deferring this, or trying to upstream a
patch
> to protobuf that would make validation an option in protobuf-lite.  WDYT?

That's a very good observation.

It seems like we weren't doing this validation on other platforms. So if we want
to validate as much as we did before, or more, we should probably do that in a
separate Cl on all platforms.

Adding an option to protobuf to do this sounds like the best way to do it.

http://codereview.chromium.org/340055/diff/7019/173
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/340055/diff/7019/173#newcode100
Line 100: int result = base::strncasecmp(reinterpret_cast<const char *>(a),
On 2009/11/02 23:44:45, nick wrote:
> Should we just go ahead and use this linux version everywhere, or does this
have
> to match some expectation elsewhere?

Done.

I thought about that and hesitated to do it in this CL since I didn't know what
could be the reason to have a separate implementation for Mac and LInux even
when PathString is wstring on Windows. I thought may be there eas some reason to
use the platform provided way of comparing strings like locale stuff etc. But we
use invariant locale and ignore case comparison anyway. So I think it's safe to
use Linux implementation for all three.

http://codereview.chromium.org/340055/diff/7019/173#newcode280
Line 280: return ::PathMatchSpecA(name_subst.c_str(), spec_subst.c_str());
On 2009/11/02 23:44:45, nick wrote:
> Wow, this function is terrible.  I expect it won't survive the week.

I so wanted to kill it; but we should do it in a separate CL since it has
dependencies.

http://codereview.chromium.org/340055/diff/7019/172
File chrome/browser/sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/340055/diff/7019/172#newcode214
Line 214: dir_.reset();
On 2009/11/02 23:44:45, nick wrote:
> The goal here was to remove the file on TearDown after the test runs.  Did you
> intend to change that?

Done.

I didn't mean to remove that.

http://codereview.chromium.org/340055/diff/7019/172#newcode901
Line 901: file_util::Delete(directory_manager_->GetSyncDataDatabasePath(),
true);
On 2009/11/02 23:44:45, nick wrote:
> 80 chars.

Done.

http://codereview.chromium.org/340055/diff/7019/195
File chrome/browser/sync/util/character_set_converters.cc (right):

http://codereview.chromium.org/340055/diff/7019/195#newcode9
Line 9: void TrimPathStringToValidCharacter(PathString* string) {
On 2009/11/02 23:44:45, nick wrote:
> I see that this is only used for the conflict resolver, code that chron hoped
to
> remove as part of the fixit, so I'm fine with whatever for now.

Yeah, I spent a bit of time figuring out references of this function, and
further reference chain, to see if I can get ir of it; like I did for every
function that deals with scrubbing file names etc.

Great to know chrone is going to do that.

http://codereview.chromium.org/340055/diff/7019/198
File chrome/browser/sync/util/character_set_converters_win.cc (right):

http://codereview.chromium.org/340055/diff/7019/198#newcode11
Line 11: std::string FilePathToUTF8(const FilePath& file_path) {
On 2009/11/02 23:44:45, nick wrote:
> Should these live in base/string_util or base/file_path? 

That's what I talked to Evan/Mark (mentovai) about on a separate thread and they
think it's not the right thing to do. That's the reason the two methods in
FilePath that deal with conversion from wstring and back contain Hack in their
names.

FWIW, this function is only used to do V10 to V11 database upgrade since V10
database used to store file paths in UTF8 in the shares table. We can kill this
function, and it's reverse, when we don't need that version upgrade code
anymore.

On a side note, I wonder if we should just do stop and start syncing for
upgrading versions more easily and not maintain code for that.

http://codereview.chromium.org/340055/diff/7019/194
File chrome/browser/sync/util/path_helpers.h (right):

http://codereview.chromium.org/340055/diff/7019/194#newcode15
Line 15: extern const char* const kPathSeparator;
On 2009/11/02 23:44:45, nick wrote:
> More typical would be:
> 
> extern const char kPathSeparator[]
> 
> 

Done.
Mistake triggered by dumb copy/paste :(

http://codereview.chromium.org/340055/diff/7019/201
File chrome/browser/sync/util/path_helpers_posix.cc (right):

http://codereview.chromium.org/340055/diff/7019/201#newcode30
Line 30: std::string get_cache_dir() {
On 2009/11/02 23:44:45, nick wrote:
> Codesearch says that these (*_cache_dir) are unused.

Done.

So does Visual Assitl killed them.

I long for the day when MakePathComponentOSLegal will be killed too. I tried
that, but it has dependencies which probably can be removed.

http://codereview.chromium.org/340055/diff/7019/207
File chrome/browser/sync/util/query_helpers.cc (right):

http://codereview.chromium.org/340055/diff/7019/207#newcode58
Line 58: int SqliteOpen(const FilePath &filename, sqlite3** db) {
On 2009/11/02 23:44:45, nick wrote:
> Swap '&' and ' '.

Done.

Typo induced by working on two separate projects with opposite styles.

http://codereview.chromium.org/340055/diff/7019/207#newcode64
Line 64: #endif
On 2009/11/02 23:44:45, nick wrote:
> #include "chrome/common/sqlite_utils.h" and use OpenSqliteDb.

Done. Nice find.

http://codereview.chromium.org/340055/diff/7019/207#newcode167
Line 167: static_cast<const wchar_t*>(sqlite3_column_text16(statement, index)),
On 2009/11/02 23:44:45, nick wrote:
> wchar_t is not UTF-16 on Mac/Linux, so this seems pretty unsafe.  Would it be
> better either to remove this function, or make it string16?

Good point. Right now the only place this function is called on is on Windows
where wstring is utf16. But it is still dangerous to keep it. So I made it a
string16 function.

http://codereview.chromium.org/340055/diff/7019/202
File chrome/browser/sync/util/sync_types.h (right):

http://codereview.chromium.org/340055/diff/7019/202#newcode27
Line 27: #define PSTR(s) PSTR_UGLY_DOUBLE_DEFINE_HACK(s)
On 2009/11/02 23:44:45, nick wrote:
> We don't need the double-define hack -- that's only to delay preprocessor
> evaluation of the stringification operator (##) in the original version.  All
> platforms should be able to just do:
> 
> #define PSTR(s) (s)

Done.

http://codereview.chromium.org/340055/diff/7019/191
File chrome/browser/sync/util/user_settings.cc (right):

http://codereview.chromium.org/340055/diff/7019/191#newcode186
Line 186: const DWORD attrs = GetFileAttributesW(settings_path.value().c_str());
On 2009/11/02 23:44:45, nick wrote:
> Is it necessary to use the W version?  UNICODE is defined for all of Chrome on
> Windows, so I think it's equivalent to just doing GetFileAttributes, right? 
> (I'm fuzzy here and I bet your MSFT foo is greater than mine)

Done. Yeah, it's safe to not specify W explicitly since it will fail to compile
if UNICODE is not defined anyway.

[ncarter (slow), 2009-11-03 19:48:12]

LGTM