Closed Bug 1010266 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 33
Tracking Status
firefox32 --- fixed
firefox33 --- verified
relnote-firefox --- -
fennec 32+ ---

People

(Reporter: liuche, Assigned: liuche)

References

Details

Attachments

(2 files, 5 obsolete files)

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.
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.
Flags: needinfo?(ibarlow)
Assignee: nobody → liuche
(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?
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)
(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.
Flags: needinfo?(ibarlow)
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.
Flags: needinfo?(lucasr.at.mozilla)
(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?
Flags: needinfo?(lucasr.at.mozilla)
Blocks: 1016363
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.
tracking-fennec: --- → ?
Duplicate of this bug: 1016363
Summary: Add a "hide suggested sites" item to the context menu for a suggested site → Blacklist Top Sites suggestions that have been "removed" by user
tracking-fennec: ? → 32+
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!
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?
Attachment #8432926 - Flags: feedback?(lucasr.at.mozilla)
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.
Attachment #8432926 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Attached patch Part 1: Remove Suggested Site (obsolete) — Splinter Review
Attachment #8432926 - Attachment is obsolete: true
Attached patch Part 1: Remove Suggested Site (obsolete) — Splinter Review
Attachment #8433735 - Attachment is obsolete: true
Attachment #8435476 - Flags: review?(lucasr.at.mozilla)
Attached patch Part 2: Tests (obsolete) — Splinter Review
Attachment #8435477 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8435477 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #8435476 - Flags: review?(lucasr.at.mozilla) → feedback+
Attached patch Part 1: Remove Suggested Site v2 (obsolete) — Splinter Review
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?
Attachment #8435476 - Attachment is obsolete: true
Attachment #8438211 - Flags: feedback?(lucasr.at.mozilla)
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.
Attachment #8438211 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Attachment #8438211 - Attachment is obsolete: true
Attachment #8438777 - Flags: review?(lucasr.at.mozilla)
Attached patch Part 2: TestsSplinter Review
Carrying over r+.
Attachment #8435477 - Attachment is obsolete: true
Attachment #8438778 - Flags: review+
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.
Attachment #8438777 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/8e2465ccc4e4
https://hg.mozilla.org/mozilla-central/rev/f79cfc838430
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
liuche, please request uplift to Aurora as this feature should be in Fx32.
Flags: needinfo?(liuche)
Status: RESOLVED → VERIFIED
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap?(fennec)
Flags: in-moztrap?(fennec) → in-moztrap?(ioana.chiorean)
QA Contact: ioana.chiorean
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
Attachment #8438777 - Flags: approval-mozilla-aurora?
Flags: needinfo?(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
Attachment #8438778 - Flags: approval-mozilla-aurora?
Attachment #8438777 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8438778 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Chenxia, could you propose a wording for the release notes? Thanks
Flags: needinfo?(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."
Flags: needinfo?(liuche)
Ok. Won't be part of the release notes then! Thanks
You need to log in before you can comment on or make changes to this bug.