--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ -7,16 +7,17 @@ package org.mozilla.gecko.db;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import org.mozilla.gecko.AboutPages;
import org.mozilla.gecko.GeckoProfile;
+import org.mozilla.gecko.R;
import org.mozilla.gecko.db.BrowserContract.Bookmarks;
import org.mozilla.gecko.db.BrowserContract.Combined;
import org.mozilla.gecko.db.BrowserContract.FaviconColumns;
import org.mozilla.gecko.db.BrowserContract.Favicons;
import org.mozilla.gecko.db.BrowserContract.History;
import org.mozilla.gecko.db.BrowserContract.Schema;
import org.mozilla.gecko.db.BrowserContract.Tabs;
import org.mozilla.gecko.db.BrowserContract.Thumbnails;
@@ -689,82 +690,79 @@ public class BrowserProvider extends Sha
// tricks to generate these:
// 1. The list of free ids is small, hence we can do a self-join to generate rowids.
// 2. The topsites list is larger, hence we use a temporary table, which automatically provides rowids.
final SQLiteDatabase db = getWritableDatabase(uri);
final String TABLE_TOPSITES = "topsites";
- final String totalLimit = uri.getQueryParameter(BrowserContract.PARAM_LIMIT);
- final String suggestedGridLimit = uri.getQueryParameter(BrowserContract.PARAM_SUGGESTEDSITES_LIMIT);
+ final String limitParam = uri.getQueryParameter(BrowserContract.PARAM_LIMIT);
+ final String gridLimitParam = uri.getQueryParameter(BrowserContract.PARAM_SUGGESTEDSITES_LIMIT);
- final String[] suggestedGridLimitArgs = new String[] {
- suggestedGridLimit
- };
+ final int totalLimit;
+ final int suggestedGridLimit;
- final String[] totalLimitArgs = new String[] {
- totalLimit
- };
+ if (limitParam == null) {
+ totalLimit = 50;
+ } else {
+ totalLimit = Integer.parseInt(limitParam, 10);
+ }
- final String pinnedSitesFromClause = "FROM " + TABLE_BOOKMARKS + " WHERE " + Bookmarks.PARENT + " == ?";
- final String[] pinnedSitesArgs = new String[] {
- String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID)
- };
+ if (gridLimitParam == null) {
+ suggestedGridLimit = getContext().getResources().getInteger(R.integer.number_of_top_sites);
+ } else {
+ suggestedGridLimit = Integer.parseInt(gridLimitParam, 10);
+ }
+
+ final String pinnedSitesFromClause = "FROM " + TABLE_BOOKMARKS + " WHERE " + Bookmarks.PARENT + " == " + Bookmarks.FIXED_PINNED_LIST_ID;
// Ideally we'd use a recursive CTE to generate our sequence, e.g. something like this worked at one point:
// " WITH RECURSIVE" +
// " cnt(x) AS (VALUES(1) UNION ALL SELECT x+1 FROM cnt WHERE x < 6)" +
// However that requires SQLite >= 3.8.3 (available on Android >= 5.0), so in the meantime
// we use a temporary numbers table.
// Note: SQLite rowids are 1-indexed, whereas we're expecting 0-indexed values for the position. Our numbers
// table starts at position = 0, which ensures the correct results here.
final String freeIDSubquery =
" SELECT count(free_ids.position) + 1 AS rowid, numbers.position AS " + Bookmarks.POSITION +
" FROM (SELECT position FROM numbers WHERE position NOT IN (SELECT " + Bookmarks.POSITION + " " + pinnedSitesFromClause + ")) AS numbers" +
" LEFT OUTER JOIN " +
" (SELECT position FROM numbers WHERE position NOT IN (SELECT " + Bookmarks.POSITION + " " + pinnedSitesFromClause + ")) AS free_ids" +
" ON numbers.position > free_ids.position" +
" GROUP BY numbers.position" +
" ORDER BY numbers.position ASC" +
- " LIMIT ?";
-
- final String[] freeIDArgs = DBUtils.concatenateSelectionArgs(
- pinnedSitesArgs,
- pinnedSitesArgs,
- suggestedGridLimitArgs);
+ " LIMIT " + suggestedGridLimit;
// Filter out: unvisited pages (history_id == -1) pinned (and other special) sites, deleted sites,
// and about: pages.
final String ignoreForTopSitesWhereClause =
"(" + Combined.HISTORY_ID + " IS NOT -1)" +
" AND " +
Combined.URL + " NOT IN (SELECT " +
Bookmarks.URL + " FROM bookmarks WHERE " +
- DBUtils.qualifyColumn("bookmarks", Bookmarks.PARENT) + " < ? AND " +
+ DBUtils.qualifyColumn("bookmarks", Bookmarks.PARENT) + " < " + Bookmarks.FIXED_ROOT_ID + " AND " +
DBUtils.qualifyColumn("bookmarks", Bookmarks.IS_DELETED) + " == 0)" +
" AND " +
"(" + Combined.URL + " NOT LIKE ?)";
final String[] ignoreForTopSitesArgs = new String[] {
- String.valueOf(Bookmarks.FIXED_ROOT_ID),
AboutPages.URL_FILTER
};
-
// Stuff the suggested sites into SQL: this allows us to filter pinned and topsites out of the suggested
// sites list as part of the final query (as opposed to walking cursors in java)
final SuggestedSites suggestedSites = GeckoProfile.get(getContext(), uri.getQueryParameter(BrowserContract.PARAM_PROFILE)).getDB().getSuggestedSites();
StringBuilder suggestedSitesBuilder = new StringBuilder();
// We could access the underlying data here, however SuggestedSites also performs filtering on the suggested
// sites list, which means we'd need to process the lists within SuggestedSites in any case. If we're doing
// that processing, there is little real between us using a MatrixCursor, or a Map (or List) instead of the
// MatrixCursor.
- final Cursor suggestedSitesCursor = suggestedSites.get(Integer.parseInt(suggestedGridLimit));
+ final Cursor suggestedSitesCursor = suggestedSites.get(suggestedGridLimit);
String[] suggestedSiteArgs = new String[0];
boolean hasProcessedAnySuggestedSites = true;
final int idColumnIndex = suggestedSitesCursor.getColumnIndexOrThrow(Bookmarks._ID);
final int urlColumnIndex = suggestedSitesCursor.getColumnIndexOrThrow(Bookmarks.URL);
final int titleColumnIndex = suggestedSitesCursor.getColumnIndexOrThrow(Bookmarks.TITLE);
@@ -786,21 +784,18 @@ public class BrowserProvider extends Sha
suggestedSitesCursor.getString(idColumnIndex),
suggestedSitesCursor.getString(urlColumnIndex),
suggestedSitesCursor.getString(titleColumnIndex)
});
}
// To restrict suggested sites to the grid, we simply subtract the number of topsites (which have already had
// the pinned sites filtered out), and the number of pinned sites.
- // SQLite completely ignores negative limits, hence we need to manually totalLimit to 0 in this case.
- final String suggestedLimitClause = " LIMIT MAX(0, (? - (SELECT COUNT(*) FROM " + TABLE_TOPSITES + ") - (SELECT COUNT(*) " + pinnedSitesFromClause + "))) ";
-
- final String[] suggestedLimitArgs = DBUtils.concatenateSelectionArgs(suggestedGridLimitArgs,
- pinnedSitesArgs);
+ // SQLite completely ignores negative limits, hence we need to manually limit to 0 in this case.
+ final String suggestedLimitClause = " LIMIT MAX(0, (" + suggestedGridLimit + " - (SELECT COUNT(*) FROM " + TABLE_TOPSITES + ") - (SELECT COUNT(*) " + pinnedSitesFromClause + "))) ";
db.beginTransaction();
try {
db.execSQL("DROP TABLE IF EXISTS " + TABLE_TOPSITES);
db.execSQL("CREATE TEMP TABLE " + TABLE_TOPSITES + " AS" +
" SELECT " +
Bookmarks._ID + ", " +
@@ -808,20 +803,19 @@ public class BrowserProvider extends Sha
Combined.HISTORY_ID + ", " +
Bookmarks.URL + ", " +
Bookmarks.TITLE + ", " +
Combined.HISTORY_ID + ", " +
TopSites.TYPE_TOP + " AS " + TopSites.TYPE +
" FROM " + Combined.VIEW_NAME +
" WHERE " + ignoreForTopSitesWhereClause +
" ORDER BY " + BrowserContract.getFrecencySortOrder(true, false) +
- " LIMIT ?",
+ " LIMIT " + totalLimit,
- DBUtils.appendSelectionArgs(ignoreForTopSitesArgs,
- totalLimitArgs));
+ ignoreForTopSitesArgs);
if (!hasProcessedAnySuggestedSites) {
db.execSQL("INSERT INTO " + TABLE_TOPSITES +
// We need to LIMIT _after_ selecting the relevant suggested sites, which requires us to
// use an additional internal subquery, since we cannot LIMIT a subquery that is part of UNION ALL.
// Hence the weird SELECT * FROM (SELECT ...relevant suggested sites... LIMIT ?)
" SELECT * FROM (SELECT " +
Bookmarks._ID + ", " +
@@ -833,29 +827,37 @@ public class BrowserProvider extends Sha
TopSites.TYPE_SUGGESTED + " as " + TopSites.TYPE +
" FROM ( " + suggestedSitesBuilder.toString() + " )" +
" WHERE " +
Bookmarks.URL + " NOT IN (SELECT url FROM " + TABLE_TOPSITES + ")" +
" AND " +
Bookmarks.URL + " NOT IN (SELECT url " + pinnedSitesFromClause + ")" +
suggestedLimitClause + " )",
- DBUtils.concatenateSelectionArgs(suggestedSiteArgs,
- pinnedSitesArgs,
- suggestedLimitArgs));
+ suggestedSiteArgs);
}
+ // If we retrieve more topsites than we have free positions for in the freeIdSubquery,
+ // we will have topsites that don't receive a position when joining TABLE_TOPSITES
+ // with freeIdSubquery. Hence we need to coalesce the position with a generated position.
+ // We know that the difference in positions will be at most suggestedGridLimit, hence we
+ // can add that to the rowid to generate a safe position.
+ // I.e. if we have 6 pinned sites then positions 0..5 are filled, the JOIN results in
+ // the first N rows having positions 6..(N+6), so row N+1 should receive a position that is at
+ // least N+1+6, which is equal to rowid + 6.
final SQLiteCursor c = (SQLiteCursor) db.rawQuery(
"SELECT " +
Bookmarks._ID + ", " +
TopSites.BOOKMARK_ID + ", " +
TopSites.HISTORY_ID + ", " +
Bookmarks.URL + ", " +
Bookmarks.TITLE + ", " +
- Bookmarks.POSITION + ", " +
+ "COALESCE(" + Bookmarks.POSITION + ", " +
+ DBUtils.qualifyColumn(TABLE_TOPSITES, "rowid") + " + " + suggestedGridLimit +
+ ")" + " AS " + Bookmarks.POSITION + ", " +
Combined.HISTORY_ID + ", " +
TopSites.TYPE +
" FROM " + TABLE_TOPSITES +
" LEFT OUTER JOIN " + // TABLE_IDS +
"(" + freeIDSubquery + ") AS id_results" +
" ON " + DBUtils.qualifyColumn(TABLE_TOPSITES, "rowid") +
" = " + DBUtils.qualifyColumn("id_results", "rowid") +
@@ -866,22 +868,21 @@ public class BrowserProvider extends Sha
Bookmarks._ID + " AS " + TopSites.BOOKMARK_ID + ", " +
" -1 AS " + TopSites.HISTORY_ID + ", " +
Bookmarks.URL + ", " +
Bookmarks.TITLE + ", " +
Bookmarks.POSITION + ", " +
"NULL AS " + Combined.HISTORY_ID + ", " +
TopSites.TYPE_PINNED + " as " + TopSites.TYPE +
" FROM " + TABLE_BOOKMARKS +
- " WHERE " + Bookmarks.PARENT + " == ? " +
+ " WHERE " + Bookmarks.PARENT + " == " + Bookmarks.FIXED_PINNED_LIST_ID +
" ORDER BY " + Bookmarks.POSITION,
- DBUtils.appendSelectionArgs(freeIDArgs,
- pinnedSitesArgs));
+ null);
c.setNotificationUri(getContext().getContentResolver(),
BrowserContract.AUTHORITY_URI);
// Force the cursor to be compiled and the cursor-window filled now:
// (A) without compiling the cursor now we won't have access to the TEMP table which
// is removed as soon as we close our connection.
// (B) this might also mitigate the situation causing this crash where we're accessing