Bug 1396054 - Filter highlight candidates based on settings. r?mcomella draft
authorChenxia Liu <liuche@mozilla.com>
Sun, 03 Sep 2017 09:35:01 -0700
changeset 661543 95197ac37eff0765a797bdfacfb949360dc418ee
parent 661528 6d0288c291650ab619dc4ff92cb0b60e3b6e7b3b
child 730627 ea21c45b7c956aa37c69ec1c8a42a2c0b6307ece
push id78821
push usercliu@mozilla.com
push dateFri, 08 Sep 2017 18:39:59 +0000
reviewersmcomella
bugs1396054
milestone57.0a1
Bug 1396054 - Filter highlight candidates based on settings. r?mcomella MozReview-Commit-ID: 8e36KZsuh27
mobile/android/app/src/main/res/layout/preference_topsites_panel_dialog.xml
mobile/android/app/src/main/res/values/bool.xml
mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsLoader.java
mobile/android/base/java/org/mozilla/gecko/activitystream/ranking/HighlightCandidate.java
mobile/android/base/java/org/mozilla/gecko/activitystream/ranking/HighlightsRanking.java
mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
--- a/mobile/android/app/src/main/res/layout/preference_topsites_panel_dialog.xml
+++ b/mobile/android/app/src/main/res/layout/preference_topsites_panel_dialog.xml
@@ -23,29 +23,29 @@
     <org.mozilla.gecko.widget.SwitchPreferenceView
         android:id="@+id/preference_pocket"
         android:layout_width="match_parent"
         android:layout_height="wrap_content"
         style="@style/Gecko.SwitchPreferenceView"
         android:paddingBottom="@dimen/dialog_switchpreferenceview_padding"
         android:text="@string/activity_stream_topstories"
         gecko:androidPreferenceKey="pref_activitystream_pocket_enabled"
-        gecko:defaultValue="true"/>
+        gecko:defaultValue="@bool/pref_activitystream_pocket_enabled_default"/>
 
     <org.mozilla.gecko.widget.SwitchPreferenceView
         android:layout_width="match_parent"
         android:layout_height="wrap_content"
         style="@style/Gecko.SwitchPreferenceView"
         android:paddingBottom="@dimen/dialog_switchpreferenceview_padding"
         android:text="@string/pref_dialog_activitystream_recentBookmarks"
         gecko:androidPreferenceKey="pref_activitystream_recentbookmarks_enabled"
-        gecko:defaultValue="true"/>
+        gecko:defaultValue="@bool/pref_activitystream_recentbookmarks_enabled_default"/>
 
     <org.mozilla.gecko.widget.SwitchPreferenceView
         android:layout_width="match_parent"
         android:layout_height="wrap_content"
         style="@style/Gecko.SwitchPreferenceView"
         android:paddingBottom="@dimen/dialog_switchpreferenceview_padding"
         android:text="@string/pref_dialog_activitystream_visited"
         gecko:androidPreferenceKey="pref_activitystream_visited_enabled"
-        gecko:defaultValue="true"/>
+        gecko:defaultValue="@bool/pref_activitystream_visited_enabled_default"/>
 
 </LinearLayout>
\ No newline at end of file
--- a/mobile/android/app/src/main/res/values/bool.xml
+++ b/mobile/android/app/src/main/res/values/bool.xml
@@ -10,9 +10,12 @@
          with the configuration retrieved by HardwareUtils (e.g. some custom ROMs allow the user to
          choose a phone or tablet version of the UI even though the hardware stays the same). This
          can cause crashes when we branch on the value returned by HardwareUtils.
 
          In order to work around this, we define the resource size in resources with the expectation that
          we branch on that value, rather than HardwareUtils, so our code is consistent with the used resources.
          See bug 1277379 for a initiative to move all of the HardwareUtils code over. -->
     <bool name="is_large_resource">false</bool>
+    <bool name="pref_activitystream_pocket_enabled_default">true</bool>
+    <bool name="pref_activitystream_visited_enabled_default">true</bool>
+    <bool name="pref_activitystream_recentbookmarks_enabled_default">true</bool>
 </resources>
