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

Issue 1142423006: Sync: fixed Implicit Permanent Folders issues found when testing with alpha server (Closed)

Created:
5 years, 7 months ago by stanisc
Modified:
5 years, 6 months ago
Reviewers:
pavely
CC:
chromium-reviews, tim+watch_chromium.org, zea+watch_chromium.org, maxbogue+watch_chromium.org, pvalenzuela+watch_chromium.org, plaree+watch_chromium.org, maniscalco+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fixed Implicit Permanent Folders issues found when testing with alpha server This addresses two more issues found when testing the client with Sync Server Alpha server with Implicit Permanent Folders feature enabled. Additionally this change bumps the protocol version to 45 - the version required on the server to turn this feature on. The issues fixed are: 1) Directory::InitialSyncEndedForType expected the permanent folder to be created from a server update. That isn't the case anymore. Permanent folders are created on demand on the client when processing updates. I've simplified the check to skip the base version check. 2) In Traversal::SupportsHierarchy the check for whether an item supports hierarchy was based on presence of Parent ID field. That cased an issue committing a new DeviceInfo which still had Parent ID specified - an attempt to commit the client side created DeviceInfo permanent folder to the server. Even if the type uses implicit permanent folders it might for a transition period still have a mix of items with and without the parent ID. The hierarchy traversal for commit should use a static check based on the type rather than checking each item's Parent ID individually. I tested sync between a client with this fixes connected to the Alpha server and a regular client connected to the regular server and didn't find any further issues. BUG=438313 Committed: https://crrev.com/b19a0a0cb7b4c2b1757c2fc828e10a93e55ac8ae Cr-Commit-Position: refs/heads/master@{#331717}

Patch Set 1 #

Patch Set 2 : Bumped protocol version #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -4 lines) Patch
M sync/engine/get_commit_ids.cc View 1 chunk +2 lines, -1 line 0 comments Download
M sync/protocol/sync.proto View 1 1 chunk +1 line, -1 line 0 comments Download
M sync/syncable/directory.cc View 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 8 (2 generated)
stanisc
PTAL!
5 years, 7 months ago (2015-05-21 18:24:44 UTC) #2
stanisc
Pavel, please take a look when you have time.
5 years, 7 months ago (2015-05-27 05:55:11 UTC) #3
pavely
lgtm
5 years, 6 months ago (2015-05-27 21:44:56 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1142423006/20001
5 years, 6 months ago (2015-05-27 23:43:28 UTC) #6
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 6 months ago (2015-05-28 02:04:46 UTC) #7
commit-bot: I haz the power
5 years, 6 months ago (2015-05-28 02:05:37 UTC) #8
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/b19a0a0cb7b4c2b1757c2fc828e10a93e55ac8ae
Cr-Commit-Position: refs/heads/master@{#331717}

Powered by Google App Engine
This is Rietveld 408576698