Bug 1153357 - Don't set SYNC_STATUS_MODIFIED unless an update touches fields that we sync. r=nalexander, a=sledru
authorRichard Newman <rnewman@mozilla.com>
Mon, 13 Apr 2015 12:37:30 -0700
changeset 258488 199b60ec60dc
parent 258487 97856a6ac44d
child 258489 df47a99c442f
push id4679
push userryanvm@gmail.com
push date2015-04-15 17:06 +0000
treeherdermozilla-beta@6c7e8d9f955c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnalexander, sledru
bugs1153357
milestone38.0
Bug 1153357 - Don't set SYNC_STATUS_MODIFIED unless an update touches fields that we sync. r=nalexander, a=sledru This patch does two things: * It doesn't set MODIFIED at all unless fields are being changed. That's the original bug. * It only sets MODIFIED in a non-Sync update if the item is marked as SYNCED. NEW items will stay new even if Fennec changes them, until they've been synced.
mobile/android/base/db/DBUtils.java
mobile/android/base/db/ReadingListProvider.java
--- a/mobile/android/base/db/DBUtils.java
+++ b/mobile/android/base/db/DBUtils.java
@@ -206,18 +206,39 @@ public class DBUtils {
     }
 
     public static void updateArraysBlindly(SQLiteDatabase db, String table, ContentValues[] values, UpdateOperation[] ops, String whereClause, String[] whereArgs) {
         updateArraysWithOnConflict(db, table, values, ops, whereClause, whereArgs, CONFLICT_NONE, false);
     }
 
     @RobocopTarget
     public enum UpdateOperation {
+        /**
+         * ASSIGN is the usual update: replaces the value in the named column with the provided value.
+         *
+         * foo = ?
+         */
         ASSIGN,
+
+        /**
+         * BITWISE_OR applies the provided value to the existing value with a bitwise OR. This is useful for adding to flags.
+         *
+         * foo |= ?
+         */
         BITWISE_OR,
+
+        /**
+         * EXPRESSION is an end-run around the API: it allows callers to specify a fragment of SQL to splice into the
+         * SET part of the query.
+         *
+         * foo = $value
+         *
+         * Be very careful not to use user input in this.
+         */
+        EXPRESSION,
     }
 
     /**
      * This is an evil reimplementation of SQLiteDatabase's methods to allow for
      * smarter updating.
      *
      * Each ContentValues has an associated enum that describes how to unify input values with the existing column values.
      */
@@ -240,17 +261,20 @@ public class DBUtils {
         sql.append("UPDATE ");
         sql.append(CONFLICT_VALUES[conflictAlgorithm]);
         sql.append(table);
         sql.append(" SET ");
 
         // move all bind args to one array
         int setValuesSize = 0;
         for (int i = 0; i < values.length; i++) {
-            setValuesSize += values[i].size();
+            // EXPRESSION types don't contribute any placeholders.
+            if (ops[i] != UpdateOperation.EXPRESSION) {
+                setValuesSize += values[i].size();
+            }
         }
 
         int bindArgsSize = (whereArgs == null) ? setValuesSize : (setValuesSize + whereArgs.length);
         Object[] bindArgs = new Object[bindArgsSize];
 
         int arg = 0;
         for (int i = 0; i < values.length; i++) {
             final ContentValues v = values[i];
@@ -272,16 +296,26 @@ public class DBUtils {
                         final String colName = entry.getKey();
                         sql.append((arg > 0) ? "," : "");
                         sql.append(colName);
                         bindArgs[arg++] = entry.getValue();
                         sql.append("= ? | ");
                         sql.append(colName);
                     }
                     break;
+                case EXPRESSION:
+                    // Treat each value as a literal SQL string.
+                    for (Map.Entry<String, Object> entry : v.valueSet()) {
+                        final String colName = entry.getKey();
+                        sql.append((arg > 0) ? "," : "");
+                        sql.append(colName);
+                        sql.append(" = ");
+                        sql.append(entry.getValue());
+                    }
+                    break;
             }
         }
 
         if (whereArgs != null) {
             for (arg = setValuesSize; arg < bindArgsSize; arg++) {
                 bindArgs[arg] = whereArgs[arg - setValuesSize];
             }
         }
