Remove unique naming.

Remove unique naming from syncer! This reduces the complexity. Modify the index to store things with parent id and metahandle instead of parent id and name.

Add a template function for extracting name from proto buffers.

Refactor some unit tests to work without unique naming.
Remove path calls since they make no sense without unique names.

Remove find by parentID and names.

Remove unique name resolution from conflict resolver.

Remove syncable name class.

TEST=included unit tests

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

[chron_chromium.org, 2009-11-11 22:05:11]

Hi Tim!

This will not go in until after branch.

[its_nick_at_chromium_org, 2009-11-12 02:26:20]

Excellent.  I plan to review this as well.

 - nick

On Wed, Nov 11, 2009 at 2:05 PM, <chron@chromium.org> wrote:

> Reviewers: timsteele,
>
> Message:
> Hi Tim!
>
> This will not go in until after branch.
>
> Description:
> Remove unique naming from syncer! This reduces the complexity. Modify the
> index
> to store things with parent id and metahandle instead of parent id and
> name.
>
> Add a template function for extracting name from proto buffers.
>
> Refactor some unit tests to work without unique naming.
> Remove path calls since they make no sense without unique names.
>
> Remove find by parentID and names.
>
> Remove unique name resolution from conflict resolver.
>
> Remove syncable name class.
>
> Please review this at http://codereview.chromium.org/371029
>
> Affected files:
>  M chrome/browser/sync/engine/apply_updates_command_unittest.cc
>  M chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
>  M chrome/browser/sync/engine/build_commit_command.cc
>  A chrome/browser/sync/engine/conflict_resolution_view.h
>  M chrome/browser/sync/engine/conflict_resolver.h
>  M chrome/browser/sync/engine/conflict_resolver.cc
>  M chrome/browser/sync/engine/get_commit_ids_command.h
>  M chrome/browser/sync/engine/process_commit_response_command.cc
>  M chrome/browser/sync/engine/process_updates_command.cc
>  M chrome/browser/sync/engine/syncapi.cc
>  M chrome/browser/sync/engine/syncer.cc
>  M chrome/browser/sync/engine/syncer_proto_util.h
>  M chrome/browser/sync/engine/syncer_proto_util.cc
>  M chrome/browser/sync/engine/syncer_proto_util_unittest.cc
>  M chrome/browser/sync/engine/syncer_types.h
>  M chrome/browser/sync/engine/syncer_unittest.cc
>  M chrome/browser/sync/engine/syncer_util.h
>  M chrome/browser/sync/engine/syncer_util.cc
>  A chrome/browser/sync/engine/update_applicator.h
>  A chrome/browser/sync/engine/update_applicator.cc
>  M chrome/browser/sync/engine/verify_updates_command.cc
>  M chrome/browser/sync/syncable/directory_backing_store.cc
>  M chrome/browser/sync/syncable/syncable.h
>  M chrome/browser/sync/syncable/syncable.cc
>  M chrome/browser/sync/syncable/syncable_columns.h
>  M chrome/browser/sync/syncable/syncable_unittest.cc
>  M chrome/chrome.gyp
>  A chrome/test/sync/engine/test_syncable_utils.h
>  A chrome/test/sync/engine/test_syncable_utils.cc
>
>
>

[ncarter (slow), 2009-11-13 00:18:14]

This is a substantial accomplishment.  Great work.

http://codereview.chromium.org/371029/diff/2056/2061
File chrome/browser/sync/engine/conflict_resolver.cc (right):

http://codereview.chromium.org/371029/diff/2056/2061#newcode46
Line 46: // number of 16 bit characters in the UTF-16 representation.
This TODO is a non-issue with Munjal's changes to make PathString be
std::string.  The result is that .length() should be accurate  -- though to be
absolutely pedantic, we would validate the UTF-8 and normalize to form C.

http://codereview.chromium.org/371029/diff/2056/2061#newcode52
Line 52: maximum_name_too_short);
This assert is invalid -- the number 24 refers to the specific uniquification
scheme used by the old FindNewName.

http://codereview.chromium.org/371029/diff/2056/2061#newcode330
Line 330: LOG(INFO) << "Undoing our deletion of " << entry <<
I'm used to seeing << pushed to the next line.

http://codereview.chromium.org/371029/diff/2056/2061#newcode338
Line 338: }
This check is redundant to what's already done inside MutableEntry::Put.

http://codereview.chromium.org/371029/diff/2056/2064
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/371029/diff/2056/2064#newcode343
Line 343: server_entry, &server_name));
DCHECK gets compiled out on release builds, so this would be incorrect.

http://codereview.chromium.org/371029/diff/2056/2064#newcode345
Line 345: LOG(INFO) << "Server provided committed name:" << server_name;
I'd nix this log statement.

http://codereview.chromium.org/371029/diff/2056/2064#newcode347
Line 347: old_name != server_name) {
Should fit on one line.

http://codereview.chromium.org/371029/diff/2056/2065
File chrome/browser/sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/371029/diff/2056/2065#newcode138
Line 138: DCHECK(SyncerProtoUtil::NameFromSyncEntity(entry, &name));
Again, incorrect in release mode

http://codereview.chromium.org/371029/diff/2056/2065#newcode158
Line 158: // that much if things don't match up exactly.
I agree it shouldn't be a CHECK, but we really do care when this invariant isn't
met.  It's the first sign that sync is becoming unhinged.  It means that our
client has lost track of some server change, or vice versa.  I'd call it
LOG(ERROR...), which logging level behaves like a DCHECK, stopping the debugger
on a debug build, if present, while being nonfatal on a release build.

http://codereview.chromium.org/371029/diff/2056/2066
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/371029/diff/2056/2066#newcode246
Line 246: &data_->title);
Add a space.

http://codereview.chromium.org/371029/diff/2056/2066#newcode413
Line 413: // Atomically change the parent and name. This will fail if it would
Comment is no longer accurate.

http://codereview.chromium.org/371029/diff/2056/2066#newcode420
Line 420: DCHECK(entry_->Put(syncable::NON_UNIQUE_NAME, name));
It's not necessary anymore -- this method doesn't need to touch the name.  We
needed to do it before because the sync glue code (which didn't care about name
uniqueness) could have caused an inadvertent name clash as result of a move, and
so we had to do the MakeNonColliding junk.  The code as written before was
careful to preserve the non-unique name.

