Closed Bug 1184582 Opened 5 years ago Closed 5 years ago

Group top sites tiles by domain

Categories

(Firefox for iOS :: Home screen, defect)

All
iOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fxios + ---

People

(Reporter: wesj, Assigned: rnewman)

References

Details

Attachments

(1 file, 1 obsolete file)

47 bytes, text/x-github-pull-request
sleroux
: review+
tecgirl
: ui-review+
Details | Review
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.
Attachment #8634755 - Flags: ui-review?(randersen)
tracking-fxios: --- → ?
(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 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?
Attachment #8634755 - Flags: feedback?(rnewman)
Too much risk for v1.
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.
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.
Component: General → Home screen
Robin: can we find some time this week to talk through the experience here?
Flags: needinfo?(randersen)
(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.
Flags: needinfo?(randersen)
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.
Assignee: nobody → wjohnston
Status: NEW → ASSIGNED
Hardware: Other → All
Depends on: 1187089
First pass review comments left, Wes. Thanks for tackling the growing scope!
Flags: needinfo?(wjohnston)
Duplicate of this bug: 1187089
Updated the PR, but had some questions about comments.
Flags: needinfo?(wjohnston)
Flags: needinfo?(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.
Flags: needinfo?(rnewman)
Attachment #8634755 - Flags: feedback?(rnewman) → feedback+
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.
Flags: needinfo?(rnewman)
Updated this a bit more again. It will now upgrade old users to the new db a little better.
I'm wrapping this up.
Assignee: wjohnston → rnewman
Flags: needinfo?(rnewman)
Urgh, this breaks a few tests, like ClearPrivateDataTests. Might be a tough landing.
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.
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
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.
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.
Attached file Pull req.
Attachment #8634755 - Attachment is obsolete: true
Attachment #8634755 - Flags: ui-review?(randersen)
Attachment #8641067 - Flags: review?(sleroux)
Attachment #8641067 - Flags: review?(kbenhmida)
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.
Flags: needinfo?(randersen)
Attachment #8641067 - Flags: ui-review?(randersen)
Comment on attachment 8641067 [details] [review]
Pull req.

Work looks good to me and works well. Just left a comment for curiosity.
Attachment #8641067 - Flags: review?(sleroux) → review+
(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.
Flags: needinfo?(randersen)
Comment on attachment 8641067 [details] [review]
Pull req.

Looks good.
Attachment #8641067 - Flags: ui-review?(randersen) → ui-review+
Blocks: 1189844
> Yes, definitely strip out the protocol and display the domain, with as much
> directory as possible with truncation.

Filed Bug 1189844.
*crosses fingers*

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

I'll be around today for any fallout.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #8641067 - Flags: review?(kbenhmida)
Duplicate of this bug: 1147493
You need to log in before you can comment on or make changes to this bug.