Replace SpecialFoldersCursorWrapper with a MatrixCursor + MergeCursor

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ahunt, Assigned: ahunt)

Tracking

Trunk
Firefox 47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
LocalBrowserDB.getBookmarksInFolder uses SpecialFoldersCursorWrapper to add the "Desktop Bookmarks" artificial folder when retrieving bookmarks for the mobile bookmarks folder (i.e. the default view).

We could avoid using SpecialFoldersCursorWrapper by using a a MatrixCursor to hold the "Desktop Bookmarks" row, followed by using a MergeCursor to combine that with the remaining mobile bookmarks.

This will also make adding further smartfolders simpler (useful for Bug 1234315 and possibly Bug 1243558) - we just need to add additional rows to the MatrixCursor as needed in getBookmarksInFolder.

Updated

3 years ago
Group: firefox-core-security
(Assignee)

Comment 1

3 years ago
(I almost have a patch for this, hence assigning to myself.)
Assignee: nobody → ahunt
(Assignee)

Comment 2

3 years ago
Created attachment 8724967 [details]
MozReview Request: Bug 1252264 - Use MatrixCursor + MergeCursor to show the desktop bookmarks folder r=mcomella

Review commit: https://reviewboard.mozilla.org/r/37239/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37239/
(Assignee)

Comment 3

3 years ago
Created attachment 8724968 [details]
MozReview Request: Bug 1252264 - Post: Remove the now-unused SpecialFoldersCursorWrapper r=me

This is the last java based CursorWrapper!

Review commit: https://reviewboard.mozilla.org/r/37241/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37241/
(Assignee)

Updated

3 years ago
Attachment #8724967 - Flags: review?(michael.l.comella)
(Assignee)

Comment 4

3 years ago
Comment on attachment 8724967 [details]
MozReview Request: Bug 1252264 - Use MatrixCursor + MergeCursor to show the desktop bookmarks folder r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37239/diff/1-2/
(Assignee)

Comment 5

3 years ago
Comment on attachment 8724968 [details]
MozReview Request: Bug 1252264 - Post: Remove the now-unused SpecialFoldersCursorWrapper r=me

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37241/diff/1-2/
Comment on attachment 8724967 [details]
MozReview Request: Bug 1252264 - Use MatrixCursor + MergeCursor to show the desktop bookmarks folder r=mcomella

https://reviewboard.mozilla.org/r/37239/#review33865

Cool cleanup! I knew CursorWrapper was janky but I didn't have a good solution like this! :)

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:835
(Diff revision 2)
> +            // We need to insert values in a specific order - in order to protect against changes
> +            // in DEFAULT_BOOKMARK_COLUMNS we can just assert that we're using the correct ordering.
> +            // Alternatively we could use RowBuilder.add(columnName, value) but that needs api >= 19,
> +            // or we could iterate over DEFAULT_BOOKMARK_COLUMNS, but that gets messy once we need
> +            // to add more than one artificial folder.
> +            if (!((DEFAULT_BOOKMARK_COLUMNS[0].equals(Bookmarks._ID)) &&
> +                    (DEFAULT_BOOKMARK_COLUMNS[1].equals(Bookmarks.GUID)) &&
> +                    (DEFAULT_BOOKMARK_COLUMNS[2].equals(Bookmarks.URL)) &&
> +                    (DEFAULT_BOOKMARK_COLUMNS[3].equals(Bookmarks.TITLE)) &&
> +                    (DEFAULT_BOOKMARK_COLUMNS[4].equals(Bookmarks.TYPE)) &&
> +                    (DEFAULT_BOOKMARK_COLUMNS[5].equals(Bookmarks.PARENT)) &&
> +                    (DEFAULT_BOOKMARK_COLUMNS.length == 6))) {
> +                // If DEFAULT_BOOKMARK_COLUMNS changes we need to update all the MatrixCursor rows
> +                // to contain appropriate data.
> +                throw new IllegalStateException("Fake folder MatrixCursor creation code must be updated to match DEFAULT_BOOKMARK_COLUMNS");
> +            }

Can we put this in a helper method like `assertDefaultBookmarkColumnOrdering`?
Attachment #8724967 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8724968 [details]
MozReview Request: Bug 1252264 - Post: Remove the now-unused SpecialFoldersCursorWrapper r=me

https://reviewboard.mozilla.org/r/37241/#review33869
(Assignee)

Comment 8

3 years ago
Comment on attachment 8724967 [details]
MozReview Request: Bug 1252264 - Use MatrixCursor + MergeCursor to show the desktop bookmarks folder r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37239/diff/2-3/
Attachment #8724967 - Attachment description: MozReview Request: Bug 1252264 - Use MatrixCursor + MergeCursor to show the desktop bookmarks folder → MozReview Request: Bug 1252264 - Use MatrixCursor + MergeCursor to show the desktop bookmarks folder r=mcomella
(Assignee)

Comment 9

3 years ago
Comment on attachment 8724968 [details]
MozReview Request: Bug 1252264 - Post: Remove the now-unused SpecialFoldersCursorWrapper r=me

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37241/diff/2-3/
(Assignee)

Comment 10

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/5620bb00b64aa5a53eeeb0447979f51d2ebe09a5
Bug 1252264 - Use MatrixCursor + MergeCursor to show the desktop bookmarks folder r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/03d260b11ec9a9a7b75c4a1a721631fddbfe712f
Bug 1252264 - Post: Remove the now-unused SpecialFoldersCursorWrapper r=me

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5620bb00b64a
https://hg.mozilla.org/mozilla-central/rev/03d260b11ec9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.