sync: Remove the remaining conflict sets code

sync: Remove the remaining conflict sets code

Conflict sets were intended to help us deal with updates that could
corrupt the directory tree if applied.  This could happen, for example,
when the server sends us a new item in a directory that has been deleted
locally.

Due to a bug, the code to deal with conflict sets was never run.  It
seems that the consequences of this weren't as bad as one would expect.
By this point, it's not worth fixing, since we are a few weeks away from
adding code to handle conflict sets entirely on the server side.

Over a series of commits, we have removed lots of dead code related to
conflict set handling.  The only remaining function of conflict sets
code was to prevent items that were in both a "conflict set" and a
"simple conflict" state from being processed as simple conflicts.  If
those items had been processed as simple conflicts, there is a small
chance we could accidentally apply them, or do other bad things.

The code that detects conflict sets is overkill for our current needs,
because it was built with the idea that we had to gather information
about these conflict sets in order to resolve them.  Now that this
requirement has been removed, I have been able to greatly simplify the
implementation of conflict set detection by moving it to the update
application code.  Since this was the last remaining purpose of the
conflict set processing code, it is now safe to entirely remove the
concept of conflict sets.

Other changes:
- Removed ConflictProgress::EraseNonBlockingConflictingItemById(); the
  function was never used.
- Added some unit tests for the new functionality.

BUG=107816
TEST=sync_unit_tests


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

[rlarocque, 2012-01-31 19:12:16]

Fred: Please look over the new test cases.  I'm not sure I'm using the right
APIs, so there are probably some issues there.

Nicolas: Please look over everything else.  In particular, I'd like a second
opinion to confirm this won't break conflict resolution.

==========

This commit removes the last of the conflict sets code.  Although this change is
not entirely free of side-effects, it shouldn't affect how conflicts are
resolved in the vast majority of cases.  

For an explanation of how this is possible, please see crbug.com/107816 comment
#5.  Better yet, come see me and we can talk it over.  The reasoning behind this
change is written in my notebook and hasn't been entirely translated to a
digital medium.

https://chromiumcodereview.appspot.com/9305001/diff/2001/chrome/browser/sync/...
File chrome/browser/sync/engine/conflict_resolver.cc (right):

https://chromiumcodereview.appspot.com/9305001/diff/2001/chrome/browser/sync/...
chrome/browser/sync/engine/conflict_resolver.cc:379: // We have a simple
conflict. In order check if positions have changed,
There are lots of indentation changes because I removed an if statement, but
otherwise the code on the left and right side of this diff should be identical.

https://chromiumcodereview.appspot.com/9305001/diff/2001/chrome/browser/sync/...
File chrome/browser/sync/engine/syncer_types.h (right):

