Bug 1286794 - Allow deleting of partner bookmarks by filtering content provider cursor. r=grisha, a=sledru
☠☠ backed out by fbf0cb3a7651 ☠ ☠
authorSebastian Kaspari <s.kaspari@gmail.com>
Thu, 14 Jul 2016 13:47:56 +0200
changeset 340091 dd907ccf026d3b5d7fa4f40b0413f1c870a05bb3
parent 340090 e68714c0861aa70d091066e9dc358dce651d4a62
child 340092 c8afbaa70f16f9448269e35f8e1f92286fdca9e6
push id6249
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 13:59:36 +0000
treeherdermozilla-beta@bad9d4f5bf7e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgrisha, sledru
bugs1286794
milestone49.0a2
Bug 1286794 - Allow deleting of partner bookmarks by filtering content provider cursor. r=grisha, a=sledru MozReview-Commit-ID: 1R6lWSXObhU
mobile/android/base/java/org/mozilla/gecko/distribution/PartnerBookmarksProviderProxy.java
mobile/android/base/java/org/mozilla/gecko/home/HomeContextMenuInfo.java
mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java
--- a/mobile/android/base/java/org/mozilla/gecko/distribution/PartnerBookmarksProviderProxy.java
+++ b/mobile/android/base/java/org/mozilla/gecko/distribution/PartnerBookmarksProviderProxy.java
@@ -5,23 +5,30 @@
 
 package org.mozilla.gecko.distribution;
 
 import android.content.ContentProvider;
 import android.content.ContentResolver;
 import android.content.ContentUris;
 import android.content.ContentValues;
 import android.content.Context;
+import android.content.SharedPreferences;
 import android.content.UriMatcher;
 import android.database.Cursor;
+import android.database.CursorWrapper;
 import android.net.Uri;
 import android.support.annotation.NonNull;
+import android.text.TextUtils;
 
+import org.mozilla.gecko.GeckoSharedPrefs;
 import org.mozilla.gecko.db.BrowserContract;
 
+import java.util.HashSet;
+import java.util.Set;
+
 /**
  * A proxy for the partner bookmarks provider. Bookmark and folder ids of the partner bookmarks providers
  * will be transformed so that they do not overlap with the ids from the local database.
  *
  * Bookmarks in folder:
  *   content://{PACKAGE_ID}.partnerbookmarks/bookmarks/{folderId}
  * Icon of bookmark:
  *   content://{PACKAGE_ID}.partnerbookmarks/icons/{bookmarkId}
@@ -47,16 +54,69 @@ public class PartnerBookmarksProviderPro
         public static final String TOUCHICON = "touchicon";
         public static final String PARENT = "parent";
     }
 
     private static final String AUTHORITY_PREFIX = ".partnerbookmarks";
 
     private static final int URI_MATCH_BOOKMARKS = 1000;
     private static final int URI_MATCH_ICON = 1001;
+    private static final int URI_MATCH_BOOKMARK = 1002;
+
+    private static final String PREF_DELETED_PARTNER_BOOKMARKS = "distribution.partner.bookmark.deleted";
+
+    /**
+     * Cursor wrapper for filtering empty folders.
+     */
+    private static class FilteredCursor extends CursorWrapper {
+        private HashSet<Integer> emptyFolderPositions;
+        private int count;
+
+        public FilteredCursor(PartnerBookmarksProviderProxy proxy, Cursor cursor) {
+            super(cursor);
+
+            emptyFolderPositions = new HashSet<>();
+            count = cursor.getCount();
+
+            for (int i = 0; i < cursor.getCount(); i++) {
+                cursor.moveToPosition(i);
+
+                final long id = cursor.getLong(cursor.getColumnIndexOrThrow(BrowserContract.Bookmarks._ID));
+                final int type = cursor.getInt(cursor.getColumnIndexOrThrow(BrowserContract.Bookmarks.TYPE));
+
+                if (type == BrowserContract.Bookmarks.TYPE_FOLDER && proxy.isFolderEmpty(id)) {
+                    // We do not support deleting folders. So at least hide partner folders that are
+                    // empty because all bookmarks inside it are deleted/hidden.
+                    // Note that this will still show folders with empty folders in them. But multi-level
+                    // partner bookmarks are very unlikely.
+
+                    count--;
+                    emptyFolderPositions.add(i);
+                }
+            }
+        }
+
+        @Override
+        public int getCount() {
+            return count;
+        }
+
+        @Override
+        public boolean moveToPosition(int position) {
+            final Cursor cursor = getWrappedCursor();
+            final int actualCount = cursor.getCount();
+
+            // Find the next position pointing to a bookmark or a non-empty folder
+            while (position < actualCount && emptyFolderPositions.contains(position)) {
+                position++;
+            }
+
+            return position < actualCount && cursor.moveToPosition(position);
+        }
+    }
 
     private static String getAuthority(Context context) {
         return context.getPackageName() + AUTHORITY_PREFIX;
     }
 
     public static Uri getUriForBookmarks(Context context, long folderId) {
         return new Uri.Builder()
                 .scheme("content")
@@ -70,85 +130,160 @@ public class PartnerBookmarksProviderPro
         return new Uri.Builder()
                 .scheme("content")
                 .authority(getAuthority(context))
                 .appendPath("icons")
                 .appendPath(String.valueOf(bookmarkId))
                 .build();
     }
 