--- a/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsLoader.java
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsLoader.java
@@ -1,22 +1,26 @@
 /* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.activitystream.homepanel;
 
 import android.content.Context;
+import android.content.SharedPreferences;
+import android.content.res.Resources;
 import android.database.ContentObserver;
 import android.database.Cursor;
 import android.os.SystemClock;
 import android.support.annotation.WorkerThread;
 import android.support.v4.content.AsyncTaskLoader;
 
+import org.mozilla.gecko.GeckoSharedPrefs;
+import org.mozilla.gecko.R;
 import org.mozilla.gecko.Telemetry;
 import org.mozilla.gecko.activitystream.ranking.HighlightsRanking;
 import org.mozilla.gecko.db.BrowserContract;
 import org.mozilla.gecko.db.BrowserDB;
 import org.mozilla.gecko.activitystream.homepanel.model.Highlight;
 import org.mozilla.gecko.util.ThreadUtils;
 
 import java.util.Collections;
@@ -58,17 +62,22 @@ import java.util.List;
         if (candidatesCursor == null) {
             return Collections.emptyList();
         }
 
         try {
             // From now on get notified about content updates and reload data - until loader is reset.
             enableContentUpdates();
 
-            final List<Highlight> highlights = HighlightsRanking.rank(candidatesCursor, highlightsLimit);
+            final SharedPreferences prefs = GeckoSharedPrefs.forProfile(getContext());
+            final Resources res = getContext().getResources();
+            final boolean includeHistory = prefs.getBoolean(ActivityStreamPanel.PREF_VISITED_ENABLED, res.getBoolean(R.bool.pref_activitystream_visited_enabled_default));
+            final boolean includeBookmarks = prefs.getBoolean(ActivityStreamPanel.PREF_BOOKMARKS_ENABLED, res.getBoolean(R.bool.pref_activitystream_recentbookmarks_enabled_default));
+
+            final List<Highlight> highlights = HighlightsRanking.rank(candidatesCursor, highlightsLimit, includeHistory, includeBookmarks);
             forceLoadHighlightMetadata(highlights); // force load now that we have a short list of the data.
 
             addToPerformanceHistogram(startTime);
 
             return highlights;
         } finally {
             candidatesCursor.close();
         }
--- a/mobile/android/base/java/org/mozilla/gecko/activitystream/ranking/HighlightCandidate.java
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/ranking/HighlightCandidate.java
@@ -1,16 +1,17 @@
 /* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.activitystream.ranking;
 
 import android.database.Cursor;
+import android.database.sqlite.SQLiteDatabase;
 import android.net.Uri;
 import android.support.annotation.CheckResult;
 import android.support.annotation.IntDef;
 import android.support.annotation.Nullable;
 import android.support.annotation.VisibleForTesting;
 import org.mozilla.gecko.activitystream.homepanel.model.Highlight;
 
 import java.lang.annotation.Retention;
@@ -63,21 +64,28 @@ import java.lang.annotation.RetentionPol
             return values[featureName];
         }
 
         /* package-private */ void put(final @FeatureName int featureName, final double value) {
             values[featureName] = value;
         }
     }
 
+    /**
+     * The BOOKMARK_ID colmun value is set to -1 for non-bookmarks in
+     * {@link org.mozilla.gecko.db.BrowserProvider#getHighlightCandidates(SQLiteDatabase, String)}
+     */
+    private static final int COLUMN_VALUE_NON_BOOKMARK = -1;
+
     @VisibleForTesting final Features features = new Features();
     private Highlight highlight;
     private @Nullable String imageUrl;
     private String host;
     private double score;
