Bug 1335198 - Add support for synchronizing bookmark creation date r=rnewman

Incoming records might be missing the dateAdded field, and so we perform some pre-processing:
- during reconciliation, dateAdded is set to the lowest of (remote lastModified, remote dateAdded, local dateAdded)
- during insertion, if dateAdded is missing it is set to lastModified

Whenever we modify dateAdded for a record during sync, we also bump its lastModified value. This will trigger an
upload of this record, and consequently a re-upload by clients which are able to provide an older dateAdded value.
It is possible that this might cause conflicts on other devices, but the expected likelyhood of that happening is low.


MozReview-Commit-ID: 3tDeXKSBgrO

--HG--
extra : rebase_source : 26cb13838df7a4adb6d4fe3c51f0ecf3fd2eda95

mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksDataAccessor.java

@@ -292,6 +292,11 @@ public class AndroidBrowserBookmarksDataAccessor extends AndroidBrowserRepositor
     cv.put(BrowserContract.Bookmarks.TITLE,       rec.title);
     cv.put(BrowserContract.Bookmarks.URL,         rec.bookmarkURI);
     cv.put(BrowserContract.Bookmarks.DESCRIPTION, rec.description);
+
+    if (rec.dateAdded != null) {
+      cv.put(BrowserContract.Bookmarks.DATE_CREATED, rec.dateAdded);
+    }
+
     if (rec.tags == null) {
       rec.tags = new JSONArray();
     }

mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java

@@ -699,6 +699,34 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
     }
   }
 
