Closed Bug 1329128 Opened 7 years ago Closed 7 years ago

Fennec Bookmark Management: Allow users to create, remove and move folders

Categories

(Firefox for Android Graveyard :: General, defect, P1)

Product:
Firefox for Android Graveyard
Component:
General
Type:
defect

Tracking

(firefox55 fixed)

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jcheng, Assigned: jwu)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1329128 - Part 1: Implement the functionality of remove bookmark folder in BrowserProvider.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1329128 - Part 2: Implement create bookmark folder page.
59 bytes, text/x-review-board-request
Grisha
: review+
ahunt
: review+
Details 
As a user, I'd like to create, remove and move folders to help me manage my bookmarks
Blocks: 1329127
Assignee: nobody → topwu.tw
Priority: -- → P1
Depends on: 1232439
Depends on: 1329131
Comment on attachment 8848974 [details]
Bug 1329128 - Part 1: Implement the functionality of remove bookmark folder in BrowserProvider.

https://reviewboard.mozilla.org/r/121824/#review125612

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1215
(Diff revision 1)
> +        values.put(Bookmarks.PARENT, parentId);
> +        values.put(Bookmarks.TITLE, title);
> +        values.put(Bookmarks.TYPE, Bookmarks.TYPE_FOLDER);
> +        final Uri uri = cr.insert(mBookmarksUriWithProfile, values);
> +
> +        // Bump parent modified time using its ID.

The two operations need to be done atomically, otherwise you will have a weird partially modified state. This is particularly bad in context of sync and "last modified" timestamps, as the way you have this code now, it's possible sync will only pick up some of the changes, and not all.

