Bug 718238 - Part 6: don't reupload folders when their children array is already accurate. r=nalexander

mobile/android/base/sync/Utils.java

@@ -48,6 +48,7 @@ import java.util.ArrayList;
 import java.util.Locale;
 import java.util.TreeMap;
 
+import org.json.simple.JSONArray;
 import org.mozilla.apache.commons.codec.binary.Base32;
 import org.mozilla.apache.commons.codec.binary.Base64;
 
@@ -243,4 +244,48 @@ public class Utils {
     bucket.add(value);
     map.put(index, bucket);
   }
+
+  /**
+   * Yes, an equality method that's null-safe.
+   *
+   * @param a
+   * @param b
+   * @return
+   */
+  private static boolean same(Object a, Object b) {
+    if (a == b) {
+      return true;
+    }
+    if (a == null || b == null) {
+      return false;      // If both null, case above applies.
+    }
+    return a.equals(b);
+  }
+
+  /**
+   * Return true if the two arrays are both null, or are both arrays
+   * containing the same elements in the same order.
+   *
+   * @param a
+   * @param b
+   * @return
+   */
+  public static boolean sameArrays(JSONArray a, JSONArray b) {
+    if (a == b) {
+      return true;
+    }
+    if (a == null || b == null) {
+      return false;
+    }
+    final int size = a.size();
+    if (size != b.size()) {
+      return false;
+    }
+    for (int i = 0; i < size; ++i) {
+      if (!same(a.get(i), b.get(i))) {
+        return false;
+      }
+    }
+    return true;
+  }
 }

mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java

@@ -709,23 +709,31 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
         try {
           final long folderID = getIDForGUID(guid);
           JSONArray inDB = getChildrenArray(folderID, false);
-          int added = 0;
-          for (Object o : inDB) {
-            if (!onServer.contains(o)) {
-              onServer.add(o);
-              added++;
+
+          // If the local children and the remote children are already
+          // the same, then we don't need to bump the modified time of the
+          // parent: we wouldn't upload a different record, so avoid the cycle.
+          if (!Utils.sameArrays(onServer, inDB)) {
+            int added = 0;
+            for (Object o : inDB) {
+              if (!onServer.contains(o)) {
+                onServer.add(o);
+                added++;
+              }
             }
+            Logger.debug(LOG_TAG, "Added " + added + " items locally.");
+            dataAccessor.bumpModified(folderID, now());
+            // Wow, this is spectacularly wasteful.
+            Logger.debug(LOG_TAG, "Untracking " + guid);
+            final Record record = retrieveByGUIDDuringStore(guid);
+            if (record == null) {
+              return;
+            }
+            untrackRecord(record);
           }
-          Logger.debug(LOG_TAG, "Added " + added + " items locally.");
+          // Until getChildrenArray can tell us if it needed to make
+          // any changes at all, always update positions.
           dataAccessor.updatePositions(new ArrayList<String>(onServer));
-          dataAccessor.bumpModified(folderID, now());
-          // Wow, this is spectacularly wasteful.
-          Logger.debug(LOG_TAG, "Untracking " + guid);
-          final Record record = retrieveByGUIDDuringStore(guid);
-          if (record == null) {
-            return;
-          }
-          untrackRecord(record);
         } catch (Exception e) {
           Logger.warn(LOG_TAG, "Error repositioning children for " + guid, e);
         }