860759 - Don't treat pinned sites as bookmarks unless they are bookmarks

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Why are pinned tabs using the bookmark star in the Top-Sites tab? This gives off the impression that it's bookmarked. When checking the bookmark tab, one wont see the item.

STR

i) http://mozilla.com
ii) Pin it as a top-site on about:home
iii) Visit the Top-Sites tab; see it starred

--
Nightly (04/11)
LG Nexus 4 (Android 4.2.2)

Comment 1

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

Pinned sites are being treated as bookmarks, even if they aren't. The check needs to be "pinned aware"

Comment 2

[:Margaret Leibovic]

The relevant code is here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/awesomebar/AllPagesTab.java#749

It looks like we may need to add the Bookmarks.PARENT column to the cursor, or we should update our filter() method to handle this:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#223

getTopSites() already does something to deal with pinned sites, I'm wondering if we can move that logic into filterAllSites.

Note, we already account for this is our other isBookmark() check, which handles things like updating the star in the menu:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#475

Comment 3

[:Margaret Leibovic]

Attachment: patch

This does the job, but I'm not sure if there's a more elegant way to go about it. I'm also not sure if we're actually expecting to have this pinned bookmark data in any consumers of this view.

These combined view migrations are pretty gross to review. To help, here's the diff with createCombinedViewOn13:

                                         Bookmarks.FIXED_READING_LIST_ID + " THEN " + Combined.DISPLAY_READER + " ELSE " +
                                         Combined.DISPLAY_NORMAL + " END AS " + Combined.DISPLAY + ", " +
                                      "-1 AS " + Combined.HISTORY_ID + ", " +
                                      "-1 AS " + Combined.VISITS + ", " +
                                      "-1 AS " + Combined.DATE_LAST_VISITED + ", " +
                                      qualifyColumn(TABLE_BOOKMARKS, Bookmarks.FAVICON_ID) + " AS " + Combined.FAVICON_ID +
                         " FROM " + TABLE_BOOKMARKS +
                         " WHERE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE)  + " = " + Bookmarks.TYPE_BOOKMARK + " AND " +
+                                    // Ignore pinned bookmarks.
+                                    qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT)  + " <> " + Bookmarks.FIXED_PINNED_LIST_ID + " AND " +
                                     qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED)  + " = 0 AND " +
                                     qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) +
                                         " NOT IN (SELECT " + History.URL + " FROM " + TABLE_HISTORY + ")" +
                         " UNION ALL" +
                         // History with and without bookmark.
                         " SELECT " + "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + " WHEN 0 THEN " +
-                                     qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) +  " ELSE NULL END AS " + Combined.BOOKMARK_ID + ", " +
+                                        // Give pinned bookmarks a NULL ID so that they're not treated as bookmarks. We can't
+                                        // completely ignore them here because they're joined with history entries we care about.
+                                        "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " WHEN " +
+                                        Bookmarks.FIXED_PINNED_LIST_ID + " THEN NULL ELSE " +
+                                        qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) + " END " +
+                                     "ELSE NULL END AS " + Combined.BOOKMARK_ID + ", " +
                                      qualifyColumn(TABLE_HISTORY, History.URL) + " AS " + Combined.URL + ", " +
                                      // Prioritize bookmark titles over history titles, since the user may have
                                      // customized the title for a bookmark.
                                      "COALESCE(" + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TITLE) + ", " +
                                                    qualifyColumn(TABLE_HISTORY, History.TITLE) +")" + " AS " + Combined.TITLE + ", " +
                                      // Only use DISPLAY_READER if the matching bookmark entry inside reading
                                      // list folder is not marked as deleted.
                                      "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + " WHEN 0 THEN CASE " +

Comment 4

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

Once the new Home page design lands, we won't need this fix. Top Sites will only bookmarked sites.

Comment 5

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Once the new Home page design lands, we won't need this fix. Top Sites will
> only bookmarked sites.

I hope that's your way of saying we should just WONTFIX this :)

Comment 6

[Brian Nicholson (:bnicholson)]

In case we decide to go with this patch: does it handle the case where an item is both pinned and a bookmark?

Comment 7

[Wesley Johnston (:wesj)]

I think we should be able to do this in LocalBrowserDB like we do in getTopSites. i.e. filterAllSites already does some modifying to selection and selectionArgs if the filter is non-null. Can we just add:

selection = DBUtils.concatenateWhere("", Combined.URL + " NOT IN (SELECT "+ Bookmarks.URL + " FROM bookmarks WHERE bookmarks." + Bookmarks.PARENT + " == ?)");

String[] selectionArgs = DBUtils.appendSelectionArgs(new String[0], new String[] { String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID) });

Comment 8

[Wesley Johnston (:wesj)]

Orthogonal, but I wonder if we can make update createCombinedViewOn15 to something more like:

void createCombinedViewForVersion(SQLiteDatabase db, int version) {
  StringBuilder createStatement = new StringBuilder("CREATE TABLE " ....);

  if (version == 15) createStatement.append("DO SOME 15 stuff");
  else createStatement.append("DO SOME other stuff");

  db.executeSQL(createStatement.toString());
}

Comment 9

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> In case we decide to go with this patch: does it handle the case where an
> item is both pinned and a bookmark?

Yes, in that case there are two different bookmarks, so we keep the data associated with the regular bookmark, but not with the pinned bookmark. I just tested to verify.

(In reply to Wesley Johnston (:wesj) from comment #7)
> I think we should be able to do this in LocalBrowserDB like we do in
> getTopSites. i.e. filterAllSites already does some modifying to selection
> and selectionArgs if the filter is non-null. Can we just add:
> 
> selection = DBUtils.concatenateWhere("", Combined.URL + " NOT IN (SELECT "+
> Bookmarks.URL + " FROM bookmarks WHERE bookmarks." + Bookmarks.PARENT + " ==
> ?)");
> 
> String[] selectionArgs = DBUtils.appendSelectionArgs(new String[0], new
> String[] { String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID) });

I believe that will make us filter out the entry altogether, when what we really want to do is just make sure that it's not marked as being a bookmark. We do want to show these history entries, we just want the star to not show up.

Comment 10

[:Margaret Leibovic]

Attachment: patch

Updated to apply on top of bug 858994.

We should decide how much we care about this problem. If we we're willing to live with it until the new about:home redesign lands, we can WONTFIX this bug, but otherwise, this is the way to fix it.

Comment 11

[Brian Nicholson (:bnicholson)]

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

LGTM

Comment 12

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0268b65bc46

Comment 13

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/b0268b65bc46

Comment 14

[Richard Newman [:rnewman]]

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

Requesting uplift per Margaret's note in Bug 858994 comment 29.

Comment 15

[:Margaret Leibovic]

Attention auto-uplifters, I want to see how bug 866150 plays out before uplifting this to aurora, so please don't uplift this for me.

Comment 16

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4f16fb0f873c

Comment 17

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 22 Beta 1 (2013-05-15)
Device: LG Nexus 4 
OS:Android 4.2.2

Comment 18

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 23.0a2 (2013-05-16)
Device: Samsung Galaxy Tab
OS:Android 4.0.4