Follow-up: Filter out "recent bookmarks" and "history" based on AS Top Sites pref

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: liuche, Assigned: liuche)

Tracking

Trunk
Firefox 57
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [MobileAS])

Attachments

(1 attachment)

Splitting this off from bug 1386735, so that we can land the pieces that are already present (that is, controlling the section visibility).

Highlights has two levels of controls, but currently each individually doesn't do anything, but toggling both off hides highlights. This is the follow-up to handle filtering out history/bookmarks based on the prefs state.
Priority: -- → P1
Iteration: --- → 1.29
Assignee: nobody → liuche
Whiteboard: [MobileAS]
Comment on attachment 8904729 [details]
Bug 1396054 - Filter highlight candidates based on settings.

https://reviewboard.mozilla.org/r/176528/#review182590

Nice and simple, lgtm!

::: mobile/android/base/java/org/mozilla/gecko/activitystream/ranking/HighlightCandidate.java:121
(Diff revision 2)
>              // Note: we used to throw an exception but sometimes many Exceptions were thrown, potentially
>              // impacting performance so we changed it to a boolean return.
>              return false;
>          }
>  
> +        candidate.isBookmark = cursor.getDouble(cursorIndices.bookmarkIDColumnIndex) != -1;

nit: constant for -1 and then you can add a comment to that constant to explain where -1 comes from (the query!)

::: mobile/android/base/java/org/mozilla/gecko/activitystream/ranking/HighlightCandidate.java:216
(Diff revision 2)
>          return host;
>      }
>  
> +    /* package-private */ boolean isBookmark() {
> +        return isBookmark;
> +    }

nit: ws below

::: mobile/android/base/java/org/mozilla/gecko/activitystream/ranking/HighlightsRanking.java:116
(Diff revision 2)
>  
>          normalize(highlights);
>  
>          scoreEntries(highlights);
>  
> +        filterOutItemsPreffedOff(highlights, includeHistory, includeBookmarks);

Any reason not to do this right after we get the list of highlights? It'd be more efficient if we didn't need to normalize and score a bunch of entries we know we'll cut.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1276
(Diff revision 2)
>                  DBUtils.qualifyColumn(Bookmarks.TABLE_NAME, Bookmarks.POSITION) + " AS " + Highlights.POSITION + ", " +
>                  DBUtils.qualifyColumn(Bookmarks.TABLE_NAME, Bookmarks.PARENT) + " AS " + Highlights.PARENT + ", " +
>                  DBUtils.qualifyColumn(History.TABLE_NAME, History._ID) + " AS " + Highlights.HISTORY_ID + ", " +
>  
> +                /**
> +                 * NB: Highlights filtering depends on Bookmarks._ID, so changes to this logic should also update

nit: -> `BOOKMARK_ID`, which is the last name of this column.
Attachment #8904729 - Flags: review?(michael.l.comella) → review+
Pushed by cliu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ec55a721cf1
Filter highlight candidates based on settings. r=mcomella
https://hg.mozilla.org/mozilla-central/rev/5ec55a721cf1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.