Closed Bug 1298783 Opened 8 years ago Closed 8 years ago

AS highlights: Implement block list

Categories

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

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: sebastian, Assigned: ahunt)

References

Details

(Whiteboard: [MobileAS])

Attachments

(6 files)

Bug 1298783 - Add highlights blocklist table
58 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1298783 - Exclude blocklist sites from highlights query
58 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1298783 - Implement adding pages to highlights blocklist
58 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1298783 - Pre: add notificationUri to highlights query
58 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1298783 - Implement expiry for activity stream blocklist
58 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1298783 - Add activity stream blocklist tests
58 bytes, text/x-review-board-request
Grisha
: review+
Details
In bug 1293710 we added a basic query for loading "highlights" from the database. The desktop version of AS allows you to hide some highlights. For that a blocklist of URLs is kept and used as a filter in the highlights wuery.
Whiteboard: [MobileAS]
I'll take this: this will probably be useful for the highlights context menu in Bug 1300144.
Assignee: nobody → ahunt
My initial approach is to have a separate table just for the blocklist. I wonder if using a whole table for this is overkill, alternatives might be: - Use a list stored on disk: this would be complicated to use in the getHighlights query. - Use the annotations table: then we could just use "url NOT IN (SELECT url FROM annotations_table WHERE key = highlights_blocklist)", although that is likely to be a bit slower than a standalone table. The separate table is really simple to maintain, and probably the fastest option, so maybe we should just stick with that.
Sounds fine to me. "Overkill" is bad second-guessing: * You have the database open already. * You're not going to write a better disk persistence layer than SQLite's. * You're not really going to get a more compact representation than SQLite. * You want to routinely join against this.
Priority: P3 → P1
Status: NEW → ASSIGNED
Attachment #8789618 - Flags: review?(gkruglov)
Attachment #8789619 - Flags: review?(gkruglov)
Attachment #8789620 - Flags: review?(gkruglov)
Attachment #8789621 - Flags: review?(gkruglov)
Some questions/comments on this: - Do we want to maintain the blocklist forever? You've swiped away a highlight ("blocked" it) half a year ago - not sure what that UX looks like. Should this be grounds for _never_ showing that URL as a highlight, or just for some time? We might consider storing a NOW timestamp, and limiting the blocklist selection by some timeframe. What does desktop A-S do here? - Additionally, if the above logic applies, it might make sense to clean up that table every now and then. - You're only filtering history highlights (not bookmarks). Is that intentional? If so, can we store history IDs instead of the URL in that table? That's a bit more work on a write (you'll need to map URL to history ID), but less space will be used and I think lookups will be a bit faster. History.URL _should_ be unique... Or perhaps that should be GUID instead, to play nicely with sync, coupled with ON UPDATE CASCADE and ON DELETE CASCADE (although this might make sync operations a tad slower). - HighlightsBlocklist should be cleared when clearing data. If you use cascaded deletes above, that will happen automatically. Otherwise you need to probably piggy back clearing on top of "Sanitize:ClearHistory" call, handled here - https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#1508
See Also: → 1303731
I'm going to modify this on the basis of comment #8. I have a feeling that content from 6 months ago is unlikely to reappear (but I need to verify our highlights algorithm to confirm this - and even then, the algorithm could change). Hence, losing sites "dismissed" 6 months ago (or some other timeframe) is not a problem. In that case, a timestamp, and automatic deletion of outdated dismissed sites, is probably good enough. (I'll also need to add deletion of the list on data-clearing, and filtering of bookmarks too.)
A quick summary of my current approach: - cap the highlights blocklist at 1/5 the history limit (default is 2000, so max 400 blocked items). This factor might need adjusting, I'll file a bug to have telemetry for usage of the dismiss button which should let us know if this is needed. - piggyback on top of history expiration to clear all older items FWIW desktop seems to currently _never_ expire blocklist items. They also have no UI to bring back blocked items (but they do have the plumbing to remove items from the blocklist), although I can imagine that being added in the future. By adding expiration we can at least make sure we don't have an infintely growing list of blocked items, but I don't think we need to worry about the UX aspects of restoring blocked items yet.
Comment on attachment 8789618 [details] Bug 1298783 - Add highlights blocklist table https://reviewboard.mozilla.org/r/77772/#review77690 Re-flag me once you push up the updated version of this. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:618 (Diff revision 1) > private SuggestedSites() {} > > public static final Uri CONTENT_URI = Uri.withAppendedPath(AUTHORITY_URI, "suggestedsites"); > } > > + public static final class HighlightsBlocklist implements CommonColumns, URLColumns { You're not using the title field (defined in URLColumns), so probably don't implement that class at all and just define the URL field here excplicitely.
Attachment #8789618 - Flags: review?(gkruglov)
Comment on attachment 8789619 [details] Bug 1298783 - Exclude blocklist sites from highlights query https://reviewboard.mozilla.org/r/77774/#review78890
Attachment #8789619 - Flags: review?(gkruglov)
Comment on attachment 8789620 [details] Bug 1298783 - Implement adding pages to highlights blocklist https://reviewboard.mozilla.org/r/77776/#review78892
Attachment #8789620 - Flags: review?(gkruglov)
Comment on attachment 8789621 [details] Bug 1298783 - Pre: add notificationUri to highlights query https://reviewboard.mozilla.org/r/77778/#review78894
Attachment #8789621 - Flags: review?(gkruglov)
(In reply to Andrzej Hunt :ahunt from comment #10) > A quick summary of my current approach: > > - cap the highlights blocklist at 1/5 the history limit (default is 2000, so > max 400 blocked items). This factor might need adjusting, I'll file a bug to > have telemetry for usage of the dismiss button which should let us know if > this is needed. > - piggyback on top of history expiration to clear all older items > > FWIW desktop seems to currently _never_ expire blocklist items. They also > have no UI to bring back blocked items (but they do have the plumbing to > remove items from the blocklist), although I can imagine that being added in > the future. By adding expiration we can at least make sure we don't have an > infintely growing list of blocked items, but I don't think we need to worry > about the UX aspects of restoring blocked items yet. I think this sounds reasonable. Agreed on UX. Note that history limits are likely to change sometime soon (hopefully greatly increase), once local sync buffering stuff is in place. (P.S.: ugh, sorry about bugspam. Reviewboard confuses me sometimes.)
I implemented expiry based purely on date of creation, however I've realised that could be an issue for topsites : my latest patches use the blocklist for both topsites, and higlights - as far as I can tell this is what desktop does, but I'm not 100% sure that's desired behaviour. This would be an issue if you: 1. Dismiss a topsite (implied: this site is frequently visited) 2. Over a period of time, dismiss various other sites 3. URL of the site removed in 1 expires, is removed from blocklist 4. That topsite reappears (if it is still frequently visited) The solutions here might be: - (A) Don't use the blocklist for topsites (which is what we currently do: removing a topsite just removes it's history): this still results in the topsite cyclically reappearing if it's visited often enough - (B) Use a separate topsites blocklist: this results in the URL never reappearing, unless we add a UI for managing blocked sites (expiry of this list can be based on the site still being in history) - (C) Only expire blocklist pages that aren't a topsite, in addition to being the oldest items. Or alternatively, only expire blocklist pages that aren't in history or bookmarks anymore (in which case they won't reappear). This runs into the same issues as (B) wrt bringing items back from dismissal. I'm tempted to just stick with (A) for now, and *if* it's a big enough issue we can switch to B/C and implement the UI to bring sites back. That said, desktop appears to dismiss sites forever (with no UI).
Are you also going to push patches to clean this new table up during clearing of of history? See my note on this in Comment 8 (Sanitize:ClearHistory stuff).
Comment on attachment 8793518 [details] Bug 1298783 - Implement expiry for activity stream blocklist https://reviewboard.mozilla.org/r/80230/#review79296
Attachment #8793518 - Flags: review?(gkruglov) → review+
Comment on attachment 8789620 [details] Bug 1298783 - Implement adding pages to highlights blocklist https://reviewboard.mozilla.org/r/77776/#review79298
Attachment #8789620 - Flags: review?(gkruglov) → review+
Comment on attachment 8789618 [details] Bug 1298783 - Add highlights blocklist table https://reviewboard.mozilla.org/r/77772/#review79300 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:911 (Diff revision 2) > + debug("Creating " + ActivityStreamBlocklist.TABLE_NAME + " table"); > + > + db.execSQL("CREATE TABLE " + ActivityStreamBlocklist.TABLE_NAME + "(" + > + ActivityStreamBlocklist._ID + " INTEGER PRIMARY KEY AUTOINCREMENT, " + > + ActivityStreamBlocklist.URL + " TEXT UNIQUE NOT NULL, " + > + ActivityStreamBlocklist.CREATED + " INTEGER NOT NULL)"); Do you think we'll need any indecies on these fields? We're ordering by CREATED during expiration, and it's possible we'll query by URL (although we don't currently). Now will be the perfect time to add them. Perhaps just the CREATED one? Not sure if the overhead is worth it though.
Attachment #8789618 - Flags: review?(gkruglov)
Comment on attachment 8789618 [details] Bug 1298783 - Add highlights blocklist table https://reviewboard.mozilla.org/r/77772/#review79304
Attachment #8789618 - Flags: review+
Comment on attachment 8789619 [details] Bug 1298783 - Exclude blocklist sites from highlights query https://reviewboard.mozilla.org/r/77774/#review79306 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:835 (Diff revision 2) > DBUtils.qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " < " + Bookmarks.FIXED_ROOT_ID + " AND " + > DBUtils.qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + " == 0)" + > " AND " + > - "(" + Combined.URL + " NOT LIKE ?)"; > + "(" + Combined.URL + " NOT LIKE ?)" + > + " AND " + Combined.URL + " NOT IN " + > + "(SELECT " + ActivityStreamBlocklist.URL + " FROM " + TABLE_ACTIVITY_STREAM_BLOCKLIST + ")"; Agreed with that we don't want to filter topsites by the same blocklist. From user's POV, it seems like a pretty weird interaction.
Attachment #8789619 - Flags: review?(gkruglov) → review+
Comment on attachment 8793519 [details] Bug 1298783 - Add activity stream blocklist tests https://reviewboard.mozilla.org/r/80232/#review79544 ::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHighlightsTest.java:350 (Diff revision 1) > + Assert.assertEquals(0, cursor.getCount()); > + cursor.close(); > + } > + > + // Add 2000 / 10 items in the loop? > + int itemsNeeded = BrowserProvider.DEFAULT_EXPIRY_RETAIN_COUNT / BrowserProvider.ACTIVITYSTREAM_BLOCKLIST_EXPIRY_FACTOR; Can you expand this test to ensure we don't expire too much? e.g. something like ensuring that 2001st url isn't expired, and still functions as a block for highlights.
Attachment #8793519 - Flags: review?(gkruglov) → review+
Comment on attachment 8789621 [details] Bug 1298783 - Pre: add notificationUri to highlights query https://reviewboard.mozilla.org/r/77778/#review79546
Attachment #8789621 - Flags: review?(gkruglov) → review+
Comment on attachment 8789618 [details] Bug 1298783 - Add highlights blocklist table https://reviewboard.mozilla.org/r/77772/#review79548 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:911 (Diff revision 2) > + debug("Creating " + ActivityStreamBlocklist.TABLE_NAME + " table"); > + > + db.execSQL("CREATE TABLE " + ActivityStreamBlocklist.TABLE_NAME + "(" + > + ActivityStreamBlocklist._ID + " INTEGER PRIMARY KEY AUTOINCREMENT, " + > + ActivityStreamBlocklist.URL + " TEXT UNIQUE NOT NULL, " + > + ActivityStreamBlocklist.CREATED + " INTEGER NOT NULL)"); Since you marked URL as unique (good), that will create a unique index on that column; that's one index down :-)
Pushed by ahunt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d05947f7d956 Add highlights blocklist table r=Grisha https://hg.mozilla.org/integration/autoland/rev/7945daf16a50 Exclude blocklist sites from highlights query r=Grisha https://hg.mozilla.org/integration/autoland/rev/0da42b264e17 Implement adding pages to highlights blocklist r=Grisha https://hg.mozilla.org/integration/autoland/rev/818fa3036aef Implement expiry for activity stream blocklist r=Grisha https://hg.mozilla.org/integration/autoland/rev/1faeb0e59cad Add activity stream blocklist tests r=Grisha https://hg.mozilla.org/integration/autoland/rev/0b9c9000ac93 Pre: add notificationUri to highlights query r=Grisha
Iteration: --- → 1.5
Blocks: 1314718
Blocks: 1319231
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: