1298785 - AS highlights: Group history by hostname

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 groups history by hostname to avoid showing multiple highlights for the same domain. It is using the 'rev_host' (reversed hostname) column for that. Such a column does not exist in the mobile database.

Comment 1

[Richard Newman [:rnewman]]

See also Bug 1237038 (Android), Bug 1184582 (iOS).

Comment 2

[Andrzej Hunt :ahunt]

(Based on IRC discussion) My current plan here is to follow the iOS example of adding a domain_id column to the history table + a domains table containing the actual domains, which should allow for simple grouping as needed. (We'll probably need to add the same for bookmarks, since we also query unvisited bookmarks in the highlights query.)

Comment 3

[Andrzej Hunt :ahunt]

Attachment: Bug 1298785 - Group highlights by domain

We want to group by domain when showing highlights (and possibly topsites too?).
Both bookmarks and history are used for generating highlights so we need to update
both tables.

TODO: we might want to add domain_id to the combined view, however currently we probably
wouldn't need to use that anywhere (topsites however does use combined, so we'd need
this change for topsites). Modifying combined now would help us avoid another migration step.

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

Comment 4

[Andrzej Hunt :ahunt]

I've got a very simplistic implementation to prevent multiple URLs from one domain. As far as I can tell, in the case of multiple URLs for a domain this would result in prioritising a bookmarked URL, followed by the most recently visited page for a given domain. I'm not sure what the longer term plans for highlights-generation are though so this might be good enough for now (especially if we end up using the weighted highlights algorithm from desktop, in which case we'd rewrite the queries anyways)?

The actual domain/host handling code is being added in Bug 1319485.

Comment 5

[Andrzej Hunt :ahunt]

Comment on attachment 8809988 [details]
Bug 1298785 - Group highlights by domain

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92462/diff/1-2/

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8809988 [details]
Bug 1298785 - Group highlights by domain

https://reviewboard.mozilla.org/r/92462/#review95240

This is based on the domain table - is this already reviewed/landed?

The desktop add-on actually groups by rev_host one level higher:
https://github.com/mozilla/activity-stream/blob/9eb9f451b553bb62ae9b8d6b41a8ef94a2e020ea/addon/PlacesProvider.js#L578

Comment 7

[Andrzej Hunt :ahunt]

(In reply to Sebastian Kaspari (:sebastian) from comment #6)
> Comment on attachment 8809988 [details]
> Bug 1298785 - Group highlights by domain
> 
> https://reviewboard.mozilla.org/r/92462/#review95240
> 
> This is based on the domain table - is this already reviewed/landed?
> 
> The desktop add-on actually groups by rev_host one level higher:
> https://github.com/mozilla/activity-stream/blob/
> 9eb9f451b553bb62ae9b8d6b41a8ef94a2e020ea/addon/PlacesProvider.js#L578
I'm still working on the domain table (Bug 1319485). It's mostly done, but waiting for review, and I still need to add a few more tests to ensure everything is working as expected.

I.e. it's going to be at least a few more days before the domain table lands.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

This won't be needed anymore: With the new algorithm (bug 1312016) we explicitly want to select items with the same host, rank them and then remove the ones that have a low score. So we won't need to filter them as part of the query.