+    public static Uri getUriForBookmark(Context context, long bookmarkId) {
+        return new Uri.Builder()
+                .scheme("content")
+                .authority(getAuthority(context))
+                .appendPath("bookmark")
+                .appendPath(String.valueOf(bookmarkId))
+                .build();
+    }
+
     private final UriMatcher uriMatcher = new UriMatcher(UriMatcher.NO_MATCH);
 
     @Override
     public boolean onCreate() {
         String authority = getAuthority(assertAndGetContext());
 
         uriMatcher.addURI(authority, "bookmarks/*", URI_MATCH_BOOKMARKS);
         uriMatcher.addURI(authority, "icons/*", URI_MATCH_ICON);
+        uriMatcher.addURI(authority, "bookmark/*", URI_MATCH_BOOKMARK);
 
         return true;
     }
 
     @Override
     public Cursor query(@NonNull Uri uri, String[] projection, String selection, String[] selectionArgs, String sortOrder) {
+        final Context context = assertAndGetContext();
         final int match = uriMatcher.match(uri);
 
-        final ContentResolver contentResolver = assertAndGetContext().getContentResolver();
+        final ContentResolver contentResolver = context.getContentResolver();
 
         switch (match) {
             case URI_MATCH_BOOKMARKS:
                 final long bookmarkId = ContentUris.parseId(uri);
                 if (bookmarkId == -1) {
                     throw new IllegalArgumentException("Bookmark id is not a number");
                 }
-                return getBookmarksInFolder(contentResolver, bookmarkId);
+                final Cursor cursor = getBookmarksInFolder(contentResolver, bookmarkId);
+                cursor.setNotificationUri(context.getContentResolver(), uri);
+                return new FilteredCursor(this, cursor);
 
             case URI_MATCH_ICON:
                 return getIcon(contentResolver, ContentUris.parseId(uri));
 
             default:
                 throw new UnsupportedOperationException("Unknown URI " + uri.toString());
         }
-    };
+    }
+
+    @Override
+    public int delete(@NonNull Uri uri, String selection, String[] selectionArgs) {
+        final int match = uriMatcher.match(uri);
+
+        switch (match) {
+            case URI_MATCH_BOOKMARK:
+                rememberRemovedBookmark(ContentUris.parseId(uri));
+                notifyBookmarkChange();
+                return 1;
+
+            default:
+                throw new UnsupportedOperationException("Unknown URI " + uri.toString());
+        }
+    }
+
+    private void notifyBookmarkChange() {
+        final Context context = assertAndGetContext();
+
+        context.getContentResolver().notifyChange(
+                new Uri.Builder()
+                        .scheme("content")
+                        .authority(getAuthority(context))
+                        .appendPath("bookmarks")
+                        .build(),
+                null);
+    }
+
+    private synchronized void rememberRemovedBookmark(long bookmarkId) {
+        Set<String> deletedIds = getRemovedBookmarkIds();
+
+        deletedIds.add(String.valueOf(bookmarkId));
+
+        GeckoSharedPrefs.forProfile(assertAndGetContext())
+                .edit()
+                .putStringSet(PREF_DELETED_PARTNER_BOOKMARKS, deletedIds)
+                .apply();
+    }
+
+    private synchronized Set<String> getRemovedBookmarkIds() {
+        SharedPreferences preferences = GeckoSharedPrefs.forProfile(assertAndGetContext());
+        return preferences.getStringSet(PREF_DELETED_PARTNER_BOOKMARKS, new HashSet<String>());
+    }
 
     private Cursor getBookmarksInFolder(ContentResolver contentResolver, long folderId) {
         // Use root folder id or transform negative id into actual (positive) folder id.
         final long actualFolderId = folderId == BrowserContract.Bookmarks.FIXED_ROOT_ID
                 ? PartnerContract.PARENT_ROOT_ID
                 : BrowserContract.Bookmarks.FAKE_PARTNER_BOOKMARKS_START - folderId;
 
+        final String removedBookmarkIds = TextUtils.join(",", getRemovedBookmarkIds());
+
         return contentResolver.query(
                 PartnerContract.CONTENT_URI,
                 new String[] {
                         // Transform ids into negative values starting with FAKE_PARTNER_BOOKMARKS_START.
                         "(" + BrowserContract.Bookmarks.FAKE_PARTNER_BOOKMARKS_START + " - " + PartnerContract.ID + ") as " + BrowserContract.Bookmarks._ID,
                         "(" + BrowserContract.Bookmarks.FAKE_PARTNER_BOOKMARKS_START + " - " + PartnerContract.ID + ") as " + BrowserContract.Combined.BOOKMARK_ID,
                         PartnerContract.TITLE + " as " + BrowserContract.Bookmarks.TITLE,
                         PartnerContract.URL +  " as " + BrowserContract.Bookmarks.URL,
                         // Transform parent ids to negative ids as well
                         "(" + BrowserContract.Bookmarks.FAKE_PARTNER_BOOKMARKS_START + " - " + PartnerContract.PARENT + ") as " + BrowserContract.Bookmarks.PARENT,
                         // Convert types (we use 0-1 and the partner provider 1-2)
                         "(2 - " + PartnerContract.TYPE + ") as " + BrowserContract.Bookmarks.TYPE,
                         // Use the ID of the entry as GUID
                         PartnerContract.ID + " as " + BrowserContract.Bookmarks.GUID
                 },
                 PartnerContract.PARENT + " = ?"
-                        // Only select entries with valid type
-                        + " AND (" + BrowserContract.Bookmarks.TYPE + " = ? OR " + BrowserContract.Bookmarks.TYPE + " = ?)"
+                        // We only want to read bookmarks or folders from the content provider
+                        + " AND " + BrowserContract.Bookmarks.TYPE + " IN (?,?)"
                         // Only select entries with non empty title
-                        + " AND " + BrowserContract.Bookmarks.TITLE + " <> ''",
+                        + " AND " + BrowserContract.Bookmarks.TITLE + " <> ''"
+                        // Filter all "deleted" ids
+                        + " AND " + BrowserContract.Combined.BOOKMARK_ID + " NOT IN (" + removedBookmarkIds + ")",
                 new String[] {
                         String.valueOf(actualFolderId),
                         String.valueOf(PartnerContract.TYPE_BOOKMARK),
                         String.valueOf(PartnerContract.TYPE_FOLDER)
                 },
                 // Same order we use in our content provider (without position)
                 BrowserContract.Bookmarks.TYPE + " ASC, " + BrowserContract.Bookmarks._ID + " ASC");
     }
 