https://chromiumcodereview.appspot.com/9305001/diff/2001/chrome/browser/sync/...
chrome/browser/sync/engine/syncer_types.h:29: enum UpdateAttemptResponse {
I'm not sure I like these names.  Suggestions for better names are welcome.

https://chromiumcodereview.appspot.com/9305001/diff/2001/chrome/browser/sync/...
File chrome/browser/sync/engine/syncer_util.cc (right):

https://chromiumcodereview.appspot.com/9305001/diff/2001/chrome/browser/sync/...
chrome/browser/sync/engine/syncer_util.cc:346: return HIERARCHY_CONFLICT;
There is a subtle side-effect to this change, which affects mostly unit tests.  

The old logic would have treated !parent.good() as though it were a simple
conflict.  This commonly happens in unit tests, so I had to update a bunch of
assertions accordingly.  

In the real world, this could happen if we received updates out of order.  Under
the old system, we would have classified this as a simple conflict, but it would
not have been put into a conflict set.  We would have tried to resolve this as
though it were a simple conflict, wasting CPU cycles and possibly throwing away
a valid server changes if we decided for some reason that this was a "client
wins" case.  

Under the new system, we will treat it as a HIERARCHY conflict, which means will
ignore the update and hope the server sends us a better update soon.

https://chromiumcodereview.appspot.com/9305001/diff/2001/chrome/browser/sync/...
chrome/browser/sync/engine/syncer_util.cc:366: if (entry->Get(IS_UNSYNCED)) {
This was moved down here because it's important that HIERARCHY_CONFLICT take
precedence over CONFLICT.

https://chromiumcodereview.appspot.com/9305001/diff/2001/chrome/browser/sync/...
File chrome/browser/sync/sessions/session_state.h (right):

https://chromiumcodereview.appspot.com/9305001/diff/2001/chrome/browser/sync/...
chrome/browser/sync/sessions/session_state.h:201: std::set<syncable::Id>
hierarchy_conflicting_item_ids_;
I'm not sure this is the right thing to do.  I decided to maintain symmetry with
the nonblocking_conflicting_item_ids, even though it doesn't make a lot of sense
to me.  

We never access the members of this set.  I think the set is intended to allow
us to count more accurately (by avoiding double-counting), but I'm not sure
that's actually necessary.  

Alternative approaches:
A. Track both encryption and hierarchy conflicts as nonblocking conflicts
because we treat them the same way.  (this may end up limiting the amount of
information we could export to about:sync)
B. Use counters instead of sets to track these items.  (This makes the most
sense to me, but I'm reluctant to delete code I don't understand.)
C. Some combination of the two: one set, two counters?

[akalin, 2012-02-01 01:10:24]

some initial test comments.  i'll take a more detailed pass after you add the
test comments. (my eyes tend to glaze over when reading lots of test code, which
is why I'm asking for test comments :) )

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:146: void
CreateUnappliedAndUnsyncedItem(const Id& item_id,
comment that metahandle_out can be NULL

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:152:
ASSERT_TRUE(item_id.ServerKnows()) << "CreateUnappliedAndUnsyncedItem()"
just to make sure you know, ASSERT_TRUE just does, roughly:

if (!condition) {
  ADD_FAILURE() << "...";
  return;
}

i.e., this check failing only halts the current function, not the entire test. 
So if this isn't what you want, you have to make this function return a bool or
something.

Although it may be okay in this case, since the test won't crash if you proceed.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:157: int64
local_metahandle_out;
initialize this int64 (just paranoia)

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:249:
TEST_F(ApplyUpdatesCommandTest, SimpleConflict) {
Please describe what the test does in a comment above TEST_F

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:265:
TEST_F(ApplyUpdatesCommandTest, HierarchyAndSimpleConflict) {
here too

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:266: int64 handle;
initialize (paranoia, although if CreateUnapplied... fails, then you can get
non-deterministic behavior since you use handle)

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:299:
TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDirectoryLoop) {
describe test

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:353:
TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDeletedParent) {
here too

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:386:
TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDeleteNonEmptyDirectory) {
here too

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:437:
TEST_F(ApplyUpdatesCommandTest, HierarchyConflictUnknownParent) {
here too (since you're touching it)

[Nicolas Zea, 2012-02-01 19:16:19]

Some comments.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/c...
File chrome/browser/sync/engine/conflict_resolver.cc (right):

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/c...
chrome/browser/sync/engine/conflict_resolver.cc:364: bool
ConflictResolver::ResolveConflicts(syncable::WriteTransaction* trans,
I think it would be good to have a comment above here defining what a set is (we
still refer to sets in a couple comments) and why we we currently ignore the
possibility of sets.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/c...
chrome/browser/sync/engine/conflict_resolver.cc:369: // First iterate over
simple conflict items.
Remove first

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_types.h (right):

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_types.h:35: // hierarchy conflicts and also
can not be dectrypted right now.  The list has
detcrypted

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_types.h:37: // priority first.  The example
given above would be treated as
Example given above?

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_types.h:39: // as a CONFLICT or
HIERARCHY_CONFLICT.
Mention that CONFLICT_ENCRYPTION and CONFLICT_HIERARCHY are nonblocking
conflicts, in that we do not attempt to resolve them or have logic based on
their existence, and rely on a later update allowing us to apply them
successfully.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_types.h:50: HIERARCHY_CONFLICT,
Prefer CONFLICT_HEIRARCHY

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_types.h:52: // This indicates that item
contains both an unapplied update and an unsynced
This indicates that the item was modified both remotely (is_unapplied_update)
and locally (is_unsynced). We use the ConflictResolver to decide which of the
changes should take priority or if we can possibly merge the data.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_util.cc:346: return HIERARCHY_CONFLICT;
On 2012/01/31 19:12:17, rlarocque wrote:
> There is a subtle side-effect to this change, which affects mostly unit tests.
 
> 
> The old logic would have treated !parent.good() as though it were a simple
> conflict.  This commonly happens in unit tests, so I had to update a bunch of
> assertions accordingly.  
> 
> In the real world, this could happen if we received updates out of order. 
Under
> the old system, we would have classified this as a simple conflict, but it
would
> not have been put into a conflict set.  We would have tried to resolve this as
> though it were a simple conflict, wasting CPU cycles and possibly throwing
away
> a valid server changes if we decided for some reason that this was a "client
> wins" case.  
> 
> Under the new system, we will treat it as a HIERARCHY conflict, which means
will
> ignore the update and hope the server sends us a better update soon.

So I'm clear, the only side effect here is that we no longer invoke the conflict
resolver on this item, right? We'll still attempt to apply this update next time
around (and if the parent has arrived by then the update will apply fine).

Effectively, hierarchy conflict is treated the same as encryption conflict.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/session_state.h:164: // Tracks progress of
conflicts and their resolution.
resolutions

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/session_state.h:191: // Conflicts that occur when
an local and server changes collide, minus
remove an

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/session_state.h:201: std::set<syncable::Id>
hierarchy_conflicting_item_ids_;
On 2012/01/31 19:12:17, rlarocque wrote:
> I'm not sure this is the right thing to do.  I decided to maintain symmetry
with
> the nonblocking_conflicting_item_ids, even though it doesn't make a lot of
sense
> to me.  
> 
> We never access the members of this set.  I think the set is intended to allow
> us to count more accurately (by avoiding double-counting), but I'm not sure
> that's actually necessary.  
> 
> Alternative approaches:
> A. Track both encryption and hierarchy conflicts as nonblocking conflicts
> because we treat them the same way.  (this may end up limiting the amount of
> information we could export to about:sync)
> B. Use counters instead of sets to track these items.  (This makes the most
> sense to me, but I'm reluctant to delete code I don't understand.)
> C. Some combination of the two: one set, two counters?

I prefer C, probably by adding an extra parameter to the nonblocking conflict
methods specifying the type of nonblocking conflict (e.g. CONFLICT_ENCRYPTION 
or CONFLICT_HIERARCHY).

[rlarocque, 2012-02-02 00:32:56]

This second diff ended up being bigger than I expected.  

One reason for this is that I made a few name changes:
- s/HIERARCHY_CONFLICT/CONFLICT_HIERARCHY
- s/CONFLICT/CONFLICT_SIMPLE (regex is approximate)
- Updated function names to match (ie. s/AddConflict/AddSimpleConflict)
- Renamed "non-blocking conflicts" to "unresolvable conflicts".  Keep an eye out
for the use of the words "non-blocking" in the comments.

Another change is that I've added another type of "unresolvable conflict":
server conflicts.  When the server returns CONFLICT in response to a commit
attempt, we now add that item to the list of non-blocking conflicts.  This was
done so that we could make the assertion that all items in
ProcessSimpleConflicts() are both IS_UNSYNCED and IS_UNAPPLIED_UPDATE.

The rest of the changes are meant to address review comments.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:152:
ASSERT_TRUE(item_id.ServerKnows()) << "CreateUnappliedAndUnsyncedItem()"
On 2012/02/01 01:10:24, akalin wrote:
> just to make sure you know, ASSERT_TRUE just does, roughly:
> 
> if (!condition) {
>   ADD_FAILURE() << "...";
>   return;
> }
> 
> i.e., this check failing only halts the current function, not the entire test.

> So if this isn't what you want, you have to make this function return a bool
or
> something.
> 
> Although it may be okay in this case, since the test won't crash if you
proceed.

The idea of this assert was to catch logic errors.  I will replace it with a
CHECK().  I realize that's not ideal, because it could prevent us from running
other unit tests.

In practice, though, the only time this condition will fail would be if someone
is modifying the tests.  Changes to logic in the main program should not affect
it.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:157: int64
local_metahandle_out;
On 2012/02/01 01:10:24, akalin wrote:
> initialize this int64 (just paranoia)

Done.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:266: int64 handle;
On 2012/02/01 01:10:24, akalin wrote:
> initialize (paranoia, although if CreateUnapplied... fails, then you can get
> non-deterministic behavior since you use handle)

Done.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/c...
File chrome/browser/sync/engine/conflict_resolver.cc (right):

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/c...
chrome/browser/sync/engine/conflict_resolver.cc:364: bool
ConflictResolver::ResolveConflicts(syncable::WriteTransaction* trans,
On 2012/02/01 19:16:19, nzea wrote:
> I think it would be good to have a comment above here defining what a set is
(we
> still refer to sets in a couple comments) and why we we currently ignore the
> possibility of sets.

I'm trying to remove the concept of conflict sets from our code base.  The
concept of hiearchy  conflicts is easier to understand, so we should use it
instead.  I intend to delete all comments referring to them, so let me know if
you find any.  

I could add a comment explaining why we don't try to process the hierarchy and
non-blocking/encryption conflicts here.  Alternatively, I could rename the
function to "ResolveSimpleConflicts()".  What do you think?

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/c...
chrome/browser/sync/engine/conflict_resolver.cc:369: // First iterate over
simple conflict items.
On 2012/02/01 19:16:19, nzea wrote:
> Remove first

Done.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_types.h (right):

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_types.h:35: // hierarchy conflicts and also
can not be dectrypted right now.  The list has
On 2012/02/01 19:16:19, nzea wrote:
> detcrypted

Done.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_types.h:37: // priority first.  The example
given above would be treated as
On 2012/02/01 19:16:19, nzea wrote:
> Example given above?

I was trying to refer to the example two sentences ago, but I think I've
refactored the wording to the point that that's no longer clear.  Will fix.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_types.h:39: // as a CONFLICT or
HIERARCHY_CONFLICT.
On 2012/02/01 19:16:19, nzea wrote:
> Mention that CONFLICT_ENCRYPTION and CONFLICT_HIERARCHY are nonblocking
> conflicts, in that we do not attempt to resolve them or have logic based on
> their existence, and rely on a later update allowing us to apply them
> successfully.

Done.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_types.h:50: HIERARCHY_CONFLICT,
On 2012/02/01 19:16:19, nzea wrote:
> Prefer CONFLICT_HEIRARCHY

Done.  I assume you meant CONFLICT_HIERARCHY.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_types.h:55: CONFLICT
I renamed this to CONFLICT_SIMPLE, just to be consistent.  I also renamed a
bunch of related methods, too.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_util.cc:346: return HIERARCHY_CONFLICT;
On 2012/02/01 19:16:19, nzea wrote:
> On 2012/01/31 19:12:17, rlarocque wrote:
> > There is a subtle side-effect to this change, which affects mostly unit
tests.
>  
> > 
> > The old logic would have treated !parent.good() as though it were a simple
> > conflict.  This commonly happens in unit tests, so I had to update a bunch
of
> > assertions accordingly.  
> > 
> > In the real world, this could happen if we received updates out of order. 
> Under
> > the old system, we would have classified this as a simple conflict, but it
> would
> > not have been put into a conflict set.  We would have tried to resolve this
as
> > though it were a simple conflict, wasting CPU cycles and possibly throwing
> away
> > a valid server changes if we decided for some reason that this was a "client
> > wins" case.  
> > 
> > Under the new system, we will treat it as a HIERARCHY conflict, which means
> will
> > ignore the update and hope the server sends us a better update soon.
> 
> So I'm clear, the only side effect here is that we no longer invoke the
conflict
> resolver on this item, right? We'll still attempt to apply this update next
time
> around (and if the parent has arrived by then the update will apply fine).
> 
> Effectively, hierarchy conflict is treated the same as encryption conflict.

Yes, that is correct.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/session_state.h:164: // Tracks progress of
conflicts and their resolution.
On 2012/02/01 19:16:19, nzea wrote:
> resolutions

Done.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/session_state.h:191: // Conflicts that occur when
an local and server changes collide, minus
On 2012/02/01 19:16:19, nzea wrote:
> remove an

Done.

http://codereview.chromium.org/9305001/diff/2001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/session_state.h:201: std::set<syncable::Id>
hierarchy_conflicting_item_ids_;
On 2012/02/01 19:16:19, nzea wrote:
> On 2012/01/31 19:12:17, rlarocque wrote:
> > I'm not sure this is the right thing to do.  I decided to maintain symmetry
> with
> > the nonblocking_conflicting_item_ids, even though it doesn't make a lot of
> sense
> > to me.  
> > 
> > We never access the members of this set.  I think the set is intended to
allow
> > us to count more accurately (by avoiding double-counting), but I'm not sure
> > that's actually necessary.  
> > 
> > Alternative approaches:
> > A. Track both encryption and hierarchy conflicts as nonblocking conflicts
> > because we treat them the same way.  (this may end up limiting the amount of
> > information we could export to about:sync)
> > B. Use counters instead of sets to track these items.  (This makes the most
> > sense to me, but I'm reluctant to delete code I don't understand.)
> > C. Some combination of the two: one set, two counters?
> 
> I prefer C, probably by adding an extra parameter to the nonblocking conflict
> methods specifying the type of nonblocking conflict (e.g. CONFLICT_ENCRYPTION 
> or CONFLICT_HIERARCHY).

Minor point: If we're going with option C, I'd prefer to use separate functions.
 It's harder to misuse two functions than one function with an enum parameter.  

Implemented in the following patch set.

[akalin, 2012-02-02 01:34:38]

More test comments

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
File chrome/browser/sync/engine/apply_updates_command_unittest.cc (left):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:169: EXPECT_EQ(2,
status->update_progress()->AppliedUpdatesSize())
comment this test, too

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:150: void
CreateUnappliedAndUnsyncedItem(const Id& item_id,
you only call this function twice, with very similar args.  I wonder if you can
simplify this a bit.  What about:

int64 CreateUnappliedAndUnsyncedItem(const string& name, syncable::ModelType) {
...
}

returns the metahandle, or an invalid metahandle on failure.  Uses
MakeServer(name) for the item_id.  Uses id_factory_.root() for the parent. 
is_folder = false.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:157:
CHECK(item_id.ServerKnows());
you CHECK here but you ASSERT_TRUE later in the function.  I think it's just
better all around to have this return false and move the ASSERT_TRUE to the
callers

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:175:
entry.Put(syncable::SERVER_VERSION, next_revision_++);
use GetNextRevision() here, too?

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:258: string
root_server_id = syncable::GetNullId().GetServerId();
this variable doesn't seem to be used

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:266:
sessions::StatusController* status = session()->mutable_status_controller();
can you use the non-mutable status_controller accessor instead?  is there a
non-mutable conflict_progress() accessor?

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:274: //
modifications *and* the remote modification can not be applied without
can not -> cannot

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:300:
sessions::StatusController* status = session()->mutable_status_controller();
here too

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:300:
sessions::StatusController* status = session()->mutable_status_controller();
all these lines are just to check HierarchyConflictingItemsSize() and they're
repeated across multiple tests.  Decomp into helper fn?

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:337:
CHECK(entry.PutPredecessor(id_factory_.root()));
ASSERT_TRUE

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:424:
entry.Put(syncable::BASE_VERSION, GetNextRevision());
i feel like you can decomp this into some helper function that the other block
with lots of entry.Put()s can use.

[Nicolas Zea, 2012-02-02 21:53:41]

Couple more comments/renames

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/c...
File chrome/browser/sync/engine/conflict_resolver.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/c...
chrome/browser/sync/engine/conflict_resolver.cc:82: // both UNSYNCED and
UNAPPLIED_UDPATEs.
are both -> have both IS_UNSYNCED and IS_UNAPPLIED_UPDATE set.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_util.cc:314: // treated as a non-blocking
conflict. See the description in syncer_types.h.
non-blocking -> unresolvable

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/u...
File chrome/browser/sync/engine/update_applicator.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/u...
chrome/browser/sync/engine/update_applicator.cc:168: // the list of nonblocking
conflicts instead of normal conflicts.
blocking -> unresolvable

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/u...
chrome/browser/sync/engine/update_applicator.cc:175: // We add them to the list
of non-blocking conflicts, along with the
list of unresolvable conflicts

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/session_state.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/session_state.cc:122: int
num_blocking_conflicting_updates,
num_resolvable (here and below)

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/session_state.h:132: int
num_blocking_conflicting_updates,
num_resolvable (here and below)

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/status_controller.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/status_controller.cc:266: int
StatusController::TotalNumBlockingConflictingItems() const {
Rename to TotalNumResolvableConflictingItems?

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/status_controller_unittest.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/status_controller_unittest.cc:155:
status.mutable_update_progress()->AddAppliedUpdate(CONFLICT_ENCRYPTION,
Any reason you changed this? Should be valid as it was before right?

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/sync_session.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session.cc:163:
status_controller_->TotalNumBlockingConflictingItems(),
numblocking -> numresolvable

[rlarocque, 2012-02-03 22:31:15]

Lots more changes in this next patch. Hopefully this is the last big update.

Highlights:
- I've taken Fred's advice in most cases regarding the unit tests.  I'm pretty
happy with the changes, the code does look a lot cleaner now.
- I did a better job of tracking down uses of the terms "blocking conflict" and
"non-blocking conflict".  I've fixed the cases Nicolas pointed out and many
more.  However, I did prefer to use the term "simple conflict" rather than
"resolvable conflict" for the conflicts formerly known as "blocking conflict".
- Added TODOs to fix the DebugInfoEventListener.  It's using the old names in
its protobufs and it's not reporting the right values.  We will require a
separate bug, CL and a server change to fix this.  I have maintained the current
(wrong) behaviour for now.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:150: void
CreateUnappliedAndUnsyncedItem(const Id& item_id,
On 2012/02/02 01:34:39, akalin wrote:
> you only call this function twice, with very similar args.  I wonder if you
can
> simplify this a bit.  What about:
> 
> int64 CreateUnappliedAndUnsyncedItem(const string& name, syncable::ModelType)
{
> ...
> }
> 
> returns the metahandle, or an invalid metahandle on failure.  Uses
> MakeServer(name) for the item_id.  Uses id_factory_.root() for the parent. 
> is_folder = false.

Yes, that makes sense.  I wrote this function before I used it.  In hindsight,
it would have made sense to think about how it would be used.  

Unfortunately, the GTEST framework doesn't like functions that can fail *and*
return values.  They need to do one thing or the other.  

I decided downgraded the ASSERTs to EXPECTs.  I think this is safe because
ScopedDirLookup can't fail, and I've added code to handle the case where
entry.good() fails.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:157:
CHECK(item_id.ServerKnows());
On 2012/02/02 01:34:39, akalin wrote:
> you CHECK here but you ASSERT_TRUE later in the function.  I think it's just
> better all around to have this return false and move the ASSERT_TRUE to the
> callers

Good point.  I would fix this, but in the latest patch this line no longer
exists.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:175:
entry.Put(syncable::SERVER_VERSION, next_revision_++);
On 2012/02/02 01:34:39, akalin wrote:
> use GetNextRevision() here, too?

Done.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:258: string
root_server_id = syncable::GetNullId().GetServerId();
On 2012/02/02 01:34:39, akalin wrote:
> this variable doesn't seem to be used

Done.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:266:
sessions::StatusController* status = session()->mutable_status_controller();
On 2012/02/02 01:34:39, akalin wrote:
> can you use the non-mutable status_controller accessor instead?  is there a
> non-mutable conflict_progress() accessor?

The ScopedModelSafeGroupRestriction needs to modify the StatusController.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:274: //
modifications *and* the remote modification can not be applied without
On 2012/02/02 01:34:39, akalin wrote:
> can not -> cannot

Done.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:300:
sessions::StatusController* status = session()->mutable_status_controller();
On 2012/02/02 01:34:39, akalin wrote:
> all these lines are just to check HierarchyConflictingItemsSize() and they're
> repeated across multiple tests.  Decomp into helper fn?

I'm on the fence about that.  

If we pass in the expectations, then they would probably end up with something
like this:

ExpectConflictCounts(GROUP_UI, 0, 1, 0, 0)

Which is hard to read if you haven't memorized the function parameter names.

On the other hand, if we return the structs, we would have:

UpdateProgress update_progress = GetUpdateProgress(GROUP_UI);
ConflcitProgress conflict_progress = GetConflictProgress(GROUP_UI);

Which is not shorter and hides what's actually going on, making it a bit harder
to understand.  

Can you think of other arguments in favor of or against these alternatives?

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:337:
CHECK(entry.PutPredecessor(id_factory_.root()));
On 2012/02/02 01:34:39, akalin wrote:
> ASSERT_TRUE

Done.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:424:
entry.Put(syncable::BASE_VERSION, GetNextRevision());
On 2012/02/02 01:34:39, akalin wrote:
> i feel like you can decomp this into some helper function that the other block
> with lots of entry.Put()s can use.

That's a good idea.  Implemented in the next patch set.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/c...
File chrome/browser/sync/engine/conflict_resolver.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/c...
chrome/browser/sync/engine/conflict_resolver.cc:82: // both UNSYNCED and
UNAPPLIED_UDPATEs.
On 2012/02/02 21:53:41, nzea wrote:
> are both -> have both IS_UNSYNCED and IS_UNAPPLIED_UPDATE set.

Done.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_util.cc:314: // treated as a non-blocking
conflict. See the description in syncer_types.h.
On 2012/02/02 21:53:41, nzea wrote:
> non-blocking -> unresolvable

Done.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/u...
File chrome/browser/sync/engine/update_applicator.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/u...
chrome/browser/sync/engine/update_applicator.cc:168: // the list of nonblocking
conflicts instead of normal conflicts.
On 2012/02/02 21:53:41, nzea wrote:
> blocking -> unresolvable

"Fixed" by removing these comments.  This is not the right place to explain what
we're doing or why.

The "why" can be better explained at the point where we detect conflicts and
choose which type of conflict to return (CONFLICT_ENCRYPTION, CONFLICT_SIMPLE,
etc.).

The "what" can be better explained in the context of the ConflictResolver's
function documentation.

Any comments left here are less likely to be read and more likely to grow stale
over time.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/engine/u...
chrome/browser/sync/engine/update_applicator.cc:175: // We add them to the list
of non-blocking conflicts, along with the
On 2012/02/02 21:53:41, nzea wrote:
> list of unresolvable conflicts

See above comment.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/session_state.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/session_state.cc:122: int
num_blocking_conflicting_updates,
On 2012/02/02 21:53:41, nzea wrote:
> num_resolvable (here and below)

See above comment.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/session_state.h:132: int
num_blocking_conflicting_updates,
On 2012/02/02 21:53:41, nzea wrote:
> num_resolvable (here and below)

Done, sort of.  I'm using "simple" instead of resolvable here.  I'm hoping that
will be easier to understand in the contexts where we're going to use these
values (ie. far away from ConflictResolver).

Let me know if you disagree with this choice of name.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/status_controller.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/status_controller.cc:266: int
StatusController::TotalNumBlockingConflictingItems() const {
On 2012/02/02 21:53:41, nzea wrote:
> Rename to TotalNumResolvableConflictingItems?

Done, sort of: TotalNumSimpleConflctingItems.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/status_controller_unittest.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/status_controller_unittest.cc:155:
status.mutable_update_progress()->AddAppliedUpdate(CONFLICT_ENCRYPTION,
On 2012/02/02 21:53:41, nzea wrote:
> Any reason you changed this? Should be valid as it was before right?

When changing the HasConflictingUpdates() function, I almost neglected to factor
the unresolvable conflicts into the result.  I hijacked this test to try to
catch those errors.

I justify this because I think it would be easier to accidentally write a
HasConflictingUpdates() function that ignores unresolvable conflicts that to
write one that would ignore simple conflicts.  

On second thought, I suppose it can't hurt to test both scenarios.  I'll add an
another unit test for this.

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/sync_session.cc (right):

http://codereview.chromium.org/9305001/diff/9001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session.cc:163:
status_controller_->TotalNumBlockingConflictingItems(),
On 2012/02/02 21:53:41, nzea wrote:
> numblocking -> numresolvable

Done, s/resolvable/simple/.

[Nicolas Zea, 2012-02-09 01:54:58]

Sorry for the delay, forgot this was on my plate.

LGTM

[akalin, 2012-02-10 23:11:53]

LGTM after comments

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:155:
EXPECT_TRUE(dir.good());
if dir is bad, probably want to return invalid metahandle, too

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:158:
EXPECT_TRUE(entry.good());
you can do

if(!entry.good()) {
  ADD_FAILURE();
  return -1;
}

to be less redundant

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:160: return -1;
use kInvalidMetaHandle from syncable.h

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:175:
EXPECT_TRUE(dir.good());
as above

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:188: return -1;
as above

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:198:
EXPECT_TRUE(entry.PutPredecessor(id_factory_.root()));
return invalidmetahandle here?

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/session...
File chrome/browser/sync/sessions/status_controller_unittest.cc (right):

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/session...
chrome/browser/sync/sessions/status_controller_unittest.cc:168:
TEST_F(StatusControllerTest, HasConflictingUpdates_NonBlockingUpdates) {
comment this test

[rlarocque, 2012-02-11 00:02:16]

Thanks for the reviews.

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:155:
EXPECT_TRUE(dir.good());
On 2012/02/10 23:11:54, akalin wrote:
> if dir is bad, probably want to return invalid metahandle, too

The dir is never bad.  Will fix anyway, just to be consistent.

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:158:
EXPECT_TRUE(entry.good());
On 2012/02/10 23:11:54, akalin wrote:
> you can do
> 
> if(!entry.good()) {
>   ADD_FAILURE();
>   return -1;
> }
> 
> to be less redundant

Good idea, thanks!

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:160: return -1;
On 2012/02/10 23:11:54, akalin wrote:
> use kInvalidMetaHandle from syncable.h

I was looking for something like that.  Will do.

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:175:
EXPECT_TRUE(dir.good());
On 2012/02/10 23:11:54, akalin wrote:
> as above

Done.

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:188: return -1;
On 2012/02/10 23:11:54, akalin wrote:
> as above

Done.

http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
chrome/browser/sync/engine/apply_updates_command_unittest.cc:198:
EXPECT_TRUE(entry.PutPredecessor(id_factory_.root()));
On 2012/02/10 23:11:54, akalin wrote:
> return invalidmetahandle here?

Done.

[akalin, 2012-02-13 22:24:34]

On 2012/02/11 00:02:16, rlarocque wrote:
> Thanks for the reviews.
> 
>
http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
> File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):
> 
>
http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
> chrome/browser/sync/engine/apply_updates_command_unittest.cc:155:
> EXPECT_TRUE(dir.good());
> On 2012/02/10 23:11:54, akalin wrote:
> > if dir is bad, probably want to return invalid metahandle, too
> 
> The dir is never bad.  Will fix anyway, just to be consistent.

Oh, right.  Can we then simply remove good()? (in another CL)

[rlarocque, 2012-02-14 00:01:59]

On 2012/02/13 22:24:34, akalin wrote:
> On 2012/02/11 00:02:16, rlarocque wrote:
> > Thanks for the reviews.
> > 
> >
>
http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
> > File chrome/browser/sync/engine/apply_updates_command_unittest.cc (right):
> > 
> >
>
http://codereview.chromium.org/9305001/diff/10010/chrome/browser/sync/engine/...
> > chrome/browser/sync/engine/apply_updates_command_unittest.cc:155:
> > EXPECT_TRUE(dir.good());
> > On 2012/02/10 23:11:54, akalin wrote:
> > > if dir is bad, probably want to return invalid metahandle, too
> > 
> > The dir is never bad.  Will fix anyway, just to be consistent.
> 
> Oh, right.  Can we then simply remove good()? (in another CL)

I've gotten sufficiently fed up with it that I created a local branch to
experiment with removing ScopedDirLookup altogether.  It's a difficult change,
but it's probably worth it in the long run.

[akalin, 2012-02-15 23:14:20]

are you still planning to land this?

[rlarocque, 2012-02-16 03:22:23]

On 2012/02/15 23:14:20, akalin wrote:
> are you still planning to land this?

That was the plan, though I wanted to better understand the trybot failure
first.  

I could always remove the failing assertion then commit.  This would not be
unsafe since my changes do not require the assertion to succeed in order to
function correctly.

[rlarocque, 2012-02-18 00:33:46]

On 2012/02/16 03:22:23, rlarocque wrote:
> On 2012/02/15 23:14:20, akalin wrote:
> > are you still planning to land this?
> 
> That was the plan, though I wanted to better understand the trybot failure
> first.  
> 
> I could always remove the failing assertion then commit.  This would not be
> unsafe since my changes do not require the assertion to succeed in order to
> function correctly.

Hooray, green trybots!  Removing the assertion fixed it.  

If there's no objection, I'll commit this early next week.  Let me know if you
would like more time to review.

[commit-bot: I haz the power, 2012-02-21 20:26:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/9305001/34001

[commit-bot: I haz the power, 2012-02-21 21:11:54]

Try job failure for 9305001-34001 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-21 21:18:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/9305001/34001

[commit-bot: I haz the power, 2012-02-21 22:49:28]

Change committed as 122894