1184582 - Group top sites tiles by domain

Status: RESOLVED FIXED

(Firefox for iOS :: Home screen, defect)

Description

[Wesley Johnston (:wesj)]

There's some thought that we should group the tiles by domain (i.e. no one needs four facebook top sites?). We can partition in memory pretty easily (adding a host column to the db also wouldn't be that hard).

Someone also mentioned maybe bringing back the rows below the tiles and pushing "secondary" sites for a particular domain down there.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: PR https://github.com/mozilla/firefox-ios/pull/756

Comment 2

[Robin Andersen [:tecgirl]]

(In reply to Wesley Johnston (:wesj) from comment #1)
> Created attachment 8634755 [details] [review]
> PR https://github.com/mozilla/firefox-ios/pull/756

Looks good!

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 8634755 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/756

Padding out tests here a bit, but what do you think rnewman?

Comment 4

[Richard Newman [:rnewman]]

Too much risk for v1.

Comment 5

[Wesley Johnston (:wesj)]

I pretty strongly disagree with this since its such common feedback (both here and Android). Maybe we move forward with a simpler solution for now? i.e. partition in memory and deal better with deletions in the future.

Comment 6

[Richard Newman [:rnewman]]

Apologies for the brief triage comment; we were running through the list.

Acknowledged that feedback on top sites is generally poor for this and other reasons.

At this stage -- weeks from ship -- I want to avoid us spending a lot of time iterating on either the UX (based on feedback) or storage (for perf), because we just don't have the time and can't afford the risk or the time spent on backing out stuff that didn't pan out.

We have a big enough backlog that having this slip by two or four weeks doesn't seem like the end of the world, even though what we ship in v1 will be far from perfect.

This _could_ make v1… _if_ we can scope it right down _and_ make sure we're not throwing the baby out with the bathwater.

However, I'm not totally convinced that we can scope grouping/clustering down without taking on risks we don't understand: e.g., partitioning in memory means fetching more things from the DB and hoping for the best (likely bad news with data in the wild, and of course a perf hit). And yes, we'd probably need to turn off deletion of top sites, which will bring its own negative feedback in the absence of Bug 1186010. Or we'll ship a confusing or surprising experience by making some decision about what to delete.

My conclusion, then: if other simple top sites improvements like Bug 1184767 make top sites less painful (and it should, because no Gmail message or Google Maps link or individual bug should outweigh any of the pages you've visited locally) then I think the risk/reward for trying to get this into v1 doesn't make sense.

Comment 7

[Richard Newman [:rnewman]]

Robin: can we find some time this week to talk through the experience here?

Comment 8

[Robin Andersen [:tecgirl]]

(In reply to Richard Newman [:rnewman] from comment #7)
> Robin: can we find some time this week to talk through the experience here?

Meeting invite sent.

Comment 9

[Richard Newman [:rnewman]]

Conclusion from meeting:

Part of this is scope/goal, and part of it is pitch.

If we're pitching top sites as suggestions, then it matters less that they're directly entries from your history. In a sense they're computed/suggested bookmarks, not a slice of your history.

The other part is that we really want to solve the "m." problem, which was one of the big motivations for filing Bug 934030 in the first place.


So we're going to do two things:

* Make sure that deletions don't actually delete anything from history. Deletion doesn't make sense at this stage. That might involve adding a domains table so that we have a place to mark a 'hide' flag. I'll file that bug now.

* Not just group on domain, but also eliminate www, m, and whatever prefixes desktop ignores in its newtab page.


We'll also have to keep an eye on the wording and user feedback to make sure we're making people happy.


This bug has a few details in it that are tricky, like grabbing the first/best/right favicon, excluding hidden top sites, and keeping within an acceptable perf envelope. As such we accept that this might slip from v1. But if it's done in time, without huge risk, then we'll take it: top sites is front-and-center and a big part of our perceived quality.


Separately, we need to figure out where we want to go with top sites: towards curation, navigation/hierarchy, prediction, or suggestion. That's a much bigger conversation that Karen, Robin and I will have over wine some time.

Comment 10

[Richard Newman [:rnewman]]

Links from Wes for desktop's logic:

https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/NewTabUtils.jsm#1364
https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/NewTabUtils.jsm#888

Comment 11

[Richard Newman [:rnewman]]

First pass review comments left, Wes. Thanks for tackling the growing scope!

Comment 13

[Wesley Johnston (:wesj)]

Updated the PR, but had some questions about comments.

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8634755 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/756

General approach is sound, but I think there are some bugs introduced by the refactor.

Comment 15

[Wesley Johnston (:wesj)]

I did! Updated. For the local insert case we actually have a connection we reuse a bunch (i.e. try to update with it, if that fails try to insert). Making that work with the remote stuff was going to require lots of moving, so I just reverted back to what you had originally.

Comment 16

[Wesley Johnston (:wesj)]

Updated this a bit more again. It will now upgrade old users to the new db a little better.

Comment 17

[Richard Newman [:rnewman]]

I'm wrapping this up.

Comment 18

[Richard Newman [:rnewman]]

Urgh, this breaks a few tests, like ClearPrivateDataTests. Might be a tough landing.

Comment 19

[Richard Newman [:rnewman]]

Wes: the approach I plan to take for a less-expensive way to do the domain upgrade without adding function support:

* Query for unique URLs. (One query.)
* Domainify into pairs in memory.
* Insert into a temporary table with two columns: URL -> domain. We can do this 500 at a time by query substitution into a multi-row insert. We'll do this within a transaction. That's ten queries for a large Sync user.
* Update the history table to join against that table in a single UPDATE.

Comment 20

[Richard Newman [:rnewman]]

This also breaks our frecency perf test, because now we default to fetching icons, which is WAY more expensive:

EXPLAIN QUERY PLAN
SELECT history.id AS historyID, history.url AS url, title, guid, 
COALESCE(max(case visits.is_local when 1 then visits.date else 0 end), 0) AS localVisitDate, 
COALESCE(max(case visits.is_local when 0 then visits.date else 0 end), 0) AS remoteVisitDate, 
COALESCE(sum(visits.is_local), 0) AS localVisitCount, 
COALESCE(sum(case visits.is_local when 1 then 0 else 1 end), 0) AS remoteVisitCount 
FROM history INNER JOIN visits ON visits.siteID = history.id 
WHERE history.is_deleted = 0
GROUP BY history.id 
ORDER BY localVisitCount * 5 * max(1, 100 * 225 / (((1437144513605000 - (localVisitDate)) / 86400000000) * ((1437144513605000 - (localVisitDate)) / 86400000000) + 225))
LIMIT 10;

=>

0           0           0           SCAN TABLE history
0           1           1           SEARCH TABLE visits USING COVERING INDEX idx_visits_siteID_is_local_date (siteID=?)
0           0           0           USE TEMP B-TREE FOR ORDER BY


versus

EXPLAIN QUERY PLAN
SELECT historyID, url, title, guid, localVisitDate, remoteVisitDate, localVisitCount, remoteVisitCount, iconID, iconURL, iconDate, iconType, iconWidth
FROM (
SELECT history.id AS historyID, history.url AS url, title, guid,
COALESCE(max(case visits.is_local when 1 then visits.date else 0 end), 0) AS localVisitDate,
COALESCE(max(case visits.is_local when 0 then visits.date else 0 end), 0) AS remoteVisitDate,
COALESCE(sum(visits.is_local), 0) AS localVisitCount,
COALESCE(sum(case visits.is_local when 1 then 0 else 1 end), 0) AS remoteVisitCount
FROM history INNER JOIN visits ON visits.siteID = history.id INNER JOIN domains ON domains.id = history.domain_id
WHERE (history.is_deleted = 0) AND (domains.showOnTopSites IS 1)
GROUP BY history.domain_id
ORDER BY (localVisitCount * (5 + localVisitCount)) * max(1, 100 * 225 / (((1438225597320207 - (localVisitDate)) / 86400000000.0) * ((1438225597320207 - (localVisitDate)) / 86400000000.0) + 225)) + remoteVisitCount * max(1, 100 * 225 / (((1438225597320224 - (remoteVisitDate)) / 86400000000.0) * ((1438225597320224 - (remoteVisitDate)) / 86400000000.0) + 225)) DESC LIMIT 10 )
LEFT OUTER JOIN view_history_id_favicon ON historyID = view_history_id_favicon.id;


=>

1           0           1           SCAN TABLE visits USING COVERING INDEX idx_visits_siteID_is_local_date
1           1           0           SEARCH TABLE history USING INTEGER PRIMARY KEY (rowid=?)
1           2           2           SEARCH TABLE domains USING INTEGER PRIMARY KEY (rowid=?)
1           0           0           USE TEMP B-TREE FOR GROUP BY
1           0           0           USE TEMP B-TREE FOR ORDER BY
3           0           0           SCAN TABLE favicon_sites USING COVERING INDEX sqlite_autoindex_favicon_sites_1
3           1           1           SEARCH TABLE favicons USING INTEGER PRIMARY KEY (rowid=?)
2           0           0           SCAN TABLE history USING COVERING INDEX sqlite_autoindex_history_2
2           1           1           SEARCH SUBQUERY 3 USING AUTOMATIC COVERING INDEX (siteID=?)
0           0           0           SCAN SUBQUERY 1
0           1           1           SCAN SUBQUERY 2

Comment 21

[Richard Newman [:rnewman]]

Even with icons removed we're taking twice as long. That extra join (even when CROSS JOIN is used to get history first) and the grouping is pretty painful.

Comment 22

[Richard Newman [:rnewman]]

I managed to get this down to an 18% regression, which isn't bad considering the extra work being done (and some of that will be logging overhead).

My PR is up to date:

https://github.com/mozilla/firefox-ios/pull/831

I'll tidy that, record a new baseline, and request review from Karim and Steph in the morning.

Comment 23

[Richard Newman [:rnewman]]

Attachment: Pull req.

Comment 24

[Richard Newman [:rnewman]]

A UI question: where there's no page title, should we show the domain instead of the URL?

Right now my top sites show

   [ Twitter ]   [ mozilla/firefox-ios ]  [http://facebook.c…]

so that would change to be
 
                                          [ facebook.com ]


Tapping the tile still takes you to the winning individual history item, not the domain.

Comment 25

[Stephan Leroux [:sleroux]]

Comment on attachment 8641067 [details] [review]
Pull req.

Work looks good to me and works well. Just left a comment for curiosity.

Comment 26

[Robin Andersen [:tecgirl]]

(In reply to Richard Newman [:rnewman] from comment #24)
> A UI question: where there's no page title, should we show the domain
> instead of the URL?
> 
> Right now my top sites show
> 
>    [ Twitter ]   [ mozilla/firefox-ios ]  [http://facebook.c…]
> 
> so that would change to be
>  
>                                           [ facebook.com ]
> 
> 
> Tapping the tile still takes you to the winning individual history item, not
> the domain.

Yes, definitely strip out the protocol and display the domain, with as much directory as possible with truncation.

Comment 27

[Robin Andersen [:tecgirl]]

Comment on attachment 8641067 [details] [review]
Pull req.

Looks good.

Comment 28

[Richard Newman [:rnewman]]

> Yes, definitely strip out the protocol and display the domain, with as much
> directory as possible with truncation.

Filed Bug 1189844.

Comment 29

[Richard Newman [:rnewman]]

*crosses fingers*

https://github.com/mozilla/firefox-ios/commit/74c5e97e3891b0cc17539412ff3d475c964ce90e

I'll be around today for any fallout.