Can you perhaps move this into BrowserProvider? See how other atomic operations are performed there (via batches, etc).

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1317
(Diff revision 1)
>                    Bookmarks._ID + " = ?",
>                    new String[] { String.valueOf(id) });
>      }
>  
>      @Override
> +    public void removeBookmarksInFolder(ContentResolver cr, long folderId) {

Same comment as above. This needs to be atomic, but isn't at the moment.

This could be literally a single DELETE statement.
Attachment #8848974 - Flags: review?(gkruglov) → review-
Attachment #8848975 - Flags: review?(ahunt)
You have multiple related bugs going through review which all seem to modify some common parts, such as BookmarkCacheHelper. I understand that you have multiple user stories involved, but can you somehow ensure that common changes are unified under one bug, if possible? Bug 1232439 might be a good place, but feel free to re-arrange things as you see fit.

This split also makes it more difficult to test than it should be.
Comment on attachment 8848975 [details]
Bug 1329128 - Part 2: Implement create bookmark folder page.

https://reviewboard.mozilla.org/r/121826/#review135950

Clearing the flag for now. Does this need to be updated on top of changes in other bugs?
Attachment #8848975 - Flags: review?(gkruglov) → review-
Comment on attachment 8848976 [details]
Bug 1329128 - Part 3: Adjust favicon test case since its size has changed to 25dp.

https://reviewboard.mozilla.org/r/121828/#review135952

Generally, adjust tests in the patch that changed the behaviour.
Attachment #8848976 - Flags: review?(gkruglov) → review+
Attachment #8848974 - Attachment is obsolete: true
Attachment #8848975 - Attachment is obsolete: true
Attachment #8848975 - Flags: review?(ahunt)
Attachment #8848976 - Attachment is obsolete: true
I'll send a new patch after Bug 1329131 and Bug 1232439 getting r+.
Comment on attachment 8848975 [details]
Bug 1329128 - Part 2: Implement create bookmark folder page.

https://reviewboard.mozilla.org/r/121826/#review136664

The UI parts look good to me, two nits though!

::: mobile/android/base/java/org/mozilla/gecko/bookmark/CreateFolderFragment.java:115
(Diff revision 1)
> +
> +    @Override
> +    public void onResume() {
> +        super.onResume();
> +
> +        getLoaderManager().initLoader(0, null, new FolderLoaderCallbacks());

nit: I'd feel happier if 0 were defined in LOADER_ID_??? just to ensure it's the same value used in destroyLoader().

::: mobile/android/base/java/org/mozilla/gecko/bookmark/CreateFolderFragment.java:267
(Diff revision 1)
> +            final BrowserDB db = BrowserDB.from(context);
> +            final ContentResolver cr = context.getContentResolver();
> +
> +            Uri uri = db.addBookmarkFolder(cr, mTitle, mParentId);
> +            Cursor cursor = cr.query(uri, new String[] { BrowserContract.Bookmarks._ID }, null, null, null, null);
> +            if (cursor != null && cursor.getCount() > 0) {

We'll leak the cursor if cursor.getCount() == 0. We should probably check the count inside try {} instead to ensure the cursor always gets closed.
Attachment #8848975 - Flags: review+
Comment on attachment 8848974 [details]
Bug 1329128 - Part 1: Implement the functionality of remove bookmark folder in BrowserProvider.

https://reviewboard.mozilla.org/r/121824/#review138186

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2269
(Diff revision 2)
>          values.putNull(Bookmarks.TAGS);
>          values.putNull(Bookmarks.FAVICON_ID);
>  
> +        // When deleting bookmarks/folders, we have to delete their all descendant bookmarks/folders as well.
> +        // We concatenate all IDs into a clause, then delete them can at once.
> +        final List<Long> ids = getBookmarksAndDescendants(db, selection, selectionArgs);

Note the comment just below. Upgrading a transaction from a reader to a writer might result in a failure with SQL_BUSY error.

You might need to split this into two calls. First, perform a regular update (marking as deleted) as the code was doing before. This will ensure that the first operation is a write, and then you can safely query for the descendants and "delete" those as well.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2270
(Diff revision 2)
>          values.putNull(Bookmarks.FAVICON_ID);
>  
> +        // When deleting bookmarks/folders, we have to delete their all descendant bookmarks/folders as well.
> +        // We concatenate all IDs into a clause, then delete them can at once.
> +        final List<Long> ids = getBookmarksAndDescendants(db, selection, selectionArgs);
> +        final String inClause = DBUtils.computeSQLInClauseFromLongs(ids, Bookmarks._ID);

Be mindful of DBUtils.SQLITE_MAX_VARIABLE_NUMBER. This will simply break if you have more than 999 children in a folder.

See https://sqlite.org/limits.html#max_variable_number for explanation.

Also check out how `bulkDeleteByHistoryGUID` deals with this problem by chunking deletes.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2319
(Diff revision 2)
> +        }
> +
> +        if (!folderIds.isEmpty()) {
> +            // If there are folders selected, recursively find their children.
> +            final String inClause = DBUtils.computeSQLInClauseFromLongs(folderIds, Bookmarks.PARENT);
> +            ids.addAll(getBookmarksAndDescendants(db, inClause, null));

You really want to avoid recursion here. See my comment on the other bookmark management review.

Can you rewrite this without relying on recursion?
Attachment #8848974 - Flags: review?(gkruglov) → review-
Comment on attachment 8848975 [details]
Bug 1329128 - Part 2: Implement create bookmark folder page.

https://reviewboard.mozilla.org/r/121826/#review138220

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/CreateFolderFragment.java:162
(Diff revision 2)
> +            return new FolderLoader(getContext());
> +        }
> +
> +        @Override
> +        public void onLoadFinished(Loader<Bundle> loader, final Bundle bundle) {
> +            final long id = bundle.getLong(FolderLoader.ARG_ID);

Your loader, as written, might return an empty Bundle object and these calls will fail.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/CreateFolderFragment.java:169
(Diff revision 2)
> +            final String guid = bundle.getString(FolderLoader.ARG_GUID);
> +
> +            parentId = id;
> +
> +            if (BrowserContract.Bookmarks.MOBILE_FOLDER_GUID.equals(guid)) {
> +                folderText.setText(R.string.bookmarks_folder_none);

I flagged this in another bug, I think, but will also comment here that this deserves another thought. "None" vs "Mobile Bookmarks" specifically.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/CreateFolderFragment.java:181
(Diff revision 2)
> +        public void onLoaderReset(Loader<Bundle> loader) {
> +        }
> +    }
> +
> +    private static class FolderLoader extends AsyncTaskLoader<Bundle> {
> +        private static final String ARG_ID = "id";

You already defined these above in the parent class.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/CreateFolderFragment.java:201
(Diff revision 2)
> +
> +            final Bundle bundle = new Bundle();
> +            final Cursor cursor = db.getBookmarkByGuid(cr, BrowserContract.Bookmarks.MOBILE_FOLDER_GUID);
> +            if (cursor != null) {
> +                try {
> +                    if (cursor.moveToFirst()) {

Missing mobile root folder is probably enough to throw an IllegalStateException and give up.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/CreateFolderFragment.java:280
(Diff revision 2)
> +        public Long doInBackground() {
> +            final Context context = getContext();
> +            final BrowserDB db = BrowserDB.from(context);
> +            final ContentResolver cr = context.getContentResolver();
> +
> +            final Uri uri = db.addBookmarkFolder(cr, title, parentId);

This is the uri of the bookmark you just inserted. It will have the ID, you just need to parse it out of the path. Rest of the code in this method is unnecessary.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/CreateFolderFragment.java:281
(Diff revision 2)
> +            final Context context = getContext();
> +            final BrowserDB db = BrowserDB.from(context);
> +            final ContentResolver cr = context.getContentResolver();
> +
> +            final Uri uri = db.addBookmarkFolder(cr, title, parentId);
> +            if (uri == null) {

I wouldn't bother with this check. This isn't something we can meaningfully recover from.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/CreateFolderFragment.java:304
(Diff revision 2)
> +        }
> +
> +        @Override
> +        public void onPostExecute(Long folderId) {
> +            if (folderId == null) {
> +                Log.w(LOG_TAG, "Create bookmark folder failed.");

Don't just fail silently. Can we add a snackbar at least to notify users of failures?

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/CreateFolderFragment.java:306
(Diff revision 2)
> +        @Override
> +        public void onPostExecute(Long folderId) {
> +            if (folderId == null) {
> +                Log.w(LOG_TAG, "Create bookmark folder failed.");
> +            } else if (callback != null) {
> +                callback.onFolderCreated(folderId, title);

Your callback chain is weird. You might as well just call `onFolderChanged` here directly.

As written, this will also end up calling `dismiss()` twice.

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:72
(Diff revision 2)
> +
> +    public static SelectFolderFragment newInstance() {
> +        final Bundle args = new Bundle();
> +        args.putBoolean(ARG_CREATE_FOLDER, false);
> +
> +        return newInstance(null);

Did you mean to pass in the `args`? Or why bother with that object otherwise?

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/SelectFolderFragment.java:75
(Diff revision 2)
> +        args.putBoolean(ARG_CREATE_FOLDER, false);
> +
> +        return newInstance(null);
> +    }
> +
> +    private static SelectFolderFragment newInstance(Bundle args) {

If you allow nulls for an argument, mark it as @Nullable. Prefer not to do that, passing around nulls sucks and makes life just a bit worse.
Attachment #8848975 - Flags: review?(gkruglov) → review-
Comment on attachment 8848975 [details]
Bug 1329128 - Part 2: Implement create bookmark folder page.

https://reviewboard.mozilla.org/r/121826/#review138220

> I flagged this in another bug, I think, but will also comment here that this deserves another thought. "None" vs "Mobile Bookmarks" specifically.

I'll ask UX to help check this issue and update the spec in Bug 1347095.

> Your callback chain is weird. You might as well just call `onFolderChanged` here directly.
> 
> As written, this will also end up calling `dismiss()` twice.

The flow to create a folder:
1. BookmarkEditFragment launches SelectFolderFragment to show whole folder list,
2. SelectFolderFragment launches CreateFolderFragment to create a new folder,
3. CreateFolderFragment launches 'another' SelectFolderFragment if user want to change parent folder,
2. After creating completed, CreateFolderFragment calls callback `onFolderChanged` implemented by SelectFolderFragment,
3. CreateFolderFragment and SelectFolderFragment call `dismiss()` and back to BookmarkEditFragment.

CreateFolderFragment cannot call onFolderChanged directly since it doesn't implement CreateFolderCallback. And two `dismiss()` are triggered by CreateFolderFragment and SelectFolderFragment separately.
(In reply to :Grisha Kruglov from comment #12)

> Be mindful of DBUtils.SQLITE_MAX_VARIABLE_NUMBER. This will simply break if
> you have more than 999 children in a folder.

If only we could actually call sqlite3_limit. :/
Comment on attachment 8848974 [details]
Bug 1329128 - Part 1: Implement the functionality of remove bookmark folder in BrowserProvider.

https://reviewboard.mozilla.org/r/121824/#review138994

This generally looks good, but I think:
- you might be doing something subtly wrong with the delete query - correct me if I'm wrong though!
- and should be able to do these deletes using significantly less SQL lookup queries!

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2233
(Diff revision 4)
>          return deleted;
>      }
>  
> +    private int bulkDeleteByBookmarkGUIDs(SQLiteDatabase db, List<String> bookmarkGUIDs, String table, String bookmarkGUIDColumn) {
> +        // Due to SQLite's maximum variable limitation, we need to chunk our delete statements.
> +        // For example, if there were 1200 GUIDs, this will perform 2 delete statements.

"... if SQLITE_MAX_VARIABLE_NUMBER is 999."

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2250
(Diff revision 4)
> +        values.putNull(Bookmarks.FAVICON_ID);
> +
> +        // Leave space for variables in values.
> +        final int maxVariableNumber = DBUtils.SQLITE_MAX_VARIABLE_NUMBER - values.size();
> +
> +        for (int chunk = 0; chunk <= bookmarkGUIDs.size() / maxVariableNumber; chunk++) {

Might be worth noting here in a comment that the chunking logic is taken from `bulkDeleteByHistoryGUID`

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2296
(Diff revision 4)
>          updateBookmarkParents(db, parentValues, selection, selectionArgs);
>  
>          final ContentValues values = new ContentValues();
>          values.put(Bookmarks.IS_DELETED, 1);
>          values.put(Bookmarks.POSITION, 0);
> -        values.putNull(Bookmarks.PARENT);
> +        // Don't set columns to null here, or the query in getBookmarkDescendantGUIDs() will fail.

Why would it fail? We're not nulling GUID, TYPE or ID here, which is what you're using further to traverse the tree.

In fact, unless I'm reading your code wrong, you will never end up actually NULLING these values for bookmarks that match `selection`, just their descendants...

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2354
(Diff revision 4)
> +
> +        while (!folderQueue.isEmpty()) {
> +            final long folderId = folderQueue.poll();
> +            final Cursor c = db.query(TABLE_BOOKMARKS,
> +                                      new String[] { Bookmarks._ID, Bookmarks.TYPE, Bookmarks.GUID },
> +                                      Bookmarks.PARENT + " = ?",

You could optimize this quite a bit to save on number of queries you're going to make by building up a list of known folders first, querying by `IF PARENT IN [list of folder IDs]`, then pushing to folderQueue once you get then results, and repeating the process (get all IDs, build a WHERE clause, run it, etc).

As it stands, you will be making a single request for every folder you encounter!

The above optimization will cut down number of SQL queries to the depth of your "to delete" tree, IIUC, which for most users will be _much_ less.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/LocalBrowserDBTest.java:182
(Diff revision 4)
> +        db.removeBookmarkWithId(cr, parentId);
> +        final Cursor cursor = db.getBookmarkById(cr, parentId);
> +        assertNull(cursor);
> +
> +        final int childCountAfterRemove = getRowCount(selection, selectionArgs);
> +        assertEquals(0, childCountAfterRemove);

With those things, it's usually a good idea to write your tests around boundary conditions. That is, test for records<limit, records=limit-1, records=limit+1, records=limit, records>limit, records=2xlimit...

{Insert a smart quote about "off by 1 errors"}
Attachment #8848974 - Flags: review?(gkruglov) → review-
Comment on attachment 8848975 [details]
Bug 1329128 - Part 2: Implement create bookmark folder page.

https://reviewboard.mozilla.org/r/121826/#review139006

::: mobile/android/base/java/org/mozilla/gecko/bookmarks/CreateFolderFragment.java:284
(Diff revision 4)
> +            final Context context = getContext();
> +            final BrowserDB db = BrowserDB.from(context);
> +            final ContentResolver cr = context.getContentResolver();
> +
> +            final Uri uri = db.addBookmarkFolder(cr, title, parentId);
> +            return Long.valueOf(uri.getLastPathSegment());

Yeah, that's much better.
Attachment #8848975 - Flags: review?(gkruglov) → review+
You should use a recursive CTE to do recursive deletion.
(In reply to Richard Newman [:rnewman] from comment #22)
> You should use a recursive CTE to do recursive deletion.

ahunt reminded me that some Android releases ship very old SQLite; 3.8.3 and higher support recursive CTEs. The ideal is to check for the SQLite version at runtime:

  db.rawQuery("select sqlite_version() AS sqlite_version", null)

because lots of vendors ship much newer versions of SQLite than AOSP.

You will naturally need to either fall back to materializing a hierarchy with TRIGGER, or making multiple queries, on earlier versions of SQLite.
Comment on attachment 8848974 [details]
Bug 1329128 - Part 1: Implement the functionality of remove bookmark folder in BrowserProvider.

https://reviewboard.mozilla.org/r/121824/#review138994

> Why would it fail? We're not nulling GUID, TYPE or ID here, which is what you're using further to traverse the tree.
> 
> In fact, unless I'm reading your code wrong, you will never end up actually NULLING these values for bookmarks that match `selection`, just their descendants...

Because `selection` is used both for `updateBookmarks` and `getBookmarkDescendantGUIDs`, modifying column might cause different filter result.

For example:
1. Suppose there are 2 bookmarks have parent ID 1024, and the selection is `Bookmarks.PARENT = 1024`
2. In `updateBookmarks`, we can mark these 2 bookmarks as deleted with no problem
3. But in `getBookmarkDescendantGUIDs`, because the PARENT of these 2 records has been set to null, we cannot find their GUID.

That's why I move `values.putNull()` into `bulkDeleteByBookmarkGUIDs` to make sure the SQL filter result are the same in both methods.


Also `bulkDeleteByBookmarkGUIDs` do reset values for bookmarks that match `selection`, I add a test `BrowserProviderBookmarksTest#testDeleteBookmarkFolder` to guarantee that all records are marked as deleted and theit columns have been reset.
Comment on attachment 8848974 [details]
Bug 1329128 - Part 1: Implement the functionality of remove bookmark folder in BrowserProvider.

https://reviewboard.mozilla.org/r/121824/#review139356

Looks good to me!

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2340
(Diff revision 5)
> +        if (cursor == null) {
> +            return Collections.emptyList();
> +        }
> +
> +        final List<String> guids = new ArrayList<>();
> +        final ArrayDeque<Long> folderQueue = new ArrayDeque<>();

File a follow-up bug to add a faster recursive CTE code path whenever sqlite version supports it.
Attachment #8848974 - Flags: review?(gkruglov) → review+
Keywords: checkin-needed
Blocks: 1363503
Hi Jing-wei, seems there is an open issue not closed, so we can't land this via autolander.
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe3a6247da58
Part 1: Implement the functionality of remove bookmark folder in BrowserProvider. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/ce2315224c0f
Part 2: Implement create bookmark folder page. r=ahunt,Grisha
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fe3a6247da58
https://hg.mozilla.org/mozilla-central/rev/ce2315224c0f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Verified as fixed on build: 56.0a1 from 2017/07/04.
Devices: Huawei MediaPad M2 (Android 5.1.1), LG G4 (Android 6.0).

I was able to create folders, move/change folders and delete them. Tried after a sync was performed and also on a clean profile. So mark this bug as verified.
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: