Bug 1311480 - Add support to query client with order by name alphabetically, r=Grisha
authormaliu <max@mxli.us>
Wed, 16 Nov 2016 14:03:31 +0800
changeset 323672 f3e6d1ac9a5d2a95d159450c0d38889e974f44a2
parent 323671 f4e90ea16802d395467d0b35051a0944027b30ff
child 323673 d6b94b6c5fb083c43cc617159d49f4ea48659c7e
push id34508
push usercbook@mozilla.com
push dateTue, 22 Nov 2016 13:45:55 +0000
treeherderautoland@f3e6d1ac9a5d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGrisha
bugs1311480
milestone53.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1311480 - Add support to query client with order by name alphabetically, r=Grisha MozReview-Commit-ID: 7bN7Vh3TqHU
mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
mobile/android/base/java/org/mozilla/gecko/db/LocalTabsAccessor.java
mobile/android/base/java/org/mozilla/gecko/db/TabsAccessor.java
mobile/android/base/java/org/mozilla/gecko/db/TabsProvider.java
mobile/android/base/java/org/mozilla/gecko/overlays/service/sharemethods/SendTab.java
mobile/android/base/java/org/mozilla/gecko/widget/ActivityChooserModel.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/db/TestTabsProviderRemoteTabs.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
@@ -426,17 +426,17 @@ public class BrowserContract {
         public static final String LAST_USED = "last_used";
 
         // Position of the tab. 0 represents foreground.
         public static final String POSITION = "position";
     }
 
     public static final class Clients implements CommonColumns {
         private Clients() {}
-        public static final Uri CONTENT_RECENCY_URI = Uri.withAppendedPath(TABS_AUTHORITY_URI, "clients_recency");
+        public static final Uri CONTENT_NO_STALE_SORTED_URI = Uri.withAppendedPath(TABS_AUTHORITY_URI, "clients_no_stale_sorted");
         public static final Uri CONTENT_URI = Uri.withAppendedPath(TABS_AUTHORITY_URI, "clients");
         public static final String CONTENT_TYPE = "vnd.android.cursor.dir/client";
         public static final String CONTENT_ITEM_TYPE = "vnd.android.cursor.item/client";
 
         // Client-provided name string. Could conceivably be null.
         public static final String NAME = "name";
 
         // Sync-assigned GUID for client device. NULL for local tabs.
--- a/mobile/android/base/java/org/mozilla/gecko/db/LocalTabsAccessor.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/LocalTabsAccessor.java
@@ -59,32 +59,32 @@ public class LocalTabsAccessor implement
             BrowserContract.Clients.GUID + " DESC, " +
             // Within a single client, most recently used tabs first.
             BrowserContract.Tabs.LAST_USED + " DESC";
 
     private static final String LOCAL_CLIENT_SELECTION = BrowserContract.Clients.GUID + " IS NULL";
 
     private static final Pattern FILTERED_URL_PATTERN = Pattern.compile("^(about|chrome|wyciwyg|file):");
 
-    private final Uri clientsRecencyUriWithProfile;
+    private final Uri clientsNoStaleSortedUriWithProfile;
     private final Uri tabsUriWithProfile;
     private final Uri clientsUriWithProfile;
 
     public LocalTabsAccessor(String profileName) {
         tabsUriWithProfile = DBUtils.appendProfileWithDefault(profileName, BrowserContract.Tabs.CONTENT_URI);
         clientsUriWithProfile = DBUtils.appendProfileWithDefault(profileName, BrowserContract.Clients.CONTENT_URI);
-        clientsRecencyUriWithProfile = DBUtils.appendProfileWithDefault(profileName, BrowserContract.Clients.CONTENT_RECENCY_URI);
+        clientsNoStaleSortedUriWithProfile = DBUtils.appendProfileWithDefault(profileName, BrowserContract.Clients.CONTENT_NO_STALE_SORTED_URI);
     }
 
     /**
      * Extracts a List of just RemoteClients from a cursor.
-     * The supplied cursor should be grouped by guid and sorted by most recently used.
+     * The supplied cursor should be grouped by guid and sorted by name alphabetically.
      */
     @Override
-    public List<RemoteClient> getClientsWithoutTabsByRecencyFromCursor(Cursor cursor) {
+    public List<RemoteClient> getClientsWithoutTabsNoStaleSortedFromCursor(Cursor cursor) {
         final ArrayList<RemoteClient> clients = new ArrayList<>(cursor.getCount());
 
         final int originalPosition = cursor.getPosition();
         try {
             if (!cursor.moveToFirst()) {
                 return clients;
             }
 
@@ -163,18 +163,18 @@ public class LocalTabsAccessor implement
         } finally {
             cursor.moveToPosition(originalPosition);
         }
 
         return clients;
     }
 
     @Override
-    public Cursor getRemoteClientsByRecencyCursor(Context context) {
-        final Uri uri = clientsRecencyUriWithProfile;
+    public Cursor getRemoteClientsNoStaleSorted(Context context) {
+        final Uri uri = clientsNoStaleSortedUriWithProfile;
         return context.getContentResolver().query(uri, CLIENTS_PROJECTION_COLUMNS,
                 REMOTE_CLIENTS_SELECTION, null, null);
     }
 
     @Override
     public Cursor getRemoteTabsCursor(Context context) {
         return getRemoteTabsCursor(context, -1);
     }
--- a/mobile/android/base/java/org/mozilla/gecko/db/TabsAccessor.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/TabsAccessor.java
@@ -12,17 +12,17 @@ import org.mozilla.gecko.Tab;
 
 import java.util.List;
 
 public interface TabsAccessor {
     public interface OnQueryTabsCompleteListener {
         public void onQueryTabsComplete(List<RemoteClient> clients);
     }
 
-    public Cursor getRemoteClientsByRecencyCursor(Context context);
+    public Cursor getRemoteClientsNoStaleSorted(Context context);
     public Cursor getRemoteTabsCursor(Context context);
     public Cursor getRemoteTabsCursor(Context context, int limit);
-    public List<RemoteClient> getClientsWithoutTabsByRecencyFromCursor(final Cursor cursor);
+    public List<RemoteClient> getClientsWithoutTabsNoStaleSortedFromCursor(final Cursor cursor);
     public List<RemoteClient> getClientsFromCursor(final Cursor cursor);
     public void getTabs(final Context context, final OnQueryTabsCompleteListener listener);
     public void getTabs(final Context context, final int limit, final OnQueryTabsCompleteListener listener);
     public void persistLocalTabs(final ContentResolver cr, final Iterable<Tab> tabs);
 }
--- a/mobile/android/base/java/org/mozilla/gecko/db/TabsProvider.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/TabsProvider.java
@@ -27,17 +27,17 @@ public class TabsProvider extends Shared
 
     static final String TABLE_TABS = "tabs";
     static final String TABLE_CLIENTS = "clients";
 
     static final int TABS = 600;
     static final int TABS_ID = 601;
     static final int CLIENTS = 602;
     static final int CLIENTS_ID = 603;
-    static final int CLIENTS_RECENCY = 604;
+    static final int CLIENTS_NO_STALE_SORTED = 604;
 
     // Exclude clients that are more than three weeks old and also any duplicates that are older than one week old.
     static final String EXCLUDE_STALE_CLIENTS_SUBQUERY =
     "(SELECT " + Clients.GUID +
     ", " + Clients.NAME +
     ", " + Clients.LAST_MODIFIED +
     ", " + Clients.DEVICE_TYPE +
     "  FROM " + TABLE_CLIENTS +
@@ -56,33 +56,33 @@ public class TabsProvider extends Shared
         " WHERE (" + Clients.LAST_MODIFIED + " < %1$s" + " AND " + Clients.LAST_MODIFIED + " > %2$s) AND " +
         Clients.NAME + " NOT IN " + "( SELECT " + Clients.NAME + " FROM " + TABLE_CLIENTS + " WHERE " + Clients.LAST_MODIFIED + " > %1$s)" +
         " GROUP BY " + Clients.NAME +
     ") AS c2" +
     " ON c." + Clients.GUID + " = c2." + Clients.GUID + ")";
 
     static final String DEFAULT_TABS_SORT_ORDER = Clients.LAST_MODIFIED + " DESC, " + Tabs.LAST_USED + " DESC";
     static final String DEFAULT_CLIENTS_SORT_ORDER = Clients.LAST_MODIFIED + " DESC";
-    static final String DEFAULT_CLIENTS_RECENCY_SORT_ORDER = "COALESCE(MAX(" + Tabs.LAST_USED + "), " + Clients.LAST_MODIFIED + ") DESC";
+    static final String DEFAULT_CLIENTS_NAME_ORDER = Clients.NAME + " COLLATE NOCASE ASC";
 
     static final String INDEX_TABS_GUID = "tabs_guid_index";
     static final String INDEX_TABS_POSITION = "tabs_position_index";
 
     static final UriMatcher URI_MATCHER = new UriMatcher(UriMatcher.NO_MATCH);
 
     static final Map<String, String> TABS_PROJECTION_MAP;
     static final Map<String, String> CLIENTS_PROJECTION_MAP;
     static final Map<String, String> CLIENTS_RECENCY_PROJECTION_MAP;
 
     static {
         URI_MATCHER.addURI(BrowserContract.TABS_AUTHORITY, "tabs", TABS);
         URI_MATCHER.addURI(BrowserContract.TABS_AUTHORITY, "tabs/#", TABS_ID);
         URI_MATCHER.addURI(BrowserContract.TABS_AUTHORITY, "clients", CLIENTS);
         URI_MATCHER.addURI(BrowserContract.TABS_AUTHORITY, "clients/#", CLIENTS_ID);
-        URI_MATCHER.addURI(BrowserContract.TABS_AUTHORITY, "clients_recency", CLIENTS_RECENCY);
+        URI_MATCHER.addURI(BrowserContract.TABS_AUTHORITY, "clients_no_stale_sorted", CLIENTS_NO_STALE_SORTED);
 
         HashMap<String, String> map;
 
         map = new HashMap<String, String>();
         map.put(Tabs._ID, Tabs._ID);
         map.put(Tabs.TITLE, Tabs.TITLE);
         map.put(Tabs.URL, Tabs.URL);
         map.put(Tabs.HISTORY, Tabs.HISTORY);
@@ -305,20 +305,20 @@ public class TabsProvider extends Shared
                 } else {
                     debug("Using sort order " + sortOrder + ".");
                 }
 
                 qb.setProjectionMap(CLIENTS_PROJECTION_MAP);
                 qb.setTables(TABLE_CLIENTS);
                 break;
 
-            case CLIENTS_RECENCY:
-                trace("Query is on CLIENTS_RECENCY: " + uri);
+            case CLIENTS_NO_STALE_SORTED:
+                trace("Query is on CLIENTS_NO_STALE_SORTED: " + uri);
                 if (TextUtils.isEmpty(sortOrder)) {
-                    sortOrder = DEFAULT_CLIENTS_RECENCY_SORT_ORDER;
+                    sortOrder = DEFAULT_CLIENTS_NAME_ORDER;
                 } else {
                     debug("Using sort order " + sortOrder + ".");
                 }
 
                 final long oneWeekAgo = System.currentTimeMillis() - ONE_WEEK_IN_MILLISECONDS;
                 final long threeWeeksAgo = System.currentTimeMillis() - THREE_WEEKS_IN_MILLISECONDS;
 
                 final String excludeStaleClientsTable = String.format(EXCLUDE_STALE_CLIENTS_SUBQUERY, oneWeekAgo, threeWeeksAgo);
--- a/mobile/android/base/java/org/mozilla/gecko/overlays/service/sharemethods/SendTab.java
+++ b/mobile/android/base/java/org/mozilla/gecko/overlays/service/sharemethods/SendTab.java
@@ -225,32 +225,32 @@ public class SendTab extends ShareMethod
             @Override
             public void executeCommand(final GlobalSession session, List<String> args) {
                 CommandProcessor.displayURI(args, session.getContext());
             }
         });
     }
 
     /**
-     * @return A collection of unique remote clients sorted by most recently used.
+     * @return A collection of unique remote clients sorted by name alphabetically.
      */
     protected Collection<RemoteClient> getOtherClients(final TabSender sender) {
         if (sender == null) {
             Log.w(LOGTAG, "No tab sender when fetching other client IDs.");
             return Collections.emptyList();
         }
 
         final BrowserDB browserDB = BrowserDB.from(context);
         final TabsAccessor tabsAccessor = browserDB.getTabsAccessor();
-        final Cursor remoteTabsCursor = tabsAccessor.getRemoteClientsByRecencyCursor(context);
+        final Cursor remoteTabsCursor = tabsAccessor.getRemoteClientsNoStaleSorted(context);
         try {
             if (remoteTabsCursor.getCount() == 0) {
                 return Collections.emptyList();
             }
-            return tabsAccessor.getClientsWithoutTabsByRecencyFromCursor(remoteTabsCursor);
+            return tabsAccessor.getClientsWithoutTabsNoStaleSortedFromCursor(remoteTabsCursor);
         } finally {
             remoteTabsCursor.close();
         }
     }
 
     /**
      * Inteface for interacting with Sync accounts. Used to hide the difference in implementation
      * between FXA and "old sync" accounts when sending tabs.
--- a/mobile/android/base/java/org/mozilla/gecko/widget/ActivityChooserModel.java
+++ b/mobile/android/base/java/org/mozilla/gecko/widget/ActivityChooserModel.java
@@ -1311,17 +1311,17 @@ public class ActivityChooserModel extend
         // checking if we have accounts set up - if not, we can't have any clients.
         if (!FirefoxAccounts.firefoxAccountsExist(mContext)) {
             return false;
         }
 
         final BrowserDB browserDB = BrowserDB.from(mContext);
         final TabsAccessor tabsAccessor = browserDB.getTabsAccessor();
         final Cursor remoteClientsCursor = tabsAccessor
-                .getRemoteClientsByRecencyCursor(mContext);
+                .getRemoteClientsNoStaleSorted(mContext);
         if (remoteClientsCursor == null) {
             return false;
         }
 
         try {
             return remoteClientsCursor.getCount() > 0;
         } finally {
             remoteClientsCursor.close();
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/db/TestTabsProviderRemoteTabs.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/db/TestTabsProviderRemoteTabs.java
@@ -29,16 +29,20 @@ import org.robolectric.shadows.ShadowCon
 import java.util.List;
 
 @RunWith(TestRunner.class)
 public class TestTabsProviderRemoteTabs {
     private static final long ONE_DAY_IN_MILLISECONDS = 1000 * 60 * 60 * 24;
     private static final long ONE_WEEK_IN_MILLISECONDS = 7 * ONE_DAY_IN_MILLISECONDS;
     private static final long THREE_WEEKS_IN_MILLISECONDS = 3 * ONE_WEEK_IN_MILLISECONDS;
 
+    private static final String CLIENT_LOCAL_NAME = "our local";
+    private static final String CLIENT_REMOTE1_NAME = "The remote1";
+    private static final String CLIENT_REMOTE2_NAME = "another remote2";
+
     protected TabsProvider provider;
 
     @Before
     public void setUp() {
         provider = new TabsProvider();
         provider.onCreate();
         ShadowContentResolver.registerProvider(BrowserContract.TABS_AUTHORITY, new DelegatingTestContentProvider(provider));
     }
@@ -50,17 +54,17 @@ public class TestTabsProviderRemoteTabs 
     }
 
     protected ContentProviderClient getClientsClient() {
         final ShadowContentResolver cr = new ShadowContentResolver();
         return cr.acquireContentProviderClient(BrowserContractHelpers.CLIENTS_CONTENT_URI);
     }
 
     @Test
-    public void testGetClientsWithoutTabsByRecencyFromCursor() throws Exception {
+    public void testGetClientsWithoutTabsNoStaleSortedFromCursor() throws Exception {
         final Uri uri = BrowserContractHelpers.CLIENTS_CONTENT_URI;
         final ContentProviderClient cpc = getClientsClient();
         final LocalTabsAccessor accessor = new LocalTabsAccessor("test"); // The profile name given doesn't matter.
 
         try {
             // Delete all tabs to begin with.
             cpc.delete(uri, null, null);
             Cursor allClients = cpc.query(uri, null, null, null, null);
@@ -69,49 +73,49 @@ public class TestTabsProviderRemoteTabs 
             } finally {
                 allClients.close();
             }
 
             // Insert a local and remote1 client record, neither with tabs.
             final long now = System.currentTimeMillis();
             // Local client has GUID = null.
             final ContentValues local = new ContentValues();
-            local.put(BrowserContract.Clients.NAME, "local");
+            local.put(BrowserContract.Clients.NAME, CLIENT_LOCAL_NAME);
             local.put(BrowserContract.Clients.LAST_MODIFIED, now + 1);
             // Remote clients have GUID != null.
             final ContentValues remote1 = new ContentValues();
             remote1.put(BrowserContract.Clients.GUID, "guid1");
-            remote1.put(BrowserContract.Clients.NAME, "remote1");
+            remote1.put(BrowserContract.Clients.NAME, CLIENT_REMOTE1_NAME);
             remote1.put(BrowserContract.Clients.LAST_MODIFIED, now + 2);
 
             final ContentValues remote2 = new ContentValues();
             remote2.put(BrowserContract.Clients.GUID, "guid2");
-            remote2.put(BrowserContract.Clients.NAME, "remote2");
+            remote2.put(BrowserContract.Clients.NAME, CLIENT_REMOTE2_NAME);
             remote2.put(BrowserContract.Clients.LAST_MODIFIED, now + 3);
 
             ContentValues[] values = new ContentValues[]{local, remote1, remote2};
             int inserted = cpc.bulkInsert(uri, values);
             Assert.assertEquals(3, inserted);
 
-            allClients = cpc.query(BrowserContract.Clients.CONTENT_RECENCY_URI, null, null, null, null);
+            allClients = cpc.query(BrowserContract.Clients.CONTENT_NO_STALE_SORTED_URI, null, null, null, null);
             try {
                 CursorDumper.dumpCursor(allClients);
                 // The local client is not ignored.
                 Assert.assertEquals(3, allClients.getCount());
-                final List<RemoteClient> clients = accessor.getClientsWithoutTabsByRecencyFromCursor(allClients);
+                final List<RemoteClient> clients = accessor.getClientsWithoutTabsNoStaleSortedFromCursor(allClients);
                 Assert.assertEquals(3, clients.size());
                 for (RemoteClient client : clients) {
                     // Each client should not have any tabs.
                     Assert.assertNotNull(client.tabs);
                     Assert.assertEquals(0, client.tabs.size());
                 }
-                // Since there are no tabs, the order should be based on last_modified.
-                Assert.assertEquals("guid2", clients.get(0).guid);
-                Assert.assertEquals("guid1", clients.get(1).guid);
-                Assert.assertEquals(null, clients.get(2).guid);
+                // Client should sorted by name alphabetically.
+                Assert.assertEquals(CLIENT_REMOTE2_NAME, clients.get(0).name);
+                Assert.assertEquals(CLIENT_LOCAL_NAME, clients.get(1).name);
+                Assert.assertEquals(CLIENT_REMOTE1_NAME, clients.get(2).name);
             } finally {
                 allClients.close();
             }
 
             // Now let's add a few tabs to one client.  The times are chosen so that one tab's
             // last used is not relevant, and the other tab is the most recent used.
             final ContentValues remoteTab1 = new ContentValues();
             remoteTab1.put(BrowserContract.Tabs.CLIENT_GUID, "guid1");
@@ -128,33 +132,32 @@ public class TestTabsProviderRemoteTabs 
             remoteTab2.put(BrowserContract.Tabs.HISTORY, "[\"http://test.com/test2\"]");
             remoteTab2.put(BrowserContract.Tabs.LAST_USED, now + 5);
             remoteTab2.put(BrowserContract.Tabs.POSITION, 1);
 
             values = new ContentValues[]{remoteTab1, remoteTab2};
             inserted = cpc.bulkInsert(BrowserContract.Tabs.CONTENT_URI, values);
             Assert.assertEquals(2, inserted);
 
-            allClients = cpc.query(BrowserContract.Clients.CONTENT_RECENCY_URI, null, BrowserContract.Clients.GUID + " IS NOT NULL", null, null);
+            allClients = cpc.query(BrowserContract.Clients.CONTENT_NO_STALE_SORTED_URI, null, BrowserContract.Clients.GUID + " IS NOT NULL", null, null);
             try {
                 CursorDumper.dumpCursor(allClients);
                 // The local client is ignored.
                 Assert.assertEquals(2, allClients.getCount());
-                final List<RemoteClient> clients = accessor.getClientsWithoutTabsByRecencyFromCursor(allClients);
+                final List<RemoteClient> clients = accessor.getClientsWithoutTabsNoStaleSortedFromCursor(allClients);
                 Assert.assertEquals(2, clients.size());
                 for (RemoteClient client : clients) {
                     // Each client should be remote and should not have any tabs.
                     Assert.assertNotNull(client.guid);
                     Assert.assertNotNull(client.tabs);
                     Assert.assertEquals(0, client.tabs.size());
                 }
-                // Since now there is a tab attached to the remote2 client more recent than the
-                // remote1 client modified time, it should be first.
-                Assert.assertEquals("guid1", clients.get(0).guid);
-                Assert.assertEquals("guid2", clients.get(1).guid);
+                // Client should sorted by name alphabetically.
+                Assert.assertEquals(CLIENT_REMOTE2_NAME, clients.get(0).name);
+                Assert.assertEquals(CLIENT_REMOTE1_NAME, clients.get(1).name);
             } finally {
                 allClients.close();
             }
         } finally {
             cpc.release();
         }
     }
 
@@ -211,27 +214,27 @@ public class TestTabsProviderRemoteTabs 
             remote5.put(BrowserContract.Clients.NAME, "remote3");
             remote5.put(BrowserContract.Clients.LAST_MODIFIED, now - ONE_WEEK_IN_MILLISECONDS);
 
             ContentValues[] values = new ContentValues[]{local, remote1, remote2, remote3, remote4, remote5};
             int inserted = cpc.bulkInsert(uri, values);
             Assert.assertEquals(values.length, inserted);
 
             final Cursor remoteClients =
-                    accessor.getRemoteClientsByRecencyCursor(context);
+                    accessor.getRemoteClientsNoStaleSorted(context);
 
             try {
                 CursorDumper.dumpCursor(remoteClients);
                 // Local client is not included.
                 // (remote1, guid1), (remote2, guid2), (remote3, guid3) are expected.
                 Assert.assertEquals(3, remoteClients.getCount());
 
                 // Check the inner data, according to recency.
                 List<RemoteClient> recentRemoteClientsList =
-                        accessor.getClientsWithoutTabsByRecencyFromCursor(remoteClients);
+                        accessor.getClientsWithoutTabsNoStaleSortedFromCursor(remoteClients);
                 Assert.assertEquals(3, recentRemoteClientsList.size());
                 Assert.assertEquals("remote1", recentRemoteClientsList.get(0).name);
                 Assert.assertEquals("guid1", recentRemoteClientsList.get(0).guid);
                 Assert.assertEquals("remote2", recentRemoteClientsList.get(1).name);
                 Assert.assertEquals("guid2", recentRemoteClientsList.get(1).guid);
                 Assert.assertEquals("remote3", recentRemoteClientsList.get(2).name);
                 Assert.assertEquals("guid3", recentRemoteClientsList.get(2).guid);
             } finally {