Closed Bug 885084 Opened 7 years ago Closed 7 years ago

BookmarksPage should show only the top bookmarks

Categories

(Firefox for Android :: General, defect, P1)

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: sriram, Assigned: Margaret)

References

Details

Attachments

(1 file)

BookmarksPage should show only the top bookmarks and the pinned ones. Currently (from bug 882365) it uses all top sites query.
Blocks: 882365
0xDBDBDB.
Duplicate of this bug: 905316
Assignee: nobody → margaret.leibovic
Priority: -- → P1
Attached patch patchSplinter Review
It makes me sad to continue to hack on special cases to all our queries, but this seems to be the simplest way to get this done.

I verified that the combined view does indeed have a NULL bookmark_id for non-bookmark items. Not to be confused with non-history items, which have history_id = -1. I really don't like this code.
Attachment #791315 - Flags: review?(wjohnston)
Comment on attachment 791315 [details] [diff] [review]
patch

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

I think we could probably do something much simpler here now, but I think its probably left to a bigger history/bookmarks cleanup.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +231,5 @@
>      }
>  
>      @Override
> +    public Cursor getTopBookmarks(ContentResolver cr, int limit) {
> +        // Only select bookmarks. Unfortunately, we need to query the combined view, 

ws

@@ +233,5 @@
>      @Override
> +    public Cursor getTopBookmarks(ContentResolver cr, int limit) {
> +        // Only select bookmarks. Unfortunately, we need to query the combined view, 
> +        // instead of just the bookmarks table, in order to do the frecency calculation.
> +        String selection = Combined.BOOKMARK_ID + " IS NOT NULL";

I'm not even sure why this concat is here. It makes sense if you've got an unknown string you're working with, but in our case... we don't.
Attachment #791315 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #4)

> @@ +233,5 @@
> >      @Override
> > +    public Cursor getTopBookmarks(ContentResolver cr, int limit) {
> > +        // Only select bookmarks. Unfortunately, we need to query the combined view, 
> > +        // instead of just the bookmarks table, in order to do the frecency calculation.
> > +        String selection = Combined.BOOKMARK_ID + " IS NOT NULL";
> 
> I'm not even sure why this concat is here. It makes sense if you've got an
> unknown string you're working with, but in our case... we don't.

Do you mean why not just do selection = "bookmark_id IS NOT NULL"? I was being consistent with all the concatenation we currently do in this file, but I agree it sucks, so maybe now should be the time to change that?
tracking-fennec: --- → ?
Hardware: ARM → All
Duplicate of this bug: 907809
https://hg.mozilla.org/mozilla-central/rev/936c053afda0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Blocks: 917455
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.