912994 - Update top sites query to exclude reading list items

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

Under the hood, reading list items are bookmarks, but we don't expose that to the user. Do we want to be including them in the top bookmarks thumbnails?

Comment 1

[Ian Barlow (:ibarlow)]

Hm, interesting. I would say no.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ian Barlow (:ibarlow) from comment #1)
> Hm, interesting. I would say no.

Agreed. In fact, I'd like to consider moving the reading list items out of the bookmark system and into their own first-class system. We have plans for the reading list that go beyond bookmarks.

Comment 3

[:Margaret Leibovic]

Attachment: patch

This patch updates this query to exclude any and all fake bookmarks from the top bookmarks results.

I also got rid of the use of DBUtils.appendSelectionArgs, since I didn't see any reason for it...

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 804793 [details] [diff] [review]
patch

Review of attachment 804793 [details] [diff] [review]:
-----------------------------------------------------------------

Minusing for feedback?

::: mobile/android/base/db/LocalBrowserDB.java
@@ +243,3 @@
>                                               DBUtils.qualifyColumn("bookmarks", Bookmarks.IS_DELETED) + " == 0)");
> +
> +        String[] selectionArgs = new String[] { String.valueOf(Bookmarks.FIXED_ROOT_ID) };

This will filter out desktop bookmarks as well. Do we want that?

Comment 5

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #4)
> Comment on attachment 804793 [details] [diff] [review]
> patch
> 
> Review of attachment 804793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Minusing for feedback?
> 
> ::: mobile/android/base/db/LocalBrowserDB.java
> @@ +243,3 @@
> >                                               DBUtils.qualifyColumn("bookmarks", Bookmarks.IS_DELETED) + " == 0)");
> > +
> > +        String[] selectionArgs = new String[] { String.valueOf(Bookmarks.FIXED_ROOT_ID) };
> 
> This will filter out desktop bookmarks as well. Do we want that?

No, it won't. Desktop bookmarks all have real parents with id > 1. We just make a fake desktop bookmarks folder for our UI (our UI doesn't actually represent the parent structure that's in the DB).

Comment 6

[:Margaret Leibovic]

(This also might not be relevant anymore given our discussion about changing the behavior of these thumbnails, since the query will change in that case.)

Comment 7

[:Margaret Leibovic]

Actually, this is still a valid issue with the new new about:home. However, it's an issue we must have had forever with the top sites query, so probably not worth tracking anymore.

Comment 8

[:Margaret Leibovic]

Attachment: updated patch

Comment 9

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/5d3704320ce5

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d3704320ce5

Comment 11

[Lucas Rocha (:lucasr)]

Margaret, request uplift to Aurora?

Comment 12

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #11)
> Margaret, request uplift to Aurora?

I can do that if we think it's important, but it's not a regression from the new about:home.

Comment 13

[:Margaret Leibovic]

Comment on attachment 808829 [details] [diff] [review]
updated patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): not a regression, but a more prominent feature with the new about:home
User impact if declined: reading list items will appear in "top sites" thumbnails
Testing completed (on m-c, etc.): landed on m-c 9/25
Risk to taking this patch (and alternatives if risky): low-risk, slightly tweaking selection in top sites query
String or IDL/UUID changes made by this patch: none

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 808829 [details] [diff] [review]
updated patch

Sounds like nice-to-have for the new feature and low risk enough we can uplift to Aurora.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9e5e23f67e31