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

Issue 1664643003: Change sync conflict resolution for extensions (Closed)

Created:
4 years, 10 months ago by asargent_no_longer_on_chrome
Modified:
4 years, 10 months ago
Reviewers:
Nicolas Zea
CC:
chromium-reviews, tim+watch_chromium.org, maxbogue+watch_chromium.org, plaree+watch_chromium.org, zea+watch_chromium.org, Marc Treib
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Change sync conflict resolution for extensions For extensions and apps, if the server has seen a delete action (corresponding to an uninstall) and the local state has a not-yet-synced change, we would prefer to resolve this conflict by keeping the server state and uninstalling locally. This helps to prevent uninstalled extensions "coming back to life". BUG=517910 Committed: https://crrev.com/d40211da2bf6d6b3a6d56e6f9bd26506249339b9 Cr-Commit-Position: refs/heads/master@{#376603}

Patch Set 1 #

Patch Set 2 : added comments and unit test #

Total comments: 17

Patch Set 3 : unit test changes, added integration test #

Total comments: 4

Patch Set 4 : moved entry type check lower down in ProcessSimpleConflict #

Unified diffs Side-by-side diffs Delta from patch set Stats (+168 lines, -25 lines) Patch
M chrome/browser/sync/test/integration/single_client_extensions_sync_test.cc View 1 2 2 chunks +50 lines, -0 lines 0 comments Download
M sync/engine/conflict_resolver.cc View 1 2 3 2 chunks +37 lines, -21 lines 0 comments Download
M sync/engine/syncer_unittest.cc View 1 2 3 chunks +81 lines, -4 lines 0 comments Download

Messages

Total messages: 15 (5 generated)
asargent_no_longer_on_chrome
https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unittest.cc File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unittest.cc#newcode3778 sync/engine/syncer_unittest.cc:3778: extension.PutIsUnsynced(true); Note: a bunch of the code I use ...
4 years, 10 months ago (2016-02-11 00:59:37 UTC) #2
Nicolas Zea
https://codereview.chromium.org/1664643003/diff/20001/sync/engine/conflict_resolver.cc File sync/engine/conflict_resolver.cc (right): https://codereview.chromium.org/1664643003/diff/20001/sync/engine/conflict_resolver.cc#newcode118 sync/engine/conflict_resolver.cc:118: // where the non_unique_name or parent don't match. Another ...
4 years, 10 months ago (2016-02-11 23:41:50 UTC) #3
asargent_no_longer_on_chrome
Thanks for the review! Please take another look. https://codereview.chromium.org/1664643003/diff/20001/sync/engine/conflict_resolver.cc File sync/engine/conflict_resolver.cc (right): https://codereview.chromium.org/1664643003/diff/20001/sync/engine/conflict_resolver.cc#newcode118 sync/engine/conflict_resolver.cc:118: // ...
4 years, 10 months ago (2016-02-18 00:53:45 UTC) #6
Nicolas Zea
LG after the conditional move. https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unittest.cc File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unittest.cc#newcode3815 sync/engine/syncer_unittest.cc:3815: extension.PutSpecifics(specifics); On 2016/02/18 00:53:45, ...
4 years, 10 months ago (2016-02-18 19:28:55 UTC) #7
Nicolas Zea
https://codereview.chromium.org/1664643003/diff/80001/sync/engine/conflict_resolver.cc File sync/engine/conflict_resolver.cc (right): https://codereview.chromium.org/1664643003/diff/80001/sync/engine/conflict_resolver.cc#newcode245 sync/engine/conflict_resolver.cc:245: // The entry is deleted on the server but ...
4 years, 10 months ago (2016-02-18 19:30:31 UTC) #8
asargent_no_longer_on_chrome
PTAL https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unittest.cc File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unittest.cc#newcode3815 sync/engine/syncer_unittest.cc:3815: extension.PutSpecifics(specifics); With the modification you suggested in conflict_resolver.cc, ...
4 years, 10 months ago (2016-02-19 23:59:53 UTC) #9
Nicolas Zea
LGTM, thanks!
4 years, 10 months ago (2016-02-20 00:08:04 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1664643003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1664643003/100001
4 years, 10 months ago (2016-02-20 00:46:09 UTC) #12
commit-bot: I haz the power
Committed patchset #4 (id:100001)
4 years, 10 months ago (2016-02-20 01:49:23 UTC) #13
commit-bot: I haz the power
4 years, 10 months ago (2016-02-20 01:50:42 UTC) #15
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/d40211da2bf6d6b3a6d56e6f9bd26506249339b9
Cr-Commit-Position: refs/heads/master@{#376603}

Powered by Google App Engine
This is Rietveld 408576698