Closed Bug 912994 Opened 7 years ago Closed 7 years ago

Update top sites query to exclude reading list items

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 27
Tracking Status
firefox26 --- fixed
firefox27 --- fixed
fennec 26+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file, 1 obsolete file)

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?
Flags: needinfo?(ibarlow)
Hm, interesting. I would say no.
Flags: needinfo?(ibarlow)
Assignee: nobody → margaret.leibovic
(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.
tracking-fennec: ? → 26+
Attached patch patch (obsolete) — Splinter Review
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...
Attachment #804793 - Flags: review?(wjohnston)
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?
Attachment #804793 - Flags: review?(wjohnston) → review-
(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).
(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.)
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.
Summary: Top bookmarks thumbnails include reading list items → Update top sites query to exclude reading list items
Attached patch updated patchSplinter Review
Attachment #804793 - Attachment is obsolete: true
Attachment #808829 - Flags: review?(wjohnston)
Attachment #808829 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/5d3704320ce5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Margaret, request uplift to Aurora?
Flags: needinfo?(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.
Flags: needinfo?(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
Attachment #808829 - Flags: approval-mozilla-aurora?
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.
Attachment #808829 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.