Bug 1395703 - Make sure modifiedBySync CV field isn't passed to ContentProvider on updates r=rnewman Comment from bugzilla on this ugly hack: While processing bookmarks, we sometimes need to mark them for re-upload as we're inserting new ones or updating existing ones. For example, we might set or update a dateAdded field. We perform insertions "in-bulk", and so we might be inserting some bookmarks which need to be re-uploaded, and some bookmarks which don't. We compile an array of ContentValue objects, and make a single call to our ContentProvider. This means we can't use a URI param to indicate our intent, and so a non-column field in ContentValues objects - modifiedFromSync - is set for those bookmarks which need special treatment during insertion. Bug 1392802 added a similar mechanism for updating bookmarks. However, updates are done differently - currently, we perform a single call to our ContentProvider for each bookmark. Which means we _can_ use a URI field as a signaling mechanism, which is what that patch did. However, in haste I forgot to take into consideration existing signaling mechanism, which lead to update failures. And so we're left with an even clumsier interface to our data store, with two ways to signal the same thing in different circumstances... A quick solution is to just make sure 'modifiedBySync' field never makes its way to contentprovider on updates; a more refined fix would probably modify update logic to use a ContentValues field for consistency... Either way, there's going to be something ugly, somewhere in the code. I anticipate a lot of this code changing sometime soon in order to support better transactionality of bookmark syncing, and smarter merging, and so I'm inclined to just to the simple thing for now. MozReview-Commit-ID: H10LFsqjbFY

--- 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
@@ -110,16 +110,21 @@ public class BookmarksDataAccessor exten
 
   /**
    * Update a record identified by GUID to new values, but only if assertion passes that localVersion
    * did not change. This is expected to be called during record reconciliation, and so we also request
    * that localVersion is incremented in the process.
    */
   /* package-private */ boolean updateAssertingLocalVersion(String guid, int expectedLocalVersion, boolean shouldIncrementLocalVersion, Record newRecord) {
     final ContentValues cv = getContentValues(newRecord);
+
+    // A (hopefully) temporary hack, see https://bugzilla.mozilla.org/show_bug.cgi?id=1395703#c1
+    // We don't need this flag in CVs, since we're signaling the same thing via uri (see below).
+    cv.remove(BrowserContract.Bookmarks.PARAM_INSERT_FROM_SYNC_AS_MODIFIED);
+
     final Bundle data = new Bundle();
     data.putString(BrowserContract.SyncColumns.GUID, guid);
     data.putInt(BrowserContract.VersionColumns.LOCAL_VERSION, expectedLocalVersion);
     data.putParcelable(BrowserContract.METHOD_PARAM_DATA, cv);
 
     final Uri callUri;
     if (shouldIncrementLocalVersion) {
       callUri = withLocalVersionIncrement(getUri());