1252264 - Replace SpecialFoldersCursorWrapper with a MatrixCursor + MergeCursor

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Andrzej Hunt :ahunt]

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.

Comment 1

[Andrzej Hunt :ahunt]

(I almost have a patch for this, hence assigning to myself.)

Comment 2

[Andrzej Hunt :ahunt]

Attachment: 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/

Comment 3

[Andrzej Hunt :ahunt]

Attachment: 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/

Comment 4

[Andrzej Hunt :ahunt]

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/

Comment 5

[Andrzej Hunt :ahunt]

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 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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`?

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

https://reviewboard.mozilla.org/r/37241/#review33869

Comment 8

[Andrzej Hunt :ahunt]

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/

Comment 9

[Andrzej Hunt :ahunt]

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/

Comment 10

[Andrzej Hunt :ahunt]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5620bb00b64a
https://hg.mozilla.org/mozilla-central/rev/03d260b11ec9