+    private boolean isFolderEmpty(long folderId) {
+        final Context context = assertAndGetContext();
+        final Cursor cursor = getBookmarksInFolder(context.getContentResolver(), folderId);
+
+        if (cursor == null) {
+            return true;
+        }
+
+        try {
+            return cursor.getCount() == 0;
+        } finally {
+            cursor.close();
+        }
+    }
+
     private Cursor getIcon(ContentResolver contentResolver, long bookmarkId) {
         final long actualId = BrowserContract.Bookmarks.FAKE_PARTNER_BOOKMARKS_START - bookmarkId;
 
         return contentResolver.query(
                 PartnerContract.CONTENT_URI,
                 new String[] {
                     PartnerContract.TOUCHICON,
                     PartnerContract.FAVICON
@@ -176,17 +311,12 @@ public class PartnerBookmarksProviderPro
     }
 
     @Override
     public Uri insert(@NonNull Uri uri, ContentValues values) {
         throw new UnsupportedOperationException();
     }
 
     @Override
-    public int delete(@NonNull Uri uri, String selection, String[] selectionArgs) {
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
     public int update(@NonNull Uri uri, ContentValues values, String selection, String[] selectionArgs) {
         throw new UnsupportedOperationException();
     }
 }
--- a/mobile/android/base/java/org/mozilla/gecko/home/HomeContextMenuInfo.java
+++ b/mobile/android/base/java/org/mozilla/gecko/home/HomeContextMenuInfo.java
@@ -1,15 +1,16 @@
 /* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.home;
 
+import org.mozilla.gecko.db.BrowserContract;
 import org.mozilla.gecko.util.StringUtils;
 
 import android.database.Cursor;
 import android.text.TextUtils;
 import android.view.View;
 import android.widget.AdapterView.AdapterContextMenuInfo;
 import android.widget.ExpandableListAdapter;
 import android.widget.ListAdapter;
@@ -38,18 +39,22 @@ public class HomeContextMenuInfo extends
     public boolean hasBookmarkId() {
         return bookmarkId > -1;
     }
 
     public boolean hasHistoryId() {
         return historyId > -1;
     }
 
+    public boolean hasPartnerBookmarkId() {
+        return bookmarkId <= BrowserContract.Bookmarks.FAKE_PARTNER_BOOKMARKS_START;
+    }
+
     public boolean canRemove() {
-        return hasBookmarkId() || hasHistoryId();
+        return hasBookmarkId() || hasHistoryId() || hasPartnerBookmarkId();
     }
 
     public String getDisplayTitle() {
         if (!TextUtils.isEmpty(title)) {
             return title;
         }
         return StringUtils.stripCommonSubdomains(StringUtils.stripScheme(url, StringUtils.UrlFlags.STRIP_HTTPS));
     }
--- a/mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java
+++ b/mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java
@@ -11,18 +11,20 @@ import org.mozilla.gecko.EditBookmarkDia
 import org.mozilla.gecko.GeckoAppShell;
 import org.mozilla.gecko.GeckoApplication;
 import org.mozilla.gecko.GeckoProfile;
 import org.mozilla.gecko.IntentHelper;
 import org.mozilla.gecko.R;
 import org.mozilla.gecko.SnackbarHelper;
 import org.mozilla.gecko.Telemetry;
 import org.mozilla.gecko.TelemetryContract;
+import org.mozilla.gecko.db.BrowserContract;
 import org.mozilla.gecko.db.BrowserDB;
 import org.mozilla.gecko.db.BrowserContract.SuggestedSites;
+import org.mozilla.gecko.distribution.PartnerBookmarksProviderProxy;
 import org.mozilla.gecko.home.HomeContextMenuInfo.RemoveItemType;
 import org.mozilla.gecko.home.HomePager.OnUrlOpenInBackgroundListener;
 import org.mozilla.gecko.home.HomePager.OnUrlOpenListener;
 import org.mozilla.gecko.home.TopSitesGridView.TopSitesGridContextMenuInfo;
 import org.mozilla.gecko.reader.SavedReaderViewHelper;
 import org.mozilla.gecko.reader.ReadingListHelper;
 import org.mozilla.gecko.restrictions.Restrictable;
 import org.mozilla.gecko.restrictions.Restrictions;
@@ -289,17 +291,21 @@ public abstract class HomeFragment exten
             new EditBookmarkDialog(context).show(info.url);
             return true;
         }
 
         if (itemId == R.id.home_remove) {
             // For Top Sites grid items, position is required in case item is Pinned.
             final int position = info instanceof TopSitesGridContextMenuInfo ? info.position : -1;
 
-            (new RemoveItemByUrlTask(context, info.url, info.itemType, position)).execute();
+            if (info.hasPartnerBookmarkId()) {
+                new RemovePartnerBookmarkTask(context, info.bookmarkId).execute();
+            } else {
+                new RemoveItemByUrlTask(context, info.url, info.itemType, position).execute();
+            }
             return true;
         }
 
         return false;
     }
 
     @Override
     public void setUserVisibleHint (boolean isVisibleToUser) {
@@ -436,9 +442,40 @@ public abstract class HomeFragment exten
 
         @Override
         public void onPostExecute(Void result) {
             SnackbarHelper.showSnackbar((Activity) mContext,
                     mContext.getString(R.string.page_removed),
                     Snackbar.LENGTH_LONG);
         }
     }
+
+    private static class RemovePartnerBookmarkTask extends UIAsyncTask.WithoutParams<Void> {
+        private Context context;
+        private long bookmarkId;
+
+        public RemovePartnerBookmarkTask(Context context, long bookmarkId) {
+            super(ThreadUtils.getBackgroundHandler());
+
+            this.context = context;
+            this.bookmarkId = bookmarkId;
+        }
+
+        @Override
+        protected Void doInBackground() {
+            context.getContentResolver().delete(
+                    PartnerBookmarksProviderProxy.getUriForBookmark(context, bookmarkId),
+                    null,
+                    null
+            );
+
+            return null;
+        }
+
+        @Override
+        protected void onPostExecute(Void aVoid) {
+            SnackbarBuilder.builder((Activity) context)
+                    .message(R.string.page_removed)
+                    .duration(Snackbar.LENGTH_LONG)
+                    .buildAndShow();
+        }
+    }
 }