Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(132)

Issue 9107055: Remove single direction conflict set code (Closed)

Created:
8 years, 11 months ago by rlarocque
Modified:
8 years, 11 months ago
CC:
chromium-reviews, Raghu Simha, ncarter (slow), akalin, tim (not reviewing)
Visibility:
Public.

Description

Remove single direction conflict set code This code was intended to operate on conflict sets that consist solely of changes from the server. However, we can never detect these kinds of conflict sets because our conflict set detection code assumes that the server never sends us an invalid set of changes; all conflict sets must be cause, at least in part, by local changes. This change also deletes various private and hidden functions that were used only by the single direction conflict set code. This reduced the BuildAndProcessConflictSetsCommand::ExecuteImpl() into a single function call, which I inlined. Also removed in this change is the conflict_sets_built() flag. The single direction conflict set processing code was the only place where its value was "set". It seems that the logic that relied on it in syncer.cc was reversed. We only processed conflicts if !conflict_sets_built(), which is the opposite of what the documentation claims. Fortunately, its value was always false (because there were no single direction conflict sets) so we always processed conflicts. Finally, this CL includes one unrelated change: it removes BuildAndProcessConflictSetsCommand::MergeSetsForNameClash(). That function is not defined anywhere, so I decided to remove its declaration. BUG=107816 TEST= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=119507

Patch Set 1 #

Patch Set 2 : Rebase #

Patch Set 3 : One more rebase fix #

Patch Set 4 : Pre-emptive rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -234 lines) Patch
M chrome/browser/sync/engine/build_and_process_conflict_sets_command.h View 1 chunk +0 lines, -16 lines 0 comments Download
M chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc View 1 chunk +5 lines, -187 lines 0 comments Download
M chrome/browser/sync/engine/syncer.cc View 1 1 chunk +1 line, -4 lines 0 comments Download
M chrome/browser/sync/sessions/session_state.h View 1 2 3 1 chunk +1 line, -6 lines 0 comments Download
M chrome/browser/sync/sessions/status_controller.h View 1 2 3 2 chunks +0 lines, -4 lines 0 comments Download
M chrome/browser/sync/sessions/status_controller.cc View 1 2 3 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/sync/sessions/status_controller_unittest.cc View 1 2 3 2 chunks +0 lines, -6 lines 0 comments Download
M chrome/browser/sync/sessions/sync_session.cc View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/sync/sessions/sync_session_unittest.cc View 1 chunk +0 lines, -7 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
rlarocque
Take your time in reviewing this. I've got plenty of other changes to commit before ...
8 years, 11 months ago (2012-01-12 22:39:15 UTC) #1
tim (not reviewing)
Nice patch. I looked at this for a while, and your logic definitely seems sound. ...
8 years, 11 months ago (2012-01-24 05:01:16 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rlarocque@chromium.org/9107055/10001
8 years, 11 months ago (2012-01-27 18:58:33 UTC) #3
commit-bot: I haz the power
8 years, 11 months ago (2012-01-27 23:05:46 UTC) #4
Change committed as 119507

Powered by Google App Engine
This is Rietveld 408576698