885084 - BookmarksPage should show only the top bookmarks

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

BookmarksPage should show only the top bookmarks and the pinned ones. Currently (from bug 882365) it uses all top sites query.

Comment 1

[Sriram Ramasubramanian [:sriram]]

0xDBDBDB.

Comment 3

[:Margaret Leibovic]

Attachment: patch

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.

Comment 4

[Wesley Johnston (:wesj)]

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.

Comment 5

[:Margaret Leibovic]

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

Comment 7

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/936c053afda0

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/936c053afda0