1010266 - Blacklist Top Sites suggestions that have been "removed" by user

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Bug 1010261 will handle adding a setting for toggling Suggested Sites, but we should also support disabling suggested sites from Top Sites, to be consistent with bug 913457.

Comment 1

[Chenxia Liu [:liuche]]

Thoughts, Ian? If how much people try to remove top sites is any indication, this will be something people want to do. I was thinking "Hide suggested sites" as a string, but I don't know if that's what we're planning on officially/publicly calling that feature.

Comment 2

[Lucas Rocha (:lucasr)]

(In reply to Chenxia Liu [:liuche] from comment #0)
> Bug 1010261 will handle adding a setting for toggling Suggested Sites, but
> we should also support disabling suggested sites from Top Sites, to be
> consistent with bug 913457.

FYI: bug 1010261 will probably be a bit more fine-grained than just a global toggle for suggested sites. For instance, I'll probably add the ability to enable/disable specific types of suggested sites. The initial types would be something like 'Mozilla' and 'Third-party sites'. In the long-term we might end up with an extra 'Sponsored' type. I still need to discuss this with ibarlow/deb/mfinkle.

The problem I see with hiding specific suggested sites is that we'd probably need to provide UI for editing the 'blacklist' somewhere. Or maybe I'm just overthinking this feature?

Comment 3

[Chenxia Liu [:liuche]]

It seems like having different classes of suggested sites lends itself to a checkbox dialog - this could definitely fit in Settings, under the Mozilla settings page. The same checkbox could be launched from the top sites context menu for consistency, though I'm less sure about how we could describe that in the context menu in a way that makes sense.

In general, I'm concerned about the impression we convey if we allow removing of the top sites grid items, but don't support it for suggested sites - this has the potential for really negative implications, especially on an already touchy subject (ads! sponsored sites! etc)

Comment 4

[Ian Barlow (:ibarlow)]

(In reply to Chenxia Liu [:liuche] from comment #1)
> Thoughts, Ian? If how much people try to remove top sites is any indication,
> this will be something people want to do. I was thinking "Hide suggested
> sites" as a string, but I don't know if that's what we're planning on
> officially/publicly calling that feature.

I think there should only be one "Remove" command in the thumbnail contextual menu. 

That command would remove anything that's currently in that thumbnail spot. If it's a history item or bookmark, it behaves as it does currently. If it's a suggested site, it nukes that suggested site. I don't think we would need to expose a way for users to edit a "blacklist" as Lucas suggests, as long as Firefox remembers the choice and doesn't show the same site again later on. 

Now, if a user is on their homepage and starts removing one suggested site after another, perhaps we could take that as a cue to display some kind of contextual help after they remove the second or third item. Perhaps some kind of menu that asks if you just want to turn off suggested sites altogether.

Comment 5

[Chenxia Liu [:liuche]]

Lucas, do you think we should file a separate bug for blacklisting, or morph this bug into creating the blacklisting of specific suggested sites? I looked through the code for loading suggested sites and it doesn't really lend itself to changing the sites that are loaded, since they're all readonly from files.

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to Chenxia Liu [:liuche] from comment #5)
> Lucas, do you think we should file a separate bug for blacklisting, or morph
> this bug into creating the blacklisting of specific suggested sites? I
> looked through the code for loading suggested sites and it doesn't really
> lend itself to changing the sites that are loaded, since they're all
> readonly from files.

Let's concentrate all the work around blacklisting suggested sites + the necessary UI changes in this bug as they are all part of the same feature. Maybe we can start with something as simple as storing the blacklist in a SharedPref or something?

Comment 7

[Aaron Train [:aaronmt]]

The current behaviour in Nightly is that 'Remove' does not work on these tiles, see bug 1016363 - remove will need to be removed or tweaked for these tiles in Fx32.

Comment 9

[Chenxia Liu [:liuche]]

Have most of a WIP for blacklisting, but I want to clean up the ugly string parsing a SharedPreference first. Shouldn't take much longer though!

Comment 10

[Chenxia Liu [:liuche]]

Attachment: Patch: WIP Remove Suggested Sites

I'm not sure about the right place to handle refreshing the adapter. I tried adding a refresh() call after blacklisting a site, but I think there might be a bug because the first suggested site doesn't get removed properly for some reason.

I'm not sure how calling notifyDataSetChanged/Invalidated on the Top Sites GridAdapter should work, but it doesn't seem to actually do anything. Is there a better way to force a refresh of the UI, since we don't actually hit the content provider to remove a suggested site?

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 8432926 [details] [diff] [review]
Patch: WIP Remove Suggested Sites

Review of attachment 8432926 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good overall. Add tests to TestSuggestedSites :-)

::: mobile/android/base/db/SuggestedSites.java
@@ +148,5 @@
> +                if (!hiddenSites.contains(url)) {
> +                    final Site site = new Site(jsonSite.getString(JSON_KEY_URL),
> +                                               jsonSite.getString(JSON_KEY_TITLE),
> +                                               jsonSite.getString(JSON_KEY_IMAGE_URL),
> +                                               jsonSite.getString(JSON_KEY_BG_COLOR));

Latest SuggestedSites has the notion of excludeSites in the get() calls. I think this blacklist should just be a built-in list that gets merged with the given list in get().

@@ +256,5 @@
> +    public boolean hideSite(String url) {
> +        List<Site> sites = cachedSites.get();
> +        if (sites == null) {
> +            sites = refresh();
> +        }

This has changed a bit in m-c. We now hold hard reference to the cached list of suggested sites. Also, cachedSites is now a LinkedHashMap which means you can simply do cachedSites.get(url) to check if the URL is in the list.

@@ +259,5 @@
> +            sites = refresh();
> +        }
> +
> +        for (Site site : sites) {
> +            if (TextUtils.equals(site.url, url)) {

You probably need to add a synchronized block here to avoid concurrent calls trying to write a different version of the blacklist.

@@ +266,5 @@
> +
> +                // Check if site has already been blacklisted, just in case.
> +                if (!siteString.matches("(.*\\s)*" + url + "(\\s.*)*")) {
> +                    final String prefString = siteString + " " + url;
> +                    ThreadUtils.postToBackgroundThread(new Runnable() {

I'd prefer the threading to be done by user of the this API instead of doing it directly here to make it easier to test this API.

Comment 12

[Chenxia Liu [:liuche]]

Attachment: Part 1: Remove Suggested Site

Comment 13

[Chenxia Liu [:liuche]]

Attachment: Part 1: Remove Suggested Site

Comment 14

[Chenxia Liu [:liuche]]

Attachment: Part 2: Tests

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 8435477 [details] [diff] [review]
Part 2: Tests

Review of attachment 8435477 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with suggested changes.

::: mobile/android/tests/browser/junit3/src/tests/TestSuggestedSites.java
@@ +198,5 @@
> +        List<String> visibleUrls = new ArrayList<String>(3);
> +        visibleUrls.add("url3");
> +        visibleUrls.add("url4");
> +        visibleUrls.add("url5");
> +        

nit: remove trailing spaces here.

@@ +205,5 @@
> +        hiddenUrls.add("url1");
> +        hiddenUrls.add("url2");
> +
> +        // Add mocked hidden sites to SharedPreferences.
> +        String hiddenUrlString = "";

Use StringBuilder instead.

@@ +220,5 @@
> +        while (c.moveToNext()) {
> +            String url = c.getString(c.getColumnIndexOrThrow(BrowserContract.SuggestedSites.URL));
> +            assertFalse(hiddenUrls.contains(url));
> +            assertTrue(visibleUrls.contains(url));
> +        }

assert on the cursor's count too.

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 8435476 [details] [diff] [review]
Part 1: Remove Suggested Site

Review of attachment 8435476 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good overall, still needs some thread-safety fixes.

::: mobile/android/base/db/SuggestedSites.java
@@ +224,5 @@
>              return cursor;
>          }
>  
> +        Log.d(LOGTAG, "Fetching blacklisted suggested sites from SharedPreferences.");
> +        final SharedPreferences preferences = GeckoSharedPrefs.forApp(context);

Shouldn't this blacklist be per-profile? In which case you should use forProfile() instead.

@@ +268,5 @@
>          final Site site = getSiteForUrl(url);
>          return (site != null ? site.bgColor : null);
>      }
> +
> +    private List<String> includeHiddenUrls(List<String> urlList, String urlsString) {

urlList -> urls

@@ +286,5 @@
> +        return urlList;
> +    }
> +
> +    /**
> +     * Blacklist a suggested site so it will no longer be returne as a suggested site.

returne -> returned

@@ +295,5 @@
> +     *
> +     * @param url String url of site to blacklist
> +     * @return true is blacklisted, false otherwise
> +     */
> +    public boolean hideSite(String url) {

You can add a ThreadUtils.assertNotOnUiThread() call here to catch incorrect hideSite() calls.

@@ +297,5 @@
> +     * @return true is blacklisted, false otherwise
> +     */
> +    public boolean hideSite(String url) {
> +        if (cachedSites == null || cachedSites.isEmpty()) {
> +            refresh();

This means we might be refreshing cachedSites from different threads in which case we need to make this code thread-safe too. The simplest way to go about this is to enclose all uses of cachedSites into synchronized blocks.

@@ +309,5 @@
> +                final SharedPreferences prefs = GeckoSharedPrefs.forApp(context);
> +                String siteString = prefs.getString(PREF_SUGGESTED_SITES_HIDDEN, "");
> +
> +                // Check if site has already been blacklisted, just in case.
> +                if (!siteString.matches("(.*\\s)*" + url + "(\\s.*)*")) {

This regex looks a bit suspicious. What you want is simply finding a SPACEurlSPACE instance, no? No need to use such a greedy regex there. 

This is prone to break if the URL is not properly encoded. For instance, you need to replace any space in the URL with the respective entity.

Comment 17

[Chenxia Liu [:liuche]]

Attachment: Part 1: Remove Suggested Site v2

Hm, the threading is tricky because cached sites are accessed all over the place, even in nested method calls. Is it enough to synchronize on the write method (refresh()), you think? hideSite should only be started in a background thread by the UI thread, through long-tap context menu > "Remove".

Any other suggestions?

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 8438211 [details] [diff] [review]
Part 1: Remove Suggested Site v2

Review of attachment 8438211 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Chenxia Liu [:liuche] from comment #17)
> Created attachment 8438211 [details] [diff] [review]
> Part 1: Remove Suggested Site v2
> 
> Hm, the threading is tricky because cached sites are accessed all over the
> place, even in nested method calls. Is it enough to synchronize on the write
> method (refresh()), you think? hideSite should only be started in a
> background thread by the UI thread, through long-tap context menu > "Remove".
> 
> Any other suggestions?

Synchronizing only refresh() is not enough because we might get thread visibility issues on cachedSites reads. From a quick look, it seems the simplest way to go about this is to synchronize all public methods. The SuggestedSites API doesn't need to have a high throughput so this shouldn't cause any performance issues as far as I can see.

::: mobile/android/base/db/SuggestedSites.java
@@ +226,5 @@
>          }
>  
> +        Log.d(LOGTAG, "Fetching blacklisted suggested sites from SharedPreferences.");
> +        final SharedPreferences preferences = GeckoSharedPrefs.forProfile(context);
> +        final String sitesString = preferences.getString(PREF_SUGGESTED_SITES_HIDDEN, "");

Missed from my last feedback: maybe you should memcache the parsed list of URLs to avoid having to read/split this string pref on every query. You'd then have to append newly hidden sites to this cached list in hideSite(). 

nit: use null instead of ""?

@@ +297,5 @@
> +     * @param url String url of site to blacklist
> +     * @return true is blacklisted, false otherwise
> +     */
> +    public boolean hideSite(String url) {
> +        ThreadUtils.assertNotOnUiThread();

nit: add empty line here.

@@ +298,5 @@
> +     * @return true is blacklisted, false otherwise
> +     */
> +    public boolean hideSite(String url) {
> +        ThreadUtils.assertNotOnUiThread();
> +        if (cachedSites == null || cachedSites.isEmpty()) {

Why do you need this isEmpty() check here?

@@ +316,5 @@
> +                final String prefString = siteString + " " + encodedUrl;
> +                prefs.edit().putString(PREF_SUGGESTED_SITES_HIDDEN, prefString).commit();
> +                return true;
> +            }
> +        }

nit: add empty line here.

Comment 19

[Chenxia Liu [:liuche]]

Attachment: Part 1: Remove Suggested Site v3

Comment 20

[Chenxia Liu [:liuche]]

Attachment: Part 2: Tests

Carrying over r+.

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 8438777 [details] [diff] [review]
Part 1: Remove Suggested Site v3

Review of attachment 8438777 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with the suggested changes.

::: mobile/android/base/db/SuggestedSites.java
@@ +283,5 @@
> +
> +        return blacklist;
> +    }
> +
> +    private List<String> includeBlacklist(List<String> origList) {

nit: origList -> originalList

@@ +316,5 @@
> +
> +        if (cachedSites == null) {
> +            refresh();
> +            if (cachedSites == null) {
> +                return false;

If this happens, it's probably a bug, no? Log a warning at least?

@@ +326,5 @@
> +                cachedBlacklist = loadBlacklist();
> +            }
> +
> +            // Check if site has already been blacklisted, just in case.
> +            if (!cachedBlacklist.contains(url)) {

This would be more efficient if cachedBlacklist was a HashMap instead.

@@ +331,5 @@
> +
> +                final SharedPreferences prefs = GeckoSharedPrefs.forProfile(context);
> +                final String prefString = prefs.getString(PREF_SUGGESTED_SITES_HIDDEN, "");
> +                final String siteString = prefString.concat(" " + Uri.encode(url));
> +                prefs.edit().putString(PREF_SUGGESTED_SITES_HIDDEN, siteString).commit();

nit: factor out this code into a separate saveBlacklistedSite() private method or something.

Comment 22

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/8e2465ccc4e4
https://hg.mozilla.org/integration/fx-team/rev/f79cfc838430

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e2465ccc4e4
https://hg.mozilla.org/mozilla-central/rev/f79cfc838430

Comment 24

[Lucas Rocha (:lucasr)]

liuche, please request uplift to Aurora as this feature should be in Fx32.

Comment 25

[Chenxia Liu [:liuche]]

Comment on attachment 8438777 [details] [diff] [review]
Part 1: Remove Suggested Site v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 913457
User impact if declined: Users will not be able to remove Suggested Sites from their Top Sites panel without visiting the page
Testing completed (on m-c, etc.): local testing, landed in nightly
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 26

[Chenxia Liu [:liuche]]

Comment on attachment 8438778 [details] [diff] [review]
Part 2: Tests

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 913457
User impact if declined: tests
Testing completed (on m-c, etc.): local testing, landed in nightly
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 27

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7037d7df546c
https://hg.mozilla.org/releases/mozilla-aurora/rev/133907276db1

Comment 28

[Chenxia Liu [:liuche]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=e7bb10c8d78b

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Chenxia, could you propose a wording for the release notes? Thanks

Comment 30

[Chenxia Liu [:liuche]]

Sure. Let me clarify though, I don't think this should be considered a separate "feature" that needs a note, because it's an implicit part of the "Suggested Sites" feature - we wouldn't want to surface site suggestions without providing a way to remove the suggestions.

If we'd still like to include something though, a short note after the Suggested Sites section "<description of suggested sites>...; these can be removed by the user."

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Ok. Won't be part of the release notes then! Thanks