Update top sites query to exclude reading list items

RESOLVED FIXED in Firefox 26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
Firefox 27
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox26 fixed, firefox27 fixed, fennec26+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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)

Updated

5 years ago
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+
(Assignee)

Comment 3

5 years ago
Created attachment 804793 [details] [diff] [review]
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...
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-
(Assignee)

Comment 5

5 years ago
(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).
(Assignee)

Comment 6

5 years ago
(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.)
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: Top bookmarks thumbnails include reading list items → Update top sites query to exclude reading list items
(Assignee)

Comment 8

5 years ago
Created attachment 808829 [details] [diff] [review]
updated patch
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Margaret, request uplift to Aurora?
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 12

5 years ago
(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)
(Assignee)

Comment 13

5 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/9e5e23f67e31
status-firefox26: --- → fixed
status-firefox27: --- → fixed
You need to log in before you can comment on or make changes to this bug.