1298786 - AS highlights: Implement domain blacklist

Status: RESOLVED WONTFIX

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

In bug 1293710 we added a basic query for loading "highlights" from the database.

The desktop version ships a blacklist of domains[1] (e.g. mail.google.com). It is using the 'rev_host' (reversed hostname) column for filtering. Such a column does not exist in the mobile database. We could try implementing such a filter using NOT LIKE syntax, but we need to make sure that this does not have a negative impact on performance.

[1] https://github.com/mozilla/activity-stream/blob/9eb9f451b553bb62ae9b8d6b41a8ef94a2e020ea/addon/PlacesProvider.js#L37

Comment 1

[Andrzej Hunt :ahunt]

I imagine we can use the domain_id column (Bug 1319485) for this, I still need to think about the optimal approach though.

I wonder if we should add histogram telemetry to the highlights query, similar to what we use for the topsites panel? I might try to land that as a separate bug so we can get some data on existing performance first?

Comment 2

[Andrzej Hunt :ahunt]

One "fast" approach would be to modify the domains table to have a "blacklisted" field, and then only select sites where domain_id isn't blacklisted. That would however require extra fields in the domains table (it hasn't landed yet, so we can still make this change now...), and would possibly make management of the domains table more complicated (especially since we'll need to prepopulate it with the blacklisted domains).

A separate domain_blacklist table might be useful, especially if we want to enable "dismiss all sites from this domain" style functionality - that still needs prepopulating, but management should be easier (we'd still need to update the domain cleanup triggers to handle three tables, *if* we stick with the triggers).

Or we can just stuff the list of blacklisted domains into SQL. The topsites query essentially does this for suggested sites, so it's probably not too slow?

Comment 3

[Andrzej Hunt :ahunt]

Attachment: Bug 1298786 - Add hardcoded highlights domain blacklist

This is really minimal, and incomplete. We'd probably want a dynamic blacklist in future?

Review commit: https://reviewboard.mozilla.org/r/99560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99560/

Comment 4

[Andrzej Hunt :ahunt]

Comment on attachment 8820009 [details]
Bug 1298786 - Add hardcoded highlights domain blacklist

Cancelling review for now - the domains table is likely to change a bit more in Bug 1319485, so I might need to adjust the blacklist to match (i.e. the iOS domain extraction algorithm removes "www.", so we'll probably need to remove that in the blacklist too).

Comment 5

[Richard Newman [:rnewman]]

(In reply to Andrzej Hunt :ahunt from comment #2)
> One "fast" approach would be to modify the domains table…

Using the domains table for this will work so long as you never want to blacklist www.foo.com and allow m.foo.com.

Tread carefully: you might want to consider a different representation, like storing a fixed-size fast hash for a domain instead of using a table.