+    private boolean isBookmark;
 
     /**
      * @return the HighlightCandidate, or null if the candidate is invalid.
      */
     @Nullable
     public static HighlightCandidate fromCursor(final Cursor cursor, final HighlightCandidateCursorIndices cursorIndices) {
         final HighlightCandidate candidate = new HighlightCandidate();
 
@@ -112,16 +120,18 @@ import java.lang.annotation.RetentionPol
         // The latter might simply be URIs with invalid characters in them (such as underscore...).
         // See Bug 1363521 and Bug 1378967.
         if (!uri.isHierarchical() || uri.getHost() == null) {
             // 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) != COLUMN_VALUE_NON_BOOKMARK;
+
         candidate.features.put(
                 FEATURE_AGE_IN_DAYS,
                 (System.currentTimeMillis() - cursor.getDouble(cursorIndices.historyDateLastVisitedColumnIndex))
                         / (1000 * 3600 * 24));
 
         candidate.features.put(
                 FEATURE_VISITS_COUNT,
                 cursor.getDouble(cursorIndices.visitsColumnIndex));
@@ -203,16 +213,20 @@ import java.lang.annotation.RetentionPol
     /* package-private */ String getUrl() {
         return highlight.getUrl();
     }
 
     /* package-private */ String getHost() {
         return host;
     }
 
+    /* package-private */ boolean isBookmark() {
+        return isBookmark;
+    }
+
     /**
      * Gets an estimate of the actual image url that should only be used to compare against other return
      * values of this method. See {@link Highlight#getFastImageURLForComparison()} for more details.
      */
     @Nullable
     /* package-private */ String getFastImageUrlForComparison() {
         return imageUrl;
     }
--- a/mobile/android/base/java/org/mozilla/gecko/activitystream/ranking/HighlightsRanking.java
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/ranking/HighlightsRanking.java
@@ -6,27 +6,30 @@
 package org.mozilla.gecko.activitystream.ranking;
 
 import android.database.Cursor;
 import android.net.Uri;
 import android.support.annotation.VisibleForTesting;
 import android.text.TextUtils;
 import android.util.Log;
 import android.util.SparseArray;
+
+import org.mozilla.gecko.SharedPreferencesHelper;
 import org.mozilla.gecko.activitystream.homepanel.model.Highlight;
 import org.mozilla.gecko.util.MapUtils;
 
 import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
+import static android.R.attr.candidatesTextStyleSpans;
 import static android.R.attr.filter;
 import static java.util.Collections.sort;
 import static org.mozilla.gecko.activitystream.ranking.HighlightCandidate.FEATURE_AGE_IN_DAYS;
 import static org.mozilla.gecko.activitystream.ranking.HighlightCandidate.FEATURE_BOOKMARK_AGE_IN_MILLISECONDS;
 import static org.mozilla.gecko.activitystream.ranking.HighlightCandidate.FEATURE_DESCRIPTION_LENGTH;
 import static org.mozilla.gecko.activitystream.ranking.HighlightCandidate.FEATURE_DOMAIN_FREQUENCY;
 import static org.mozilla.gecko.activitystream.ranking.HighlightCandidate.FEATURE_IMAGE_COUNT;
 import static org.mozilla.gecko.activitystream.ranking.HighlightCandidate.FEATURE_IMAGE_SIZE;
@@ -98,19 +101,21 @@ public class HighlightsRanking {
     private static final double BOOKMARK_AGE_DIVIDEND = 3 * 24 * 60 * 60 * 1000;
 
     /**
      * Create a list of highlights based on the candidates provided by the input cursor.
      *
      * THIS METHOD IS CRITICAL FOR HIGHLIGHTS PERFORMANCE AND HAS BEEN OPTIMIZED (bug 1369604):
      * please be careful what you add to it!
      */
-    public static List<Highlight> rank(Cursor cursor, int limit) {
+    public static List<Highlight> rank(Cursor cursor, int limit, boolean includeHistory, boolean includeBookmarks) {
         List<HighlightCandidate> highlights = extractFeatures(cursor);
 
+        filterOutItemsPreffedOff(highlights, includeHistory, includeBookmarks);
+
         normalize(highlights);
 
         scoreEntries(highlights);
 
         filterOutItemsWithNoScore(highlights);
 
         sortDescendingByScore(highlights);
 
@@ -202,16 +207,33 @@ public class HighlightsRanking {
                     return 1;
                 } else {
                     return 0;
                 }
             }
         });
     }
 
+    @VisibleForTesting static void filterOutItemsPreffedOff(List<HighlightCandidate> candidates, final boolean includeHistory, final boolean includeBookmarks) {
+        // Pinned items are not bookmarks, and will be grouped with history.
+        filter(candidates, new Func1<HighlightCandidate, Boolean>() {
+            @Override
+            public Boolean call(HighlightCandidate candidate) {
+                if (includeBookmarks && includeHistory) {
+                    return true;
+                } else if (!includeBookmarks && !includeHistory) {
+                    return false;
+                } else {
+                    // Either B or H are enabled, but not both, so we can filter on bookmark state.
+                    return includeBookmarks == candidate.isBookmark();
+                }
+            }
+        });
+    }
+
     /**
      * Remove all items without or with a negative score.
      */
     @VisibleForTesting static void filterOutItemsWithNoScore(List<HighlightCandidate> candidates) {
         filter(candidates, new Func1<HighlightCandidate, Boolean>() {
             @Override
             public Boolean call(HighlightCandidate candidate) {
                 return candidate.getScore() > 0;
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ -1267,16 +1267,20 @@ public class BrowserProvider extends Sha
                 DBUtils.qualifyColumn(PageMetadata.TABLE_NAME, PageMetadata.JSON) + " AS " + Highlights.METADATA + ", " +
                 DBUtils.qualifyColumn(Bookmarks.TABLE_NAME, Bookmarks.DATE_CREATED) + ", " +
 
                 DBUtils.qualifyColumn(History.TABLE_NAME, History.TITLE) + " AS " + Highlights.TITLE + ", " +
                 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 BOOKMARK_ID, so changes to this logic should also update
+                 * {@link org.mozilla.gecko.activitystream.ranking.HighlightsRanking#filterOutItemsPreffedOff(List, boolean, boolean)}
+                 */
                 "CASE WHEN " + DBUtils.qualifyColumn(Bookmarks.TABLE_NAME, Bookmarks._ID) + " IS NOT NULL "
                 + "AND " + DBUtils.qualifyColumn(Bookmarks.TABLE_NAME, Bookmarks.PARENT) + " IS NOT " + Bookmarks.FIXED_PINNED_LIST_ID + " "
                 + "THEN " + DBUtils.qualifyColumn(Bookmarks.TABLE_NAME, Bookmarks._ID) + " "
                 + "ELSE -1 "
                 + "END AS " + Highlights.BOOKMARK_ID + ", " +
 
                 "CASE WHEN " + DBUtils.qualifyColumn(Bookmarks.TABLE_NAME, Bookmarks.DATE_CREATED) + " IS NOT NULL "
                     + "THEN " + DBUtils.qualifyColumn(Bookmarks.TABLE_NAME, Bookmarks.DATE_CREATED) + " "