http://codereview.chromium.org/371029/diff/2056/2068
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/371029/diff/2056/2068#newcode241
Line 241: bool NameFromEntry(T entry, PathString* out_pathstring) {
The empty string is understood to be an invalid value for a name, so I'd use
that instead of a bool.

http://codereview.chromium.org/371029/diff/2056/2068#newcode258
Line 258: return NameFromEntry <const SyncEntity&> (entry, out_pathstring);
name is required, this will always return true.

http://codereview.chromium.org/371029/diff/2056/2068#newcode264
Line 264: return NameFromEntry <const CommitResponse_EntryResponse&> (entry,
out_pathstring);
I think the template hurts readability.

http://codereview.chromium.org/371029/diff/2056/2069
File chrome/browser/sync/engine/syncer_proto_util.h (right):

http://codereview.chromium.org/371029/diff/2056/2069#newcode61
Line 61: static bool NameFromSyncEntity(const SyncEntity& entry,
Boolean values saying "o noes invalid UTF-8" have proven to be pretty useless in
practice.  I think it's better to DCHECK at the point of the error, rather than
propagating it, and then proceed after substituting an appropriately truncated
or default value.

[ Ideally we'd put that logic as part of the generated proto parser, but a
thread I started suggests that the maintainers aren't in favor (there's some
validation, but it's debug-only and not supported at all in protobuf-lite). ]

This applies to the next function, too.

http://codereview.chromium.org/371029/diff/2056/2070
File chrome/browser/sync/engine/syncer_proto_util_unittest.cc (right):

http://codereview.chromium.org/371029/diff/2056/2070#newcode79
Line 79: const string one_name_string("Eggheadednesses");
PathString == string now, so this can be condensed.

http://codereview.chromium.org/371029/diff/2056/2070#newcode88
Line 88:
EXPECT_TRUE(SyncerProtoUtil::NameFromCommitEntryResponse(one_name_response,
&name_b));
80 chars.

http://codereview.chromium.org/371029/diff/2056/2070#newcode99
Line 99: const PathString second_one_name_path_string(PSTR("snakesonaplane"));
Consider being consistent with the pattern:
http://spreadsheets.google.com/ccc?key=0AqzQKOKy_xXfdE1teWowQ3lPZl9HVHZNVmFBM...

http://codereview.chromium.org/371029/diff/2056/2072
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/371029/diff/2056/2072#newcode71
Line 71: using syncable::NON_UNIQUE_NAME;
Sort.

http://codereview.chromium.org/371029/diff/2056/2072#newcode820
Line 820: PathString parent2_name = PSTR("A");
Can use string and no PSTR(), but it's fine.

http://codereview.chromium.org/371029/diff/2056/2072#newcode825
Line 825: MutableEntry parent(&wtrans, syncable::CREATE, wtrans.root_id(),
parent1_name);
80 chars (several places in this file).  Run lint.

http://codereview.chromium.org/371029/diff/2056/2072#newcode837
Line 837: MutableEntry parent2(&wtrans, syncable::CREATE, parent_id_,
parent2_name);
There's no reason for MutableEntry to take a title in the CREATE ctor anymore,
but I'm not convinced it's worth the effort to change.

http://codereview.chromium.org/371029/diff/2056/2072#newcode875
Line 875: GetOnlyEntryWithName(&rtrans, parent_id_, parent2_name);
You're not in namespace syncable, and you didn't import the syncable utils into
this namespace via a using-declaration.  So I'm not sure why this works.  Please
import all three and use them consistently without the syncable:: qualification.

http://codereview.chromium.org/371029/diff/2056/2072#newcode902
Line 902: // TODO(chron): This is an arbitrary local client value. Maybe we
should use NewLocalId?
Sounds good to me.  It should just work.  Of course, it's a little redundant,
because the CREATE ctor assigns a unique local ID already, but none of this
really impacts the test.

http://codereview.chromium.org/371029/diff/2056/2072#newcode918
Line 918: // This is an arbitrary local client value. Consider NewLocalId.
Redundant to earlier comment.

http://codereview.chromium.org/371029/diff/2056/2072#newcode979
Line 979: EXPECT_FALSE(SyncerStuck(state_.get()));
Whee!

http://codereview.chromium.org/371029/diff/2056/2072#newcode1100
Line 1100: EXPECT_TRUE(bad_parent.Get(IS_UNAPPLIED_UPDATE))
oops, nice catch.

http://codereview.chromium.org/371029/diff/2056/2072#newcode1198
Line 1198: 
Remove blank line.

http://codereview.chromium.org/371029/diff/2056/2072#newcode1267
Line 1267: int64 metahandle_entry;
Move so that it's as close as possible to its initialization.

http://codereview.chromium.org/371029/diff/2056/2072#newcode1346
Line 1346: VerifyTestDataInEntry(&trans, &entry);
You lost something valuable here: you no longer verify that a lookup via
folder_id (the original, pre-reunite ID) fails.

http://codereview.chromium.org/371029/diff/2056/2072#newcode1585
Line 1585: PSTR("bob_child"));
Why change this?

http://codereview.chromium.org/371029/diff/2056/2072#newcode1600
Line 1600: PSTR("bob"));
It fit on one line before.

http://codereview.chromium.org/371029/diff/2056/2072#newcode1624
Line 1624: bool TouchFredAndGingerInRoot(Directory* dir) {
It's unreferenced.

http://codereview.chromium.org/371029/diff/2056/2072#newcode1810
Line 1810: }
Worst test ever.

http://codereview.chromium.org/371029/diff/2056/2072#newcode1996
Line 1996: TEST_F(SyncerTest, DISABLED_FolderSwapCommit) {
I think we can delete these tests now.  They don't do anything interesting.

http://codereview.chromium.org/371029/diff/2056/2072#newcode2279
Line 2279: }
Much improved.

http://codereview.chromium.org/371029/diff/2056/2072#newcode2510
Line 2510: mock_server_->AddUpdateDirectory(bob_id, TestIdFactory::root(),
"bob", 1, 10);
ids_.root() is more compact: an option.

http://codereview.chromium.org/371029/diff/2056/2072#newcode2553
Line 2553: TEST_F(SyncerTest, DISABLED_ServerDeletingFolderWeHaveAnOpenEntryIn)
{
Don't comment it out -- that's a death sentence.  Actually, please try fixing it
up and running it.  You make have fixed it.

http://codereview.chromium.org/371029/diff/2056/2072#newcode2658
Line 2658: namespace FolderMoveDeleteRenameTest {
This isn't an appropriate use of (or name for) a namespace.  Better would be to
make this a test fixture that derives from SyncerTest.

http://codereview.chromium.org/371029/diff/2056/2072#newcode2691
Line 2691: FolderMoveDeleteRenameTest::bob_id_number);
Underindented (also below).

http://codereview.chromium.org/371029/diff/2056/2072#newcode3001
Line 3001: void DeleteSusanInRoot(Directory* dir) {
If you decide to transform the earlier case (MoveBobIntoID2) into a test
fixture, please go ahead and do the same here

http://codereview.chromium.org/371029/diff/2056/2073
File chrome/browser/sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/371029/diff/2056/2073#newcode361
Line 361: // TODO(chron): Is it OK to compare pathstring?
Yes, it's fine.

http://codereview.chromium.org/371029/diff/2056/2078
File chrome/browser/sync/syncable/directory_backing_store.cc (right):

http://codereview.chromium.org/371029/diff/2056/2078#newcode40
Line 40: static const int32 kCurrentDBVersion = 68;
When we push this, the version number bump from the resulting download will
cause a traffic spike of getupdates-from-timestamp-zeroes.  Which may be fine. 
It will be smoothed out by the average time until browser restart.

Let's talk it out.  How hard would the column-dropping migration be? 
http://www.sqlite.org/faq.html#q11

http://codereview.chromium.org/371029/diff/2056/2079
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/371029/diff/2056/2079#newcode348
Line 348: needle_.ref(META_HANDLE) = -1;
It's a signed value, so numeric_limits<int64>::min() would be better.

http://codereview.chromium.org/371029/diff/2056/2079#newcode354
Line 354: needle_.ref(META_HANDLE) = -1;
make the needle numeric_limits<int64>::max() and then just return the
upper_bound (no loop needed).  The edge cases will work out.

http://codereview.chromium.org/371029/diff/2056/2079#newcode379
Line 379: kernel_->parent_id_child_index;
Should fit on one line.

http://codereview.chromium.org/371029/diff/2056/2079#newcode458
Line 458: entry->ref(PARENT_ID) = new_parent_id;
return

http://codereview.chromium.org/371029/diff/2056/2079#newcode1031
Line 1031: : Entry(trans), write_transaction_(trans) {
Indent two more spaces; one initializer per line.

http://codereview.chromium.org/371029/diff/2056/2080
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/371029/diff/2056/2080#newcode134
Line 134: // Name, equivelant to bookmark name. Will be truncated by server.
misspelled "equivalent".  Don't say "bookmark name", because some sync items may
not be bookmarks in the future.  The "OS legal" bit is probably not worth
mentioning.

http://codereview.chromium.org/371029/diff/2056/2080#newcode360
Line 360: inline PathString GetName() const {
Are these still used?

http://codereview.chromium.org/371029/diff/2056/2080#newcode370
Line 370: inline bool ExistsOnClientBecauseDatabaseNameIsNonEmpty() const {
This check is still valid, I think.  Rename it since the concept of "Database
name" was an artifact of sanitization.

http://codereview.chromium.org/371029/diff/2056/2082
File chrome/browser/sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/371029/diff/2056/2082#newcode161
Line 161: dir.SaveChanges();
remove("SimpleTest.sqlite3");

http://codereview.chromium.org/371029/diff/2056/2082#newcode543
Line 543: EXPECT_TRUE(entry.Get(META_HANDLE) == entry_handle);
EXPECT_EQ, and put the expected value as the first argument.

http://codereview.chromium.org/371029/diff/2056/2084
File chrome/test/sync/engine/test_syncable_utils.cc (right):

http://codereview.chromium.org/371029/diff/2056/2084#newcode56
Line 56: CHECK(1 == CountEntriesWithName(rtrans, parent_id, name));
This is test code.  Use EXPECT instead of CHECK so that the test fails but
execution continues on to the next test.

[ncarter (slow), 2009-11-13 00:22:09]

http://codereview.chromium.org/371029/diff/2056/2066
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/371029/diff/2056/2066#newcode420
Line 420: DCHECK(entry_->Put(syncable::NON_UNIQUE_NAME, name));
On 2009/11/13 00:18:14, nick wrote:
> It's not necessary anymore -- this method doesn't need to touch the name.  We
> needed to do it before because the sync glue code (which didn't care about
name
> uniqueness) could have caused an inadvertent name clash as result of a move,
and
> so we had to do the MakeNonColliding junk.  The code as written before was
> careful to preserve the non-unique name.

Also -- this is another dcheck bug.

[chron_chromium.org, 2009-11-13 23:21:25]

Thanks for the review. I've changed callbacks to closures and made all other
fixes.

http://codereview.chromium.org/371029/diff/2056/2061
File chrome/browser/sync/engine/conflict_resolver.cc (right):

http://codereview.chromium.org/371029/diff/2056/2061#newcode46
Line 46: // number of 16 bit characters in the UTF-16 representation.
On 2009/11/13 00:18:14, nick wrote:
> This TODO is a non-issue with Munjal's changes to make PathString be
> std::string.  The result is that .length() should be accurate  -- though to be
> absolutely pedantic, we would validate the UTF-8 and normalize to form C.

Done.

http://codereview.chromium.org/371029/diff/2056/2061#newcode52
Line 52: maximum_name_too_short);
Removed this function.

http://codereview.chromium.org/371029/diff/2056/2061#newcode330
Line 330: LOG(INFO) << "Undoing our deletion of " << entry <<
On 2009/11/13 00:18:14, nick wrote:
> I'm used to seeing << pushed to the next line.

Done.

http://codereview.chromium.org/371029/diff/2056/2061#newcode338
Line 338: }
On 2009/11/13 00:18:14, nick wrote:
> This check is redundant to what's already done inside MutableEntry::Put.

Done.

http://codereview.chromium.org/371029/diff/2056/2064
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/371029/diff/2056/2064#newcode343
Line 343: server_entry, &server_name));
Removed DCHECK, altered function not to use bool.

http://codereview.chromium.org/371029/diff/2056/2064#newcode345
Line 345: LOG(INFO) << "Server provided committed name:" << server_name;
On 2009/11/13 00:18:14, nick wrote:
> I'd nix this log statement.

Done.

http://codereview.chromium.org/371029/diff/2056/2064#newcode347
Line 347: old_name != server_name) {
On 2009/11/13 00:18:14, nick wrote:
> Should fit on one line.

Done.

http://codereview.chromium.org/371029/diff/2056/2065
File chrome/browser/sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/371029/diff/2056/2065#newcode138
Line 138: DCHECK(SyncerProtoUtil::NameFromSyncEntity(entry, &name));
On 2009/11/13 00:18:14, nick wrote:
> Again, incorrect in release mode

Done.

http://codereview.chromium.org/371029/diff/2056/2065#newcode158
Line 158: // that much if things don't match up exactly.
Log error it is. In most of the cases that I've seen this, we should just take
the server side. I'll add a todo. If log error won't trigger in release mode
then this is fine.

http://codereview.chromium.org/371029/diff/2056/2066
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/371029/diff/2056/2066#newcode420
Line 420: DCHECK(entry_->Put(syncable::NON_UNIQUE_NAME, name));
On 2009/11/13 00:18:14, nick wrote:
> It's not necessary anymore -- this method doesn't need to touch the name.  We
> needed to do it before because the sync glue code (which didn't care about
name
> uniqueness) could have caused an inadvertent name clash as result of a move,
and
> so we had to do the MakeNonColliding junk.  The code as written before was
> careful to preserve the non-unique name.

Done.

http://codereview.chromium.org/371029/diff/2056/2066#newcode420
Line 420: DCHECK(entry_->Put(syncable::NON_UNIQUE_NAME, name));
On 2009/11/13 00:18:14, nick wrote:
> It's not necessary anymore -- this method doesn't need to touch the name.  We
> needed to do it before because the sync glue code (which didn't care about
name
> uniqueness) could have caused an inadvertent name clash as result of a move,
and
> so we had to do the MakeNonColliding junk.  The code as written before was
> careful to preserve the non-unique name.

Done.

http://codereview.chromium.org/371029/diff/2056/2068
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/371029/diff/2056/2068#newcode241
Line 241: bool NameFromEntry(T entry, PathString* out_pathstring) {
On 2009/11/13 00:18:14, nick wrote:
> The empty string is understood to be an invalid value for a name, so I'd use
> that instead of a bool.

Done.

http://codereview.chromium.org/371029/diff/2056/2068#newcode258
Line 258: return NameFromEntry <const SyncEntity&> (entry, out_pathstring);
On 2009/11/13 00:18:14, nick wrote:
> name is required, this will always return true.

Surprisingly invalid protobuffers are happily accepted usually. But I removed
the bool anyway.

http://codereview.chromium.org/371029/diff/2056/2068#newcode264
Line 264: return NameFromEntry <const CommitResponse_EntryResponse&> (entry,
out_pathstring);
On 2009/11/13 00:18:14, nick wrote:
> I think the template hurts readability.

Done.

http://codereview.chromium.org/371029/diff/2056/2069
File chrome/browser/sync/engine/syncer_proto_util.h (right):

http://codereview.chromium.org/371029/diff/2056/2069#newcode61
Line 61: static bool NameFromSyncEntity(const SyncEntity& entry,
On 2009/11/13 00:18:14, nick wrote:
> Boolean values saying "o noes invalid UTF-8" have proven to be pretty useless
in
> practice.  I think it's better to DCHECK at the point of the error, rather
than
> propagating it, and then proceed after substituting an appropriately truncated
> or default value.
> 
> [ Ideally we'd put that logic as part of the generated proto parser, but a
> thread I started suggests that the maintainers aren't in favor (there's some
> validation, but it's debug-only and not supported at all in protobuf-lite). ]
> 
> This applies to the next function, too.

Done.

http://codereview.chromium.org/371029/diff/2056/2070
File chrome/browser/sync/engine/syncer_proto_util_unittest.cc (right):

http://codereview.chromium.org/371029/diff/2056/2070#newcode79
Line 79: const string one_name_string("Eggheadednesses");
On 2009/11/13 00:18:14, nick wrote:
> PathString == string now, so this can be condensed.

Done.

http://codereview.chromium.org/371029/diff/2056/2070#newcode88
Line 88:
EXPECT_TRUE(SyncerProtoUtil::NameFromCommitEntryResponse(one_name_response,
&name_b));
On 2009/11/13 00:18:14, nick wrote:
> 80 chars.

Done.

http://codereview.chromium.org/371029/diff/2056/2070#newcode99
Line 99: const PathString second_one_name_path_string(PSTR("snakesonaplane"));
On 2009/11/13 00:18:14, nick wrote:
> Consider being consistent with the pattern:
>
http://spreadsheets.google.com/ccc?key=0AqzQKOKy_xXfdE1teWowQ3lPZl9HVHZNVmFBM...

Done.

http://codereview.chromium.org/371029/diff/2056/2072
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/371029/diff/2056/2072#newcode71
Line 71: using syncable::NON_UNIQUE_NAME;
On 2009/11/13 00:18:14, nick wrote:
> Sort.

I need to put this on the wall.
http://www.poster.net/sesamstrasse/sesamstrasse-sesame-street-alphabet-996898...

http://codereview.chromium.org/371029/diff/2056/2072#newcode820
Line 820: PathString parent2_name = PSTR("A");
On 2009/11/13 00:18:14, nick wrote:
> Can use string and no PSTR(), but it's fine.

Will string gut in the next pass.

http://codereview.chromium.org/371029/diff/2056/2072#newcode825
Line 825: MutableEntry parent(&wtrans, syncable::CREATE, wtrans.root_id(),
parent1_name);
On 2009/11/13 00:18:14, nick wrote:
> 80 chars (several places in this file).  Run lint.

Ran and fixed.

http://codereview.chromium.org/371029/diff/2056/2072#newcode837
Line 837: MutableEntry parent2(&wtrans, syncable::CREATE, parent_id_,
parent2_name);
On 2009/11/13 00:18:14, nick wrote:
> There's no reason for MutableEntry to take a title in the CREATE ctor anymore,
> but I'm not convinced it's worth the effort to change.

For the next fixit I think.

http://codereview.chromium.org/371029/diff/2056/2072#newcode875
Line 875: GetOnlyEntryWithName(&rtrans, parent_id_, parent2_name);
On 2009/11/13 00:18:14, nick wrote:
> You're not in namespace syncable, and you didn't import the syncable utils
into
> this namespace via a using-declaration.  So I'm not sure why this works. 
Please
> import all three and use them consistently without the syncable::
qualification.

It is a mystery. Done.

http://codereview.chromium.org/371029/diff/2056/2072#newcode902
Line 902: // TODO(chron): This is an arbitrary local client value. Maybe we
should use NewLocalId?
On 2009/11/13 00:18:14, nick wrote:
> Sounds good to me.  It should just work.  Of course, it's a little redundant,
> because the CREATE ctor assigns a unique local ID already, but none of this
> really impacts the test.

Done.

http://codereview.chromium.org/371029/diff/2056/2072#newcode918
Line 918: // This is an arbitrary local client value. Consider NewLocalId.
On 2009/11/13 00:18:14, nick wrote:
> Redundant to earlier comment.

Done.

http://codereview.chromium.org/371029/diff/2056/2072#newcode1267
Line 1267: int64 metahandle_entry;
On 2009/11/13 00:18:14, nick wrote:
> Move so that it's as close as possible to its initialization.

Done.

http://codereview.chromium.org/371029/diff/2056/2072#newcode1346
Line 1346: VerifyTestDataInEntry(&trans, &entry);
On 2009/11/13 00:18:14, nick wrote:
> You lost something valuable here: you no longer verify that a lookup via
> folder_id (the original, pre-reunite ID) fails.
> 
> 

Good catch.

http://codereview.chromium.org/371029/diff/2056/2072#newcode1585
Line 1585: PSTR("bob_child"));
On 2009/11/13 00:18:14, nick wrote:
> Why change this?

Done.

http://codereview.chromium.org/371029/diff/2056/2072#newcode1600
Line 1600: PSTR("bob"));
On 2009/11/13 00:18:14, nick wrote:
> It fit on one line before.

Done.

http://codereview.chromium.org/371029/diff/2056/2072#newcode1624
Line 1624: bool TouchFredAndGingerInRoot(Directory* dir) {
On 2009/11/13 00:18:14, nick wrote:
> It's unreferenced.

But it's so hard to bring myself to delete it... :P

Done.

http://codereview.chromium.org/371029/diff/2056/2072#newcode1810
Line 1810: }
On 2009/11/13 00:18:14, nick wrote:
> Worst test ever.
I beg to differ. THIS is the worst test ever:
http://cs/p?sa=N&cd=1&ct=rc#chrome/trunk/src/chrome/browser/sync/syncable/syn...

http://codereview.chromium.org/371029/diff/2056/2072#newcode1996
Line 1996: TEST_F(SyncerTest, DISABLED_FolderSwapCommit) {
On 2009/11/13 00:18:14, nick wrote:
> I think we can delete these tests now.  They don't do anything interesting.

Done.

http://codereview.chromium.org/371029/diff/2056/2072#newcode2279
Line 2279: }
On 2009/11/13 00:18:14, nick wrote:
> Much improved.

Done.

http://codereview.chromium.org/371029/diff/2056/2072#newcode2510
Line 2510: mock_server_->AddUpdateDirectory(bob_id, TestIdFactory::root(),
"bob", 1, 10);
On 2009/11/13 00:18:14, nick wrote:
> ids_.root() is more compact: an option.

I prefer TestIdFactory::root() since it's static. I could use ids_.root() but I
prefer knowing explicitly that it's static.

http://codereview.chromium.org/371029/diff/2056/2072#newcode2553
Line 2553: TEST_F(SyncerTest, DISABLED_ServerDeletingFolderWeHaveAnOpenEntryIn)
{
On 2009/11/13 00:18:14, nick wrote:
> Don't comment it out -- that's a death sentence.  Actually, please try fixing
it
> up and running it.  You make have fixed it.

Whoa. I ran it, the case is kind of scary. We should fix it at some point. Added
a todo.

http://codereview.chromium.org/371029/diff/2056/2072#newcode2658
Line 2658: namespace FolderMoveDeleteRenameTest {
On 2009/11/13 00:18:14, nick wrote:
> This isn't an appropriate use of (or name for) a namespace.  Better would be
to
> make this a test fixture that derives from SyncerTest.

Modified to make a test fixture and a closure.

http://codereview.chromium.org/371029/diff/2056/2072#newcode2658
Line 2658: namespace FolderMoveDeleteRenameTest {
On 2009/11/13 00:18:14, nick wrote:
> This isn't an appropriate use of (or name for) a namespace.  Better would be
to
> make this a test fixture that derives from SyncerTest.

Done.

http://codereview.chromium.org/371029/diff/2056/2072#newcode2691
Line 2691: FolderMoveDeleteRenameTest::bob_id_number);
On 2009/11/13 00:18:14, nick wrote:
> Underindented (also below).

Done.

http://codereview.chromium.org/371029/diff/2056/2072#newcode2691
Line 2691: FolderMoveDeleteRenameTest::bob_id_number);
On 2009/11/13 00:18:14, nick wrote:
> Underindented (also below).

Done.

http://codereview.chromium.org/371029/diff/2056/2072#newcode3001
Line 3001: void DeleteSusanInRoot(Directory* dir) {
On 2009/11/13 00:18:14, nick wrote:
> If you decide to transform the earlier case (MoveBobIntoID2) into a test
> fixture, please go ahead and do the same here

Done.

http://codereview.chromium.org/371029/diff/2056/2072#newcode3001
Line 3001: void DeleteSusanInRoot(Directory* dir) {
On 2009/11/13 00:18:14, nick wrote:
> If you decide to transform the earlier case (MoveBobIntoID2) into a test
> fixture, please go ahead and do the same here

Done.

http://codereview.chromium.org/371029/diff/2056/2078
File chrome/browser/sync/syncable/directory_backing_store.cc (right):

http://codereview.chromium.org/371029/diff/2056/2078#newcode40
Line 40: static const int32 kCurrentDBVersion = 68;
On 2009/11/13 00:18:14, nick wrote:
> When we push this, the version number bump from the resulting download will
> cause a traffic spike of getupdates-from-timestamp-zeroes.  Which may be fine.

> It will be smoothed out by the average time until browser restart.
> 
> Let's talk it out.  How hard would the column-dropping migration be? 
> http://www.sqlite.org/faq.html#q11

I think the risk factors are a bit high for the column migration, it tends to be
error prone. At least for this migration it isn't really worth it. The browser
smoothing I think should be fine; cosmo has indicated they don't care about
reads that much either. We can talk more though.

http://codereview.chromium.org/371029/diff/2056/2079
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/371029/diff/2056/2079#newcode348
Line 348: needle_.ref(META_HANDLE) = -1;
On 2009/11/13 00:18:14, nick wrote:
> It's a signed value, so numeric_limits<int64>::min() would be better.

Done.

http://codereview.chromium.org/371029/diff/2056/2079#newcode354
Line 354: needle_.ref(META_HANDLE) = -1;
On 2009/11/13 00:18:14, nick wrote:
> make the needle numeric_limits<int64>::max() and then just return the
> upper_bound (no loop needed).  The edge cases will work out.

Done.

http://codereview.chromium.org/371029/diff/2056/2079#newcode379
Line 379: kernel_->parent_id_child_index;
On 2009/11/13 00:18:14, nick wrote:
> Should fit on one line.

Done.

http://codereview.chromium.org/371029/diff/2056/2079#newcode458
Line 458: entry->ref(PARENT_ID) = new_parent_id;
On 2009/11/13 00:18:14, nick wrote:
> return

Done.

http://codereview.chromium.org/371029/diff/2056/2079#newcode1031
Line 1031: : Entry(trans), write_transaction_(trans) {
On 2009/11/13 00:18:14, nick wrote:
> Indent two more spaces; one initializer per line.

Done.

http://codereview.chromium.org/371029/diff/2056/2080
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/371029/diff/2056/2080#newcode360
Line 360: inline PathString GetName() const {
On 2009/11/13 00:18:14, nick wrote:
> Are these still used?

Done.

http://codereview.chromium.org/371029/diff/2056/2080#newcode370
Line 370: inline bool ExistsOnClientBecauseDatabaseNameIsNonEmpty() const {
On 2009/11/13 00:18:14, nick wrote:
> This check is still valid, I think.  Rename it since the concept of "Database
> name" was an artifact of sanitization.

Done.

http://codereview.chromium.org/371029/diff/2056/2082
File chrome/browser/sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/371029/diff/2056/2082#newcode161
Line 161: dir.SaveChanges();
On 2009/11/13 00:18:14, nick wrote:
> remove("SimpleTest.sqlite3");

Done. Truly this is the worst unit test of all time.

http://codereview.chromium.org/371029/diff/2056/2084
File chrome/test/sync/engine/test_syncable_utils.cc (right):

http://codereview.chromium.org/371029/diff/2056/2084#newcode56
Line 56: CHECK(1 == CountEntriesWithName(rtrans, parent_id, name));
On 2009/11/13 00:18:14, nick wrote:
> This is test code.  Use EXPECT instead of CHECK so that the test fails but
> execution continues on to the next test.

I tried it but sometimes it does not properly trigger. EXPECT running outside of
the main test block works oddly.

[ncarter (slow), 2009-11-17 08:01:41]

You're in the queue behind Tim.

 - nick

On Mon, Nov 16, 2009 at 11:23 PM, Jonathan Huang <chron@google.com> wrote:

> ping on this cl
>
> On Fri, Nov 13, 2009 at 6:21 PM,  <chron@chromium.org> wrote:
> > Thanks for the review. I've changed callbacks to closures and made all
> other
> > fixes.
> >
> >
> > http://codereview.chromium.org/371029/diff/2056/2061
> > File chrome/browser/sync/engine/conflict_resolver.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2061#newcode46
> > Line 46: // number of 16 bit characters in the UTF-16 representation.
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> This TODO is a non-issue with Munjal's changes to make PathString be
> >> std::string.  The result is that .length() should be accurate  --
> >
> > though to be
> >>
> >> absolutely pedantic, we would validate the UTF-8 and normalize to form
> >
> > C.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2061#newcode52
> > Line 52: maximum_name_too_short);
> > Removed this function.
> >
> > http://codereview.chromium.org/371029/diff/2056/2061#newcode330
> > Line 330: LOG(INFO) << "Undoing our deletion of " << entry <<
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> I'm used to seeing << pushed to the next line.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2061#newcode338
> > Line 338: }
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> This check is redundant to what's already done inside
> >
> > MutableEntry::Put.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2064
> > File chrome/browser/sync/engine/process_commit_response_command.cc
> > (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2064#newcode343
> > Line 343: server_entry, &server_name));
> > Removed DCHECK, altered function not to use bool.
> >
> > http://codereview.chromium.org/371029/diff/2056/2064#newcode345
> > Line 345: LOG(INFO) << "Server provided committed name:" << server_name;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> I'd nix this log statement.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2064#newcode347
> > Line 347: old_name != server_name) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Should fit on one line.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2065
> > File chrome/browser/sync/engine/process_updates_command.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2065#newcode138
> > Line 138: DCHECK(SyncerProtoUtil::NameFromSyncEntity(entry, &name));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Again, incorrect in release mode
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2065#newcode158
> > Line 158: // that much if things don't match up exactly.
> > Log error it is. In most of the cases that I've seen this, we should
> > just take the server side. I'll add a todo. If log error won't trigger
> > in release mode then this is fine.
> >
> > http://codereview.chromium.org/371029/diff/2056/2066
> > File chrome/browser/sync/engine/syncapi.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2066#newcode420
> > Line 420: DCHECK(entry_->Put(syncable::NON_UNIQUE_NAME, name));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> It's not necessary anymore -- this method doesn't need to touch the
> >
> > name.  We
> >>
> >> needed to do it before because the sync glue code (which didn't care
> >
> > about name
> >>
> >> uniqueness) could have caused an inadvertent name clash as result of a
> >
> > move, and
> >>
> >> so we had to do the MakeNonColliding junk.  The code as written before
> >
> > was
> >>
> >> careful to preserve the non-unique name.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2066#newcode420
> > Line 420: DCHECK(entry_->Put(syncable::NON_UNIQUE_NAME, name));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> It's not necessary anymore -- this method doesn't need to touch the
> >
> > name.  We
> >>
> >> needed to do it before because the sync glue code (which didn't care
> >
> > about name
> >>
> >> uniqueness) could have caused an inadvertent name clash as result of a
> >
> > move, and
> >>
> >> so we had to do the MakeNonColliding junk.  The code as written before
> >
> > was
> >>
> >> careful to preserve the non-unique name.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2068
> > File chrome/browser/sync/engine/syncer_proto_util.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2068#newcode241
> > Line 241: bool NameFromEntry(T entry, PathString* out_pathstring) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> The empty string is understood to be an invalid value for a name, so
> >
> > I'd use
> >>
> >> that instead of a bool.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2068#newcode258
> > Line 258: return NameFromEntry <const SyncEntity&> (entry,
> > out_pathstring);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> name is required, this will always return true.
> >
> > Surprisingly invalid protobuffers are happily accepted usually. But I
> > removed the bool anyway.
> >
> > http://codereview.chromium.org/371029/diff/2056/2068#newcode264
> > Line 264: return NameFromEntry <const CommitResponse_EntryResponse&>
> > (entry, out_pathstring);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> I think the template hurts readability.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2069
> > File chrome/browser/sync/engine/syncer_proto_util.h (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2069#newcode61
> > Line 61: static bool NameFromSyncEntity(const SyncEntity& entry,
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Boolean values saying "o noes invalid UTF-8" have proven to be pretty
> >
> > useless in
> >>
> >> practice.  I think it's better to DCHECK at the point of the error,
> >
> > rather than
> >>
> >> propagating it, and then proceed after substituting an appropriately
> >
> > truncated
> >>
> >> or default value.
> >
> >> [ Ideally we'd put that logic as part of the generated proto parser,
> >
> > but a
> >>
> >> thread I started suggests that the maintainers aren't in favor
> >
> > (there's some
> >>
> >> validation, but it's debug-only and not supported at all in
> >
> > protobuf-lite). ]
> >
> >> This applies to the next function, too.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2070
> > File chrome/browser/sync/engine/syncer_proto_util_unittest.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2070#newcode79
> > Line 79: const string one_name_string("Eggheadednesses");
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> PathString == string now, so this can be condensed.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2070#newcode88
> > Line 88:
> >
> EXPECT_TRUE(SyncerProtoUtil::NameFromCommitEntryResponse(one_name_response,
> > &name_b));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> 80 chars.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2070#newcode99
> > Line 99: const PathString
> > second_one_name_path_string(PSTR("snakesonaplane"));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Consider being consistent with the pattern:
> >
> >
>
http://spreadsheets.google.com/ccc?key=0AqzQKOKy_xXfdE1teWowQ3lPZl9HVHZNVmFBM...
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072
> > File chrome/browser/sync/engine/syncer_unittest.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode71
> > Line 71: using syncable::NON_UNIQUE_NAME;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Sort.
> >
> > I need to put this on the wall.
> >
>
http://www.poster.net/sesamstrasse/sesamstrasse-sesame-street-alphabet-996898...
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode820
> > Line 820: PathString parent2_name = PSTR("A");
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Can use string and no PSTR(), but it's fine.
> >
> > Will string gut in the next pass.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode825
> > Line 825: MutableEntry parent(&wtrans, syncable::CREATE,
> > wtrans.root_id(), parent1_name);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> 80 chars (several places in this file).  Run lint.
> >
> > Ran and fixed.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode837
> > Line 837: MutableEntry parent2(&wtrans, syncable::CREATE, parent_id_,
> > parent2_name);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> There's no reason for MutableEntry to take a title in the CREATE ctor
> >
> > anymore,
> >>
> >> but I'm not convinced it's worth the effort to change.
> >
> > For the next fixit I think.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode875
> > Line 875: GetOnlyEntryWithName(&rtrans, parent_id_, parent2_name);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> You're not in namespace syncable, and you didn't import the syncable
> >
> > utils into
> >>
> >> this namespace via a using-declaration.  So I'm not sure why this
> >
> > works.  Please
> >>
> >> import all three and use them consistently without the syncable::
> >
> > qualification.
> >
> > It is a mystery. Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode902
> > Line 902: // TODO(chron): This is an arbitrary local client value. Maybe
> > we should use NewLocalId?
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Sounds good to me.  It should just work.  Of course, it's a little
> >
> > redundant,
> >>
> >> because the CREATE ctor assigns a unique local ID already, but none of
> >
> > this
> >>
> >> really impacts the test.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode918
> > Line 918: // This is an arbitrary local client value. Consider
> > NewLocalId.
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Redundant to earlier comment.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode1267
> > Line 1267: int64 metahandle_entry;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Move so that it's as close as possible to its initialization.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode1346
> > Line 1346: VerifyTestDataInEntry(&trans, &entry);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> You lost something valuable here: you no longer verify that a lookup
> >
> > via
> >>
> >> folder_id (the original, pre-reunite ID) fails.
> >
> >
> >
> > Good catch.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode1585
> > Line 1585: PSTR("bob_child"));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Why change this?
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode1600
> > Line 1600: PSTR("bob"));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> It fit on one line before.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode1624
> > Line 1624: bool TouchFredAndGingerInRoot(Directory* dir) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> It's unreferenced.
> >
> > But it's so hard to bring myself to delete it... :P
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode1810
> > Line 1810: }
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Worst test ever.
> >
> > I beg to differ. THIS is the worst test ever:
> >
>
http://cs/p?sa=N&cd=1&ct=rc#chrome/trunk/src/chrome/browser/sync/syncable/syn...
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode1996
> > Line 1996: TEST_F(SyncerTest, DISABLED_FolderSwapCommit) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> I think we can delete these tests now.  They don't do anything
> >
> > interesting.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode2279
> > Line 2279: }
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Much improved.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode2510
> > Line 2510: mock_server_->AddUpdateDirectory(bob_id,
> > TestIdFactory::root(), "bob", 1, 10);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> ids_.root() is more compact: an option.
> >
> > I prefer TestIdFactory::root() since it's static. I could use
> > ids_.root() but I prefer knowing explicitly that it's static.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode2553
> > Line 2553: TEST_F(SyncerTest,
> > DISABLED_ServerDeletingFolderWeHaveAnOpenEntryIn) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Don't comment it out -- that's a death sentence.  Actually, please try
> >
> > fixing it
> >>
> >> up and running it.  You make have fixed it.
> >
> > Whoa. I ran it, the case is kind of scary. We should fix it at some
> > point. Added a todo.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode2658
> > Line 2658: namespace FolderMoveDeleteRenameTest {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> This isn't an appropriate use of (or name for) a namespace.  Better
> >
> > would be to
> >>
> >> make this a test fixture that derives from SyncerTest.
> >
> > Modified to make a test fixture and a closure.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode2658
> > Line 2658: namespace FolderMoveDeleteRenameTest {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> This isn't an appropriate use of (or name for) a namespace.  Better
> >
> > would be to
> >>
> >> make this a test fixture that derives from SyncerTest.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode2691
> > Line 2691: FolderMoveDeleteRenameTest::bob_id_number);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Underindented (also below).
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode2691
> > Line 2691: FolderMoveDeleteRenameTest::bob_id_number);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Underindented (also below).
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode3001
> > Line 3001: void DeleteSusanInRoot(Directory* dir) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> If you decide to transform the earlier case (MoveBobIntoID2) into a
> >
> > test
> >>
> >> fixture, please go ahead and do the same here
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode3001
> > Line 3001: void DeleteSusanInRoot(Directory* dir) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> If you decide to transform the earlier case (MoveBobIntoID2) into a
> >
> > test
> >>
> >> fixture, please go ahead and do the same here
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2078
> > File chrome/browser/sync/syncable/directory_backing_store.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2078#newcode40
> > Line 40: static const int32 kCurrentDBVersion = 68;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> When we push this, the version number bump from the resulting download
> >
> > will
> >>
> >> cause a traffic spike of getupdates-from-timestamp-zeroes.  Which may
> >
> > be fine.
> >>
> >> It will be smoothed out by the average time until browser restart.
> >
> >> Let's talk it out.  How hard would the column-dropping migration be?
> >> http://www.sqlite.org/faq.html#q11
> >
> > I think the risk factors are a bit high for the column migration, it
> > tends to be error prone. At least for this migration it isn't really
> > worth it. The browser smoothing I think should be fine; cosmo has
> > indicated they don't care about reads that much either. We can talk more
> > though.
> >
> > http://codereview.chromium.org/371029/diff/2056/2079
> > File chrome/browser/sync/syncable/syncable.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2079#newcode348
> > Line 348: needle_.ref(META_HANDLE) = -1;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> It's a signed value, so numeric_limits<int64>::min() would be better.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2079#newcode354
> > Line 354: needle_.ref(META_HANDLE) = -1;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> make the needle numeric_limits<int64>::max() and then just return the
> >> upper_bound (no loop needed).  The edge cases will work out.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2079#newcode379
> > Line 379: kernel_->parent_id_child_index;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Should fit on one line.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2079#newcode458
> > Line 458: entry->ref(PARENT_ID) = new_parent_id;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> return
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2079#newcode1031
> > Line 1031: : Entry(trans), write_transaction_(trans) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Indent two more spaces; one initializer per line.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2080
> > File chrome/browser/sync/syncable/syncable.h (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2080#newcode360
> > Line 360: inline PathString GetName() const {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Are these still used?
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2080#newcode370
> > Line 370: inline bool ExistsOnClientBecauseDatabaseNameIsNonEmpty()
> > const {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> This check is still valid, I think.  Rename it since the concept of
> >
> > "Database
> >>
> >> name" was an artifact of sanitization.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2082
> > File chrome/browser/sync/syncable/syncable_unittest.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2082#newcode161
> > Line 161: dir.SaveChanges();
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> remove("SimpleTest.sqlite3");
> >
> > Done. Truly this is the worst unit test of all time.
> >
> > http://codereview.chromium.org/371029/diff/2056/2084
> > File chrome/test/sync/engine/test_syncable_utils.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2084#newcode56
> > Line 56: CHECK(1 == CountEntriesWithName(rtrans, parent_id, name));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> This is test code.  Use EXPECT instead of CHECK so that the test fails
> >
> > but
> >>
> >> execution continues on to the next test.
> >
> > I tried it but sometimes it does not properly trigger. EXPECT running
> > outside of the main test block works oddly.
> >
> > http://codereview.chromium.org/371029
> >
>
>
>
> --
> Syndicated
>

[chron, 2009-11-17 14:23:56]

Haha, thanks! I'll give tim's cl a pass too.

On Nov 17, 2009, at 3:01 AM, Nick Carter <nick@chromium.org> wrote:

> You're in the queue behind Tim.
>
>  - nick
>
> On Mon, Nov 16, 2009 at 11:23 PM, Jonathan Huang <chron@google.com>  
> wrote:
> ping on this cl
>
> On Fri, Nov 13, 2009 at 6:21 PM,  <chron@chromium.org> wrote:
> > Thanks for the review. I've changed callbacks to closures and made  
> all other
> > fixes.
> >
> >
> > http://codereview.chromium.org/371029/diff/2056/2061
> > File chrome/browser/sync/engine/conflict_resolver.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2061#newcode46
> > Line 46: // number of 16 bit characters in the UTF-16  
> representation.
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> This TODO is a non-issue with Munjal's changes to make PathString  
> be
> >> std::string.  The result is that .length() should be accurate  --
> >
> > though to be
> >>
> >> absolutely pedantic, we would validate the UTF-8 and normalize to  
> form
> >
> > C.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2061#newcode52
> > Line 52: maximum_name_too_short);
> > Removed this function.
> >
> > http://codereview.chromium.org/371029/diff/2056/2061#newcode330
> > Line 330: LOG(INFO) << "Undoing our deletion of " << entry <<
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> I'm used to seeing << pushed to the next line.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2061#newcode338
> > Line 338: }
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> This check is redundant to what's already done inside
> >
> > MutableEntry::Put.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2064
> > File chrome/browser/sync/engine/process_commit_response_command.cc
> > (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2064#newcode343
> > Line 343: server_entry, &server_name));
> > Removed DCHECK, altered function not to use bool.
> >
> > http://codereview.chromium.org/371029/diff/2056/2064#newcode345
> > Line 345: LOG(INFO) << "Server provided committed name:" <<  
> server_name;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> I'd nix this log statement.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2064#newcode347
> > Line 347: old_name != server_name) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Should fit on one line.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2065
> > File chrome/browser/sync/engine/process_updates_command.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2065#newcode138
> > Line 138: DCHECK(SyncerProtoUtil::NameFromSyncEntity(entry, &name));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Again, incorrect in release mode
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2065#newcode158
> > Line 158: // that much if things don't match up exactly.
> > Log error it is. In most of the cases that I've seen this, we should
> > just take the server side. I'll add a todo. If log error won't  
> trigger
> > in release mode then this is fine.
> >
> > http://codereview.chromium.org/371029/diff/2056/2066
> > File chrome/browser/sync/engine/syncapi.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2066#newcode420
> > Line 420: DCHECK(entry_->Put(syncable::NON_UNIQUE_NAME, name));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> It's not necessary anymore -- this method doesn't need to touch the
> >
> > name.  We
> >>
> >> needed to do it before because the sync glue code (which didn't  
> care
> >
> > about name
> >>
> >> uniqueness) could have caused an inadvertent name clash as result  
> of a
> >
> > move, and
> >>
> >> so we had to do the MakeNonColliding junk.  The code as written  
> before
> >
> > was
> >>
> >> careful to preserve the non-unique name.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2066#newcode420
> > Line 420: DCHECK(entry_->Put(syncable::NON_UNIQUE_NAME, name));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> It's not necessary anymore -- this method doesn't need to touch the
> >
> > name.  We
> >>
> >> needed to do it before because the sync glue code (which didn't  
> care
> >
> > about name
> >>
> >> uniqueness) could have caused an inadvertent name clash as result  
> of a
> >
> > move, and
> >>
> >> so we had to do the MakeNonColliding junk.  The code as written  
> before
> >
> > was
> >>
> >> careful to preserve the non-unique name.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2068
> > File chrome/browser/sync/engine/syncer_proto_util.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2068#newcode241
> > Line 241: bool NameFromEntry(T entry, PathString* out_pathstring) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> The empty string is understood to be an invalid value for a name,  
> so
> >
> > I'd use
> >>
> >> that instead of a bool.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2068#newcode258
> > Line 258: return NameFromEntry <const SyncEntity&> (entry,
> > out_pathstring);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> name is required, this will always return true.
> >
> > Surprisingly invalid protobuffers are happily accepted usually.  
> But I
> > removed the bool anyway.
> >
> > http://codereview.chromium.org/371029/diff/2056/2068#newcode264
> > Line 264: return NameFromEntry <const CommitResponse_EntryResponse&>
> > (entry, out_pathstring);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> I think the template hurts readability.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2069
> > File chrome/browser/sync/engine/syncer_proto_util.h (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2069#newcode61
> > Line 61: static bool NameFromSyncEntity(const SyncEntity& entry,
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Boolean values saying "o noes invalid UTF-8" have proven to be  
> pretty
> >
> > useless in
> >>
> >> practice.  I think it's better to DCHECK at the point of the error,
> >
> > rather than
> >>
> >> propagating it, and then proceed after substituting an  
> appropriately
> >
> > truncated
> >>
> >> or default value.
> >
> >> [ Ideally we'd put that logic as part of the generated proto  
> parser,
> >
> > but a
> >>
> >> thread I started suggests that the maintainers aren't in favor
> >
> > (there's some
> >>
> >> validation, but it's debug-only and not supported at all in
> >
> > protobuf-lite). ]
> >
> >> This applies to the next function, too.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2070
> > File chrome/browser/sync/engine/syncer_proto_util_unittest.cc  
> (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2070#newcode79
> > Line 79: const string one_name_string("Eggheadednesses");
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> PathString == string now, so this can be condensed.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2070#newcode88
> > Line 88:
> > EXPECT_TRUE(SyncerProtoUtil::NameFromCommitEntryResponse 
> (one_name_response,
> > &name_b));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> 80 chars.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2070#newcode99
> > Line 99: const PathString
> > second_one_name_path_string(PSTR("snakesonaplane"));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Consider being consistent with the pattern:
> >
> >
http://spreadsheets.google.com/ccc?key=0AqzQKOKy_xXfdE1teWowQ3lPZl9HVHZNVmFBM...
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072
> > File chrome/browser/sync/engine/syncer_unittest.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode71
> > Line 71: using syncable::NON_UNIQUE_NAME;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Sort.
> >
> > I need to put this on the wall.
> >
http://www.poster.net/sesamstrasse/sesamstrasse-sesame-street-alphabet-996898...
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode820
> > Line 820: PathString parent2_name = PSTR("A");
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Can use string and no PSTR(), but it's fine.
> >
> > Will string gut in the next pass.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode825
> > Line 825: MutableEntry parent(&wtrans, syncable::CREATE,
> > wtrans.root_id(), parent1_name);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> 80 chars (several places in this file).  Run lint.
> >
> > Ran and fixed.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode837
> > Line 837: MutableEntry parent2(&wtrans, syncable::CREATE,  
> parent_id_,
> > parent2_name);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> There's no reason for MutableEntry to take a title in the CREATE  
> ctor
> >
> > anymore,
> >>
> >> but I'm not convinced it's worth the effort to change.
> >
> > For the next fixit I think.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode875
> > Line 875: GetOnlyEntryWithName(&rtrans, parent_id_, parent2_name);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> You're not in namespace syncable, and you didn't import the  
> syncable
> >
> > utils into
> >>
> >> this namespace via a using-declaration.  So I'm not sure why this
> >
> > works.  Please
> >>
> >> import all three and use them consistently without the syncable::
> >
> > qualification.
> >
> > It is a mystery. Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode902
> > Line 902: // TODO(chron): This is an arbitrary local client value.  
> Maybe
> > we should use NewLocalId?
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Sounds good to me.  It should just work.  Of course, it's a little
> >
> > redundant,
> >>
> >> because the CREATE ctor assigns a unique local ID already, but  
> none of
> >
> > this
> >>
> >> really impacts the test.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode918
> > Line 918: // This is an arbitrary local client value. Consider
> > NewLocalId.
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Redundant to earlier comment.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode1267
> > Line 1267: int64 metahandle_entry;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Move so that it's as close as possible to its initialization.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode1346
> > Line 1346: VerifyTestDataInEntry(&trans, &entry);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> You lost something valuable here: you no longer verify that a  
> lookup
> >
> > via
> >>
> >> folder_id (the original, pre-reunite ID) fails.
> >
> >
> >
> > Good catch.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode1585
> > Line 1585: PSTR("bob_child"));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Why change this?
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode1600
> > Line 1600: PSTR("bob"));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> It fit on one line before.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode1624
> > Line 1624: bool TouchFredAndGingerInRoot(Directory* dir) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> It's unreferenced.
> >
> > But it's so hard to bring myself to delete it... :P
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode1810
> > Line 1810: }
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Worst test ever.
> >
> > I beg to differ. THIS is the worst test ever:
> >
http://cs/p?sa=N&cd=1&ct=rc#chrome/trunk/src/chrome/browser/sync/syncable/syn...
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode1996
> > Line 1996: TEST_F(SyncerTest, DISABLED_FolderSwapCommit) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> I think we can delete these tests now.  They don't do anything
> >
> > interesting.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode2279
> > Line 2279: }
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Much improved.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode2510
> > Line 2510: mock_server_->AddUpdateDirectory(bob_id,
> > TestIdFactory::root(), "bob", 1, 10);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> ids_.root() is more compact: an option.
> >
> > I prefer TestIdFactory::root() since it's static. I could use
> > ids_.root() but I prefer knowing explicitly that it's static.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode2553
> > Line 2553: TEST_F(SyncerTest,
> > DISABLED_ServerDeletingFolderWeHaveAnOpenEntryIn) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Don't comment it out -- that's a death sentence.  Actually,  
> please try
> >
> > fixing it
> >>
> >> up and running it.  You make have fixed it.
> >
> > Whoa. I ran it, the case is kind of scary. We should fix it at some
> > point. Added a todo.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode2658
> > Line 2658: namespace FolderMoveDeleteRenameTest {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> This isn't an appropriate use of (or name for) a namespace.  Better
> >
> > would be to
> >>
> >> make this a test fixture that derives from SyncerTest.
> >
> > Modified to make a test fixture and a closure.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode2658
> > Line 2658: namespace FolderMoveDeleteRenameTest {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> This isn't an appropriate use of (or name for) a namespace.  Better
> >
> > would be to
> >>
> >> make this a test fixture that derives from SyncerTest.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode2691
> > Line 2691: FolderMoveDeleteRenameTest::bob_id_number);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Underindented (also below).
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode2691
> > Line 2691: FolderMoveDeleteRenameTest::bob_id_number);
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Underindented (also below).
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode3001
> > Line 3001: void DeleteSusanInRoot(Directory* dir) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> If you decide to transform the earlier case (MoveBobIntoID2) into a
> >
> > test
> >>
> >> fixture, please go ahead and do the same here
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2072#newcode3001
> > Line 3001: void DeleteSusanInRoot(Directory* dir) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> If you decide to transform the earlier case (MoveBobIntoID2) into a
> >
> > test
> >>
> >> fixture, please go ahead and do the same here
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2078
> > File chrome/browser/sync/syncable/directory_backing_store.cc  
> (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2078#newcode40
> > Line 40: static const int32 kCurrentDBVersion = 68;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> When we push this, the version number bump from the resulting  
> download
> >
> > will
> >>
> >> cause a traffic spike of getupdates-from-timestamp-zeroes.  Which  
> may
> >
> > be fine.
> >>
> >> It will be smoothed out by the average time until browser restart.
> >
> >> Let's talk it out.  How hard would the column-dropping migration  
> be?
> >> http://www.sqlite.org/faq.html#q11
> >
> > I think the risk factors are a bit high for the column migration, it
> > tends to be error prone. At least for this migration it isn't really
> > worth it. The browser smoothing I think should be fine; cosmo has
> > indicated they don't care about reads that much either. We can  
> talk more
> > though.
> >
> > http://codereview.chromium.org/371029/diff/2056/2079
> > File chrome/browser/sync/syncable/syncable.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2079#newcode348
> > Line 348: needle_.ref(META_HANDLE) = -1;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> It's a signed value, so numeric_limits<int64>::min() would be  
> better.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2079#newcode354
> > Line 354: needle_.ref(META_HANDLE) = -1;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> make the needle numeric_limits<int64>::max() and then just return  
> the
> >> upper_bound (no loop needed).  The edge cases will work out.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2079#newcode379
> > Line 379: kernel_->parent_id_child_index;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Should fit on one line.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2079#newcode458
> > Line 458: entry->ref(PARENT_ID) = new_parent_id;
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> return
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2079#newcode1031
> > Line 1031: : Entry(trans), write_transaction_(trans) {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Indent two more spaces; one initializer per line.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2080
> > File chrome/browser/sync/syncable/syncable.h (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2080#newcode360
> > Line 360: inline PathString GetName() const {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> Are these still used?
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2080#newcode370
> > Line 370: inline bool ExistsOnClientBecauseDatabaseNameIsNonEmpty()
> > const {
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> This check is still valid, I think.  Rename it since the concept of
> >
> > "Database
> >>
> >> name" was an artifact of sanitization.
> >
> > Done.
> >
> > http://codereview.chromium.org/371029/diff/2056/2082
> > File chrome/browser/sync/syncable/syncable_unittest.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2082#newcode161
> > Line 161: dir.SaveChanges();
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> remove("SimpleTest.sqlite3");
> >
> > Done. Truly this is the worst unit test of all time.
> >
> > http://codereview.chromium.org/371029/diff/2056/2084
> > File chrome/test/sync/engine/test_syncable_utils.cc (right):
> >
> > http://codereview.chromium.org/371029/diff/2056/2084#newcode56
> > Line 56: CHECK(1 == CountEntriesWithName(rtrans, parent_id, name));
> > On 2009/11/13 00:18:14, nick wrote:
> >>
> >> This is test code.  Use EXPECT instead of CHECK so that the test  
> fails
> >
> > but
> >>
> >> execution continues on to the next test.
> >
> > I tried it but sometimes it does not properly trigger. EXPECT  
> running
> > outside of the main test block works oddly.
> >
> > http://codereview.chromium.org/371029
> >
>
>
>
> --
> Syndicated
>

[tim (not reviewing), 2009-11-17 17:33:19]

http://codereview.chromium.org/371029/diff/1017/1025
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/371029/diff/1017/1025#newcode124
Line 124: // This is important to activate conflict resolution.
Mention how is it important? 
"This is important" can easily and confusingly bitrot.

http://codereview.chromium.org/371029/diff/1017/1026
File chrome/browser/sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/371029/diff/1017/1026#newcode158
Line 158: // that much if things don't match up exactly.
Mention why this is allowed to occur and what is interesting about logging it.
And/or add a << "blah blah" to the LOG_IF.

http://codereview.chromium.org/371029/diff/1017/1031
File chrome/browser/sync/engine/syncer_proto_util.h (right):

http://codereview.chromium.org/371029/diff/1017/1031#newcode60
Line 60: static std::string NameFromSyncEntity(const SyncEntity& entry);
these could return const std::string&

http://codereview.chromium.org/371029/diff/1017/1034
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/371029/diff/1017/1034#newcode829
Line 829: MutableEntry parent(&wtrans, syncable::CREATE, wtrans.root_id(),
parent1_name);
nit- 80 lines

[ncarter (slow), 2009-11-18 00:52:29]

LGTM

http://codereview.chromium.org/371029/diff/2056/2078
File chrome/browser/sync/syncable/directory_backing_store.cc (right):

http://codereview.chromium.org/371029/diff/2056/2078#newcode40
Line 40: static const int32 kCurrentDBVersion = 68;
On 2009/11/13 23:21:26, chron_chromium.org wrote:
> On 2009/11/13 00:18:14, nick wrote:
> > When we push this, the version number bump from the resulting download will
> > cause a traffic spike of getupdates-from-timestamp-zeroes.  Which may be
fine.
> 
> > It will be smoothed out by the average time until browser restart.
> > 
> > Let's talk it out.  How hard would the column-dropping migration be? 
> > http://www.sqlite.org/faq.html#q11
> 
> I think the risk factors are a bit high for the column migration, it tends to
be
> error prone. At least for this migration it isn't really worth it. The browser
> smoothing I think should be fine; cosmo has indicated they don't care about
> reads that much either. We can talk more though.

Sounds fine.

http://codereview.chromium.org/371029/diff/1017/1025
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/371029/diff/1017/1025#newcode344
Line 344: if (!server_name.empty() &&  old_name != server_name) {
Extra space here.

http://codereview.chromium.org/371029/diff/1017/1026
File chrome/browser/sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/371029/diff/1017/1026#newcode160
Line 160: &update_entry)) << update_entry;
Indentation is off inside this block.