--- a/mobile/android/base/db/ReadingListProvider.java
+++ b/mobile/android/base/db/ReadingListProvider.java
@@ -7,16 +7,17 @@ package org.mozilla.gecko.db;
 import android.content.ContentUris;
 import android.content.ContentValues;
 import android.content.UriMatcher;
 import android.database.Cursor;
 import android.database.sqlite.SQLiteDatabase;
 import android.database.sqlite.SQLiteQueryBuilder;
 import android.net.Uri;
 import android.text.TextUtils;
+import org.mozilla.gecko.db.DBUtils.UpdateOperation;
 
 import static org.mozilla.gecko.db.BrowserContract.ReadingListItems.*;
 
 public class ReadingListProvider extends SharedBrowserDatabaseProvider {
     static final String TABLE_READING_LIST = TABLE_NAME;
 
     static final int ITEMS = 101;
     static final int ITEMS_ID = 102;
@@ -63,29 +64,26 @@ public class ReadingListProvider extends
         }
         return updated;
     }
 
     /**
      * This method does two things:
      * * Based on the values provided, it computes and returns an incremental status change
      *   that can be applied to the database to track changes for syncing. This should be
-     *   applied with {@link org.mozilla.gecko.db.DBUtils.UpdateOperation#BITWISE_OR}.
+     *   applied with {@link UpdateOperation#BITWISE_OR}.
      * * It mutates the provided values to mark absolute field changes.
      *
      * @return null if no values were provided, or no change needs to be recorded.
      */
     private ContentValues processChangeValues(ContentValues values) {
         if (values == null || values.size() == 0) {
             return null;
         }
 
-        // Otherwise, it must have been modified.
-        values.put(SYNC_STATUS, SYNC_STATUS_MODIFIED);
-
         final ContentValues out = new ContentValues();
         int flag = 0;
         if (values.containsKey(MARKED_READ_BY) ||
             values.containsKey(MARKED_READ_ON) ||
             values.containsKey(IS_UNREAD)) {
             flag |= SYNC_CHANGE_UNREAD_CHANGED;
         }
 
@@ -115,23 +113,30 @@ public class ReadingListProvider extends
     public int updateItemsWithFlags(Uri uri, ContentValues values, ContentValues flags, String selection, String[] selectionArgs) {
         trace("Updating ReadingListItems on URI: " + uri);
         final SQLiteDatabase db = getWritableDatabase(uri);
         if (!values.containsKey(CLIENT_LAST_MODIFIED)) {
             values.put(CLIENT_LAST_MODIFIED, System.currentTimeMillis());
         }
 
         if (flags == null) {
-            // Dunno what we're doing with the DB that isn't changing anything we care about, but hey.
+            // This code path is used by Sync. Bypass metadata changes.
             return db.update(TABLE_READING_LIST, values, selection, selectionArgs);
         }
 
-        // Otherwise, we need to do smart updating to change flags.
-        final ContentValues[] valuesAndFlags = {values, flags};
-        final DBUtils.UpdateOperation[] ops = {DBUtils.UpdateOperation.ASSIGN, DBUtils.UpdateOperation.BITWISE_OR};
+        // Set synced items to MODIFIED; otherwise, leave the sync status untouched.
+        final ContentValues setModified = new ContentValues();
+        setModified.put(SYNC_STATUS, "CASE " + SYNC_STATUS +
+                                     " WHEN " + SYNC_STATUS_SYNCED +
+                                     " THEN " + SYNC_STATUS_MODIFIED +
+                                     " ELSE " + SYNC_STATUS +
+                                     " END");
+
+        final ContentValues[] valuesAndFlags = {values, flags, setModified};
+        final UpdateOperation[] ops = {UpdateOperation.ASSIGN, UpdateOperation.BITWISE_OR, UpdateOperation.EXPRESSION};
 
         return DBUtils.updateArrays(db, TABLE_READING_LIST, valuesAndFlags, ops, selection, selectionArgs);
     }
 
     /**
      * Inserts a new item into the DB. CLIENT_LAST_MODIFIED is generated if it is not specified.
      *
      * Non-Sync callers will have ADDED_ON and ADDED_BY set appropriately if they are missing;