Closed Bug 1396054 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Android
enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: liuche, Assigned: liuche)

References

Details

(Whiteboard: [MobileAS])

Attachments

(1 file)

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: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.