+  /**
+   * Incoming bookmark records might be missing dateAdded field, because clients started to sync it
+   * only in version 55. However, record's lastModified value is a good upper bound. In order to
+   * encourage modern clients to re-upload the record with an earlier local dateAdded value,
+   * we bump record's lastModified value if we perform any substitutions.
+   *
+   * 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 = now();
+      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 = now();
+      return toProcess;
+    }
+
+    return toProcess;
+  }
+
   @Override
   protected Record reconcileRecords(Record remoteRecord, Record localRecord,
                                     long lastRemoteRetrieval,
@@ -708,15 +736,40 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
                                                                         lastRemoteRetrieval,
                                                                         lastLocalRetrieval);
 
+    final BookmarkRecord remote = (BookmarkRecord) remoteRecord;
+    final BookmarkRecord local = (BookmarkRecord) localRecord;
+
     // For now we *always* use the remote record's children array as a starting point.
     // We won't write it into the database yet; we'll record it and process as we go.
-    reconciled.children = ((BookmarkRecord) remoteRecord).children;
+    reconciled.children = remote.children;
 
     // *Always* track folders, though: if we decide we need to reposition items, we'll
     // untrack later.
     if (reconciled.isFolder()) {
       trackRecord(reconciled);
     }
+
+    // We should always have:
+    // - local dateAdded
+    // - lastModified values for both records
+    // We might not have the remote dateAdded.
+    // 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) {
+      lowest = local.dateAdded;
+    }
+
+    if (remote.dateAdded != null && remote.dateAdded < lowest && remote.dateAdded > releaseOfNCSAMosaicMillis) {
+      lowest = remote.dateAdded;
+    }
+
+    reconciled.dateAdded = lowest;
+
     return reconciled;
   }
 
@@ -1089,6 +1142,7 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
     rec.description = RepoUtils.getStringFromCursor(cur, BrowserContract.Bookmarks.DESCRIPTION);
     rec.tags = RepoUtils.getJSONArrayFromCursor(cur, BrowserContract.Bookmarks.TAGS);
     rec.keyword = RepoUtils.getStringFromCursor(cur, BrowserContract.Bookmarks.KEYWORD);
+    rec.dateAdded = RepoUtils.getLongFromCursor(cur, BrowserContract.SyncColumns.DATE_CREATED);
 
     rec.androidID = RepoUtils.getLongFromCursor(cur, BrowserContract.Bookmarks._ID);
     rec.androidPosition = RepoUtils.getLongFromCursor(cur, BrowserContract.Bookmarks.POSITION);

mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserRepositorySession.java

@@ -484,7 +484,7 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos
           if (existingRecord == null) {
             // The record is new.
             trace("No match. Inserting.");
-            insert(record);
+            insert(processBeforeInsertion(record));
             return;
           }
 
@@ -552,6 +552,13 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos
     storeDelegate.onRecordStoreSucceeded(record.guid);
   }
 
+  /**
+   * Identity function by default. Override in subclasses if necessary.
+   */
+  protected Record processBeforeInsertion(Record toProcess) {
+    return toProcess;
+  }
+
   protected void insert(Record record) throws NoGuidForIdException, NullCursorException, ParentNotFoundException {
     Record toStore = prepareRecord(record);
     Uri recordURI = dbHelper.insert(toStore);

mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/domain/BookmarkRecord.java

@@ -4,6 +4,8 @@
 
 package org.mozilla.gecko.sync.repositories.domain;
 
+import android.support.annotation.Nullable;
+
 import java.io.UnsupportedEncodingException;
 import java.net.URLEncoder;
 import java.util.Map;
@@ -57,6 +59,10 @@ public class BookmarkRecord extends Record {
   public String  type;
   public long    androidPosition;
 
+  // This field started to be synced by clients in v55, so there's a chance it will be `null`
+  // if this instance is initialized from a remote payload.
+  @Nullable public Long dateAdded;
+
   public JSONArray children;
   public JSONArray tags;
 
@@ -104,6 +110,7 @@ public class BookmarkRecord extends Record {
     out.androidParentID = this.androidParentID;
     out.type            = this.type;
     out.androidPosition = this.androidPosition;
+    out.dateAdded       = this.dateAdded;
 
     out.children        = this.copyChildren();
     out.tags            = this.copyTags();
@@ -174,6 +181,21 @@ public class BookmarkRecord extends Record {
     this.parentID    = payload.getString("parentid");
     this.parentName  = payload.getString("parentName");
 
+    if (!payload.containsKey("dateAdded")) {
+      // Missing data will be filled later on, during record insertion/reconciliation.
+      this.dateAdded = null;
+    } else {
+      // Oddly enough, in local testing both of these exception paths turned out to be necessary.
+      try {
+        this.dateAdded = payload.getLong("dateAdded");
+      } catch (ClassCastException e) {
+        this.dateAdded = Long.parseLong(payload.getString("dateAdded"));
+      } catch (Exception e) {
+        Logger.error(LOG_TAG, "Cannot parse bookmark's 'dateAdded' value. Setting it to null.", e);
+        this.dateAdded = null;
+      }
+    }
+
     if (isFolder()) {
       try {
         this.children = payload.getArray("children");
@@ -256,6 +278,16 @@ public class BookmarkRecord extends Record {
     putPayload(payload, "parentName", this.parentName);
     putPayload(payload, "keyword", this.keyword);
 
+    // This field is marked as nullable, so we're careful when unboxing it. However, we do not expect
+    // it to be null at this point, since it was constructed from our DB cursor which will always have
+    // a `created` timestamp.
+    // However, very old bookmarks that didn't have a corresponding history item might not have a
+    // creation date. `created` is not a NOT NULL field in the bookmarks table. Either way, it pays
+    // to be cautious here.
+    if (this.dateAdded != null) {
+      putPayload(payload, "dateAdded", Long.toString(this.dateAdded));
+    }
+
     if (isFolder()) {
       payload.put("children", this.children);
       return;
@@ -451,6 +483,7 @@ public class BookmarkRecord extends Record {
    type:          "bookmark",
    title:         "Your Flight Status",
    parentName:    "mobile",
+   dateAdded:     1490997969974,
    bmkUri:        "http: //www.flightstats.com/go/Mobile/flightStatusByFlightProcess.do;jsessionid=13A6C8DCC9592AF141A43349040262CE.web3: 8009?utm_medium=cpc&utm_campaign=co-op&utm_source=airlineInformationAndStatus&id=212492593",
    tags:          [],
    keyword:       null,
@@ -469,6 +502,7 @@ public class BookmarkRecord extends Record {
    parentName:  "",
    title:       "mobile",
    description: null,
+   dateAdded:   1490997969974,
    children:    ["1ROdlTuIoddD", "3Z_bMIHPSZQ8", "4mSDUuOo2iVB", "8aEdE9IIrJVr",
                  "9DzPTmkkZRDb", "Qwwb99HtVKsD", "s8tM36aGPKbq", "JMTi61hOO3JV",
                  "JQUDk0wSvYip", "LmVH-J1r3HLz", "NhgQlC5ykYGW", "OVanevUUaqO2",