Bug 1274029 - Part 3: Update aggregates during syncing r=sebastian draft
authorGrigory Kruglov <gkruglov@mozilla.com>
Wed, 01 Jun 2016 10:20:59 -0700
changeset 374082 6c731e5085a0fd86f2ca1879fdeaabb02717799a
parent 374081 de9a104308a9e634e867f09986662dc819cf625d
child 374083 2a26b46931e2211ca4c5f2dfcd1e9fd9c8f9116a
push id19919
push usergkruglov@mozilla.com
push dateWed, 01 Jun 2016 19:30:11 +0000
reviewerssebastian
bugs1274029
milestone49.0a1
Bug 1274029 - Part 3: Update aggregates during syncing r=sebastian MozReview-Commit-ID: 9WyBE0Lk3Tp
mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryDataAccessor.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
@@ -45,16 +45,17 @@ public class BrowserContract {
     public static final String PARAM_PROFILE_PATH = "profilePath";
     public static final String PARAM_LIMIT = "limit";
     public static final String PARAM_SUGGESTEDSITES_LIMIT = "suggestedsites_limit";
     public static final String PARAM_IS_SYNC = "sync";
     public static final String PARAM_SHOW_DELETED = "show_deleted";
     public static final String PARAM_IS_TEST = "test";
     public static final String PARAM_INSERT_IF_NEEDED = "insert_if_needed";
     public static final String PARAM_INCREMENT_VISITS = "increment_visits";
+    public static final String PARAM_INCREMENT_REMOTE_AGGREGATES = "increment_remote_aggregates";
     public static final String PARAM_EXPIRE_PRIORITY = "priority";
     public static final String PARAM_DATASET_ID = "dataset_id";
     public static final String PARAM_GROUP_BY = "group_by";
 
     static public enum ExpirePriority {
         NORMAL,
         AGGRESSIVE
     }
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ -378,16 +378,21 @@ public class BrowserProvider extends Sha
         db.execSQL(sql);
     }
 
     private boolean shouldIncrementVisits(Uri uri) {
         String incrementVisits = uri.getQueryParameter(BrowserContract.PARAM_INCREMENT_VISITS);
         return Boolean.parseBoolean(incrementVisits);
     }
 
+    private boolean shouldIncrementRemoteAggregates(Uri uri) {
+        final String incrementRemoteAggregates = uri.getQueryParameter(BrowserContract.PARAM_INCREMENT_REMOTE_AGGREGATES);
+        return Boolean.parseBoolean(incrementRemoteAggregates);
+    }
+
     @Override
     public String getType(Uri uri) {
         final int match = URI_MATCHER.match(uri);
 
         trace("Getting URI type: " + uri);
 
         switch (match) {
             case BOOKMARKS:
@@ -1418,34 +1423,50 @@ public class BrowserProvider extends Sha
 
         final SQLiteDatabase db = getWritableDatabase(uri);
 
         if (!values.containsKey(History.DATE_MODIFIED)) {
             values.put(History.DATE_MODIFIED, System.currentTimeMillis());
         }
 
         // Use the simple code path for easy updates.
-        if (!shouldIncrementVisits(uri)) {
+        if (!shouldIncrementVisits(uri) && !shouldIncrementRemoteAggregates(uri)) {
             trace("Updating history meta data only");
             return db.update(TABLE_HISTORY, values, selection, selectionArgs);
         }
 
         trace("Updating history meta data and incrementing visits");
 
         if (values.containsKey(History.DATE_LAST_VISITED)) {
             values.put(History.LOCAL_DATE_LAST_VISITED, values.getAsLong(History.DATE_LAST_VISITED));
         }
 
-        // Update data and increment visits by 1.
-        final long incVisits = 1;
-
         // Create a separate set of values that will be updated as an expression.
         final ContentValues visits = new ContentValues();
-        visits.put(History.VISITS, History.VISITS + " + " + incVisits);
-        visits.put(History.LOCAL_VISITS, History.LOCAL_VISITS + " + " + incVisits);
+        if (shouldIncrementVisits(uri)) {
+            // Update data and increment visits by 1.
+            final long incVisits = 1;
+
+            visits.put(History.VISITS, History.VISITS + " + " + incVisits);
+            visits.put(History.LOCAL_VISITS, History.LOCAL_VISITS + " + " + incVisits);
+        }
+
+        if (shouldIncrementRemoteAggregates(uri)) {
+            // Let's fail loudly instead of trying to assume what users of this API meant to do.
+            if (!values.containsKey(History.REMOTE_VISITS)) {
+                throw new IllegalArgumentException(
+                        "Tried incrementing History.REMOTE_VISITS by unknown value");
+            }
+            visits.put(
+                    History.REMOTE_VISITS,
+                    History.REMOTE_VISITS + " + " + values.getAsInteger(History.REMOTE_VISITS)
+            );
+            // Need to remove passed in value, so that we increment REMOTE_VISITS, and not just set it.
+            values.remove(History.REMOTE_VISITS);
+        }
 
         final ContentValues[] valuesAndVisits = { values,  visits };
         final UpdateOperation[] ops = { UpdateOperation.ASSIGN, UpdateOperation.EXPRESSION };
 
         return DBUtils.updateArrays(db, TABLE_HISTORY, valuesAndVisits, ops, selection, selectionArgs);
     }
 
     private long insertVisitForHistory(Uri uri, ContentValues values, String selection, String[] selectionArgs) {
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryDataAccessor.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryDataAccessor.java
@@ -34,28 +34,24 @@ public class AndroidBrowserHistoryDataAc
   protected ContentValues getContentValues(Record record) {
     ContentValues cv = new ContentValues();
     HistoryRecord rec = (HistoryRecord) record;
     cv.put(BrowserContract.History.GUID, rec.guid);
     cv.put(BrowserContract.History.TITLE, rec.title);
     cv.put(BrowserContract.History.URL, rec.histURI);
     if (rec.visits != null) {
       JSONArray visits = rec.visits;
-      long mostRecent = 0;
-      for (int i = 0; i < visits.size(); i++) {
-        JSONObject visit = (JSONObject) visits.get(i);
-        long visitDate = (Long) visit.get(VisitsHelper.SYNC_DATE_KEY);
-        if (visitDate > mostRecent) {
-          mostRecent = visitDate;
-        }
-      }
+      long mostRecent = getLastVisited(visits);
+
       // Fennec stores history timestamps in milliseconds, and visit timestamps in microseconds.
       // The rest of Sync works in microseconds. This is the conversion point for records coming form Sync.
       cv.put(BrowserContract.History.DATE_LAST_VISITED, mostRecent / 1000);
+      cv.put(BrowserContract.History.REMOTE_DATE_LAST_VISITED, mostRecent / 1000);
       cv.put(BrowserContract.History.VISITS, Long.toString(visits.size()));
+      cv.put(BrowserContract.History.REMOTE_VISITS, Long.toString(visits.size()));
     }
     return cv;
   }
 
   @Override
   protected String[] getAllColumns() {
     return BrowserContractHelpers.HistoryColumns;
   }
@@ -135,21 +131,59 @@ public class AndroidBrowserHistoryDataAc
     if (inserted == size) {
       Logger.debug(LOG_TAG, "Inserted " + inserted + " records, as expected.");
     } else {
       Logger.debug(LOG_TAG, "Inserted " +
                    inserted + " records but expected " +
                    size     + " records; continuing to update visits.");
     }
 
+    final ContentValues remoteVisitAggregateValues = new ContentValues();
+    final Uri historyIncrementRemoteAggregateUri = getUri().buildUpon()
+            .appendQueryParameter(BrowserContract.PARAM_INCREMENT_REMOTE_AGGREGATES, "true")
+            .build();
     for (Record record : records) {
       HistoryRecord rec = (HistoryRecord) record;
       if (rec.visits != null && rec.visits.size() != 0) {
-        context.getContentResolver().bulkInsert(
+        int remoteVisitsInserted = context.getContentResolver().bulkInsert(
                 BrowserContract.Visits.CONTENT_URI,
                 VisitsHelper.getVisitsContentValues(rec.guid, rec.visits)
         );
+
+        // If we just inserted any visits, update remote visit aggregate values.
+        // While inserting visits, we might not insert all of rec.visits - if we already have a local
+        // visit record with matching (guid,date), we will skip that visit.
+        // Remote visits aggregate value will be incremented by number of visits inserted.
+        // Note that we don't need to set REMOTE_DATE_LAST_VISITED, because it already gets set above.
+        if (remoteVisitsInserted > 0) {
+          // Note that REMOTE_VISITS must be set before calling cr.update(...) with a URI
+          // that has PARAM_INCREMENT_REMOTE_AGGREGATES=true.
+          remoteVisitAggregateValues.put(BrowserContract.History.REMOTE_VISITS, remoteVisitsInserted);
+          context.getContentResolver().update(
+                  historyIncrementRemoteAggregateUri,
+                  remoteVisitAggregateValues,
+                  BrowserContract.History.GUID + " = ?", new String[] {rec.guid}
+          );
+        }
       }
     }
 
     return inserted;
   }
+
+  /**
+   * Helper method used to find largest <code>VisitsHelper.SYNC_DATE_KEY</code> value in a provided JSONArray.
+   *
+   * @param visits Array of objects which will be searched.
+   * @return largest value of <code>VisitsHelper.SYNC_DATE_KEY</code>.
+     */
+  private long getLastVisited(JSONArray visits) {
+    long mostRecent = 0;
+    for (int i = 0; i < visits.size(); i++) {
+      final JSONObject visit = (JSONObject) visits.get(i);
+      long visitDate = (Long) visit.get(VisitsHelper.SYNC_DATE_KEY);
+      if (visitDate > mostRecent) {
+        mostRecent = visitDate;
+      }
+    }
+    return mostRecent;
+  }
 }