Bug 1311480 - Add support to query client with order by name alphabetically, r=grisha draft
authormaliu <max@mxli.us>
Wed, 16 Nov 2016 14:03:31 +0800
changeset 442317 38278bd19971ead91995196ff7ba8b78cbc50a7f
parent 434549 673b5327afe1d489c41f683c79c6e8ad22040526
child 537777 8a002235d29756c74c55c868416c542547909791
push id36678
push userbmo:max@mxli.us
push dateTue, 22 Nov 2016 10:18:06 +0000
reviewersgrisha
bugs1311480
milestone52.0a1
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 {