Bug 1392802 - Increment localVersion while reconciling a bookmark if we modified its dateAdded t.s. r=rnewman draft
authorGrigory Kruglov <gkruglov@mozilla.com>
Tue, 29 Aug 2017 20:12:46 -0400
changeset 655405 3f8ac41de99d7082cd9d7fc7254386d99d5431bd
parent 655385 db7f19e26e571ae1dd309f5d2f387b06ba670c30
child 728830 f1b9cbbf1246495e15ea54e10eb846428be048c2
push id76870
push userbmo:gkruglov@mozilla.com
push dateWed, 30 Aug 2017 00:15:34 +0000
reviewersrnewman
bugs1392802
milestone57.0a1
Bug 1392802 - Increment localVersion while reconciling a bookmark if we modified its dateAdded t.s. r=rnewman We might decide that there's an older dateAdded timestamp present for an incoming bookmark while processing it, in which case we need to ensure that our changes will be uploaded. MozReview-Commit-ID: BKLh4rYBiRu
mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksDataAccessor.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksSessionHelper.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/domain/Record.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
@@ -158,17 +158,17 @@ public class BrowserContract {
 
     @RobocopTarget
     public interface VersionColumns {
         String LOCAL_VERSION = "localVersion";
         String SYNC_VERSION = "syncVersion";
 
         // This is a flag used to indicate that records inserted from sync should start as modified.
         // That is, their versions will start as (2,1), instead of (1,1).
-        String PARAM_INSERT_FROM_SYNC_AS_MODIFIED = "insertFromSyncAsModified";
+        String PARAM_INSERT_FROM_SYNC_AS_MODIFIED = "modifiedBySync";
     }
 
     @RobocopTarget
     public interface SyncColumns extends DateSyncColumns {
         public static final String GUID = "guid";
         public static final String IS_DELETED = "deleted";
     }
 
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksDataAccessor.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksDataAccessor.java
@@ -339,17 +339,17 @@ public class BookmarksDataAccessor exten
 
     if (rec.tags == null) {
       rec.tags = new JSONArray();
     }
 
     // We might want to indicate that this record is to be inserted as "modified". Incoming records
     // might be modified as we're processing them for insertion, and so should be re-uploaded.
     // Our data provider layer manages versions, so we don't pass in localVersion explicitly.
-    if (rec.insertFromSyncAsModified) {
+    if (rec.modifiedBySync) {
       cv.put(BrowserContract.Bookmarks.PARAM_INSERT_FROM_SYNC_AS_MODIFIED, true);
     }
 
     cv.put(BrowserContract.Bookmarks.TAGS,        rec.tags.toJSONString());
     cv.put(BrowserContract.Bookmarks.KEYWORD,     rec.keyword);
     cv.put(BrowserContract.Bookmarks.PARENT,      rec.androidParentID);
     cv.put(BrowserContract.Bookmarks.POSITION,    rec.androidPosition);
 
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksSessionHelper.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksSessionHelper.java
@@ -287,26 +287,26 @@ import java.util.concurrent.ConcurrentHa
      * This function is only called while inserting a record which doesn't exist locally.
      */
     @Override
     protected Record processBeforeInsertion(Record toProcess) {
         // If incoming record is missing dateAdded, use its lastModified value instead.
         if (((BookmarkRecord) toProcess).dateAdded == null) {
             ((BookmarkRecord) toProcess).dateAdded = toProcess.lastModified;
             toProcess.lastModified = RepositorySession.now();
-            toProcess.insertFromSyncAsModified = true;
+            toProcess.modifiedBySync = true;
             return toProcess;
         }
 
         // If both are present, use the lowest value. We trust server's monotonously increasing timestamps
         // more than clients' potentially bogus ones.
         if (toProcess.lastModified < ((BookmarkRecord) toProcess).dateAdded) {
             ((BookmarkRecord) toProcess).dateAdded = toProcess.lastModified;
             toProcess.lastModified = RepositorySession.now();
-            toProcess.insertFromSyncAsModified = true;
+            toProcess.modifiedBySync = true;
             return toProcess;
         }
 
         return toProcess;
     }
 
     /**
      * Implement method of BookmarksInsertionManager.BookmarkInserter.
@@ -428,21 +428,22 @@ import java.util.concurrent.ConcurrentHa
                     Logger.error(LOG_TAG, "Error finishing up store: " + e);
                 }
             }
         };
     }
 
     @Override
     /* package-private */ boolean doReplaceRecord(Record toStore, Record existingRecord, RepositorySessionStoreDelegate delegate) throws NoGuidForIdException, NullCursorException, ParentNotFoundException {
-        // Request that localVersion is bumped if we're working with a record that also changed
-        // locally since we last synced. We do this so that any merged changes are uploaded.
+        // Request that localVersion is bumped if an incoming record has been modified upstream, or
+        // if we're working with a record that also changed locally since we last synced.
+        // We do this so that any merged changes are uploaded.
         // We simply replace and don't subsequently upload records which didn't change locally
         // since our last sync.
-        boolean shouldIncrementLocalVersion = existingRecord.localVersion > existingRecord.syncVersion;
+        boolean shouldIncrementLocalVersion = toStore.modifiedBySync || (existingRecord.localVersion > existingRecord.syncVersion);
         Record replaced = replaceWithAssertion(
                 toStore,
                 existingRecord,
                 shouldIncrementLocalVersion
         );
         if (replaced == null) {
             return false;
         }
@@ -509,20 +510,22 @@ import java.util.concurrent.ConcurrentHa
         // We always pick the lowest value out of what is available.
         long lowest = remote.lastModified;
 
         // During a similar operation, desktop clients consider dates before Jan 23, 1993 to be invalid.
         // We do the same here out of a desire to be consistent.
         final long releaseOfNCSAMosaicMillis = 727747200000L;
 
         if (local.dateAdded != null && local.dateAdded < lowest && local.dateAdded > releaseOfNCSAMosaicMillis) {
+            reconciled.modifiedBySync = true;
             lowest = local.dateAdded;
         }
 
         if (remote.dateAdded != null && remote.dateAdded < lowest && remote.dateAdded > releaseOfNCSAMosaicMillis) {
+            reconciled.modifiedBySync = true;
             lowest = remote.dateAdded;
         }
 
         reconciled.dateAdded = lowest;
 
         return reconciled;
     }
 
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/domain/Record.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/domain/Record.java
@@ -72,17 +72,17 @@ public abstract class Record {
   public String collection;
   public long lastModified;
   public boolean deleted;
   public long androidID;
 
   // Not all record types support versioning. See Bug 1383894.
   @Nullable public Integer localVersion;
   @Nullable public Integer syncVersion;
-  public boolean insertFromSyncAsModified = false;
+  public boolean modifiedBySync = false;
 
   /**
    * An integer indicating the relative importance of this item in the collection.
    * <p>
    * Default is 0.
    */
   public long sortIndex;
   /**