The TopSites item title in iOS looks like [1]: if let provider = site.metadata?.providerName { titleLabel.text = provider.lowercased() } else { titleLabel.text = site.tileURL.hostSLD } In bug 1377287, we only implemented the latter, the hostSLD. The provider is stored in the database so, because it's a lot of extra work and I'm stumbling through it, I figured it'd be more efficient to break it out into a separate bug. Also, it may relate to bug 1369604 (highlights query is slow) because suddenly we'll have to join the page metadata table with the top sites table. [1]: https://github.com/mozilla-mobile/firefox-ios/blob/deb9736c905cdf06822ecc4a20152df7b342925d/Client/Frontend/Home/ActivityStreamTopSitesCell.swift#L147

I flagged this for triage, instead of fixing it this sprint (even though it's a follow-up) because it feels low priority to me, compared to the other work that we have to do. In practice, fixing this bug will make the top sites tiles' titles change like so: google -> accounts google mozilla -> bugzilla mozilla rockpapershotgun -> rock paper shotgun With favicons, this isn't too bad in most cases (e.g. bugzilla.mozilla.org would have "mozilla" as the title but the bugzilla favicon). Most providers also won't change at all: kotaku -> kotaku apple -> apple There will usually only be benefits for domains with subdomains (e.g. accounts.google.com) because it gives the website the opportunity to express the subdomain in the top sites titles. --- Though now that I've been digging through the DB, this might not be as hard as I initially thought it would be - performance may still be a concern, however and working on bug 1369604 first may give me more context.

This is borderline P2/P3 – we don't have a good sense of what the other P2 bugs are so let's compare it against them.

Not as simple as I thought I remembered that this was borderline P2/P3.

This provider is not as effective as iOS' algorithm: I filed bug 1386902. Review commit: https://reviewboard.mozilla.org/r/164158/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/164158/

Comment on attachment 8893156 [details] Bug 1382036: Use provider in top sites tile. https://reviewboard.mozilla.org/r/164158/#review170478 ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/TopSite.java:48 (Diff revision 1) > this.url = url; > this.title = title; > this.isBookmarked = isBookmarked; > this.isPinned = type == BrowserContract.TopSites.TYPE_PINNED; > this.type = type; > + this.metadata = metadata; Why did you decide to pass around metadata, rather than the provider? ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/TopSite.java:77 (Diff revision 1) > > public Boolean isPinned() { > return isPinned; > } > > + public Metadata getMetadata() { What kind of data is present in Metadata? On the other hand I understand that maybe you don't want to document that here though, because whoever changes Metadata won't necessarily change a comment here, and people can just go to the declaration of Metadata.

Comment on attachment 8893156 [details] Bug 1382036: Use provider in top sites tile. https://reviewboard.mozilla.org/r/164158/#review170478 > Why did you decide to pass around metadata, rather than the provider? I guess I didn't think about it very much. :P Thinking about it now, there can be an argument either way: 1) we have the data, why not expose it? or 2) Don't expose what you don't need. A plus for 1, in this case, is that we need to pull the data out of the DB anyway because it's a JSON string so it's more work to hide the data than to expose its data class. I'm going to leave it the way it is (expose the data we have) because the same pattern is used on Highlights items. > What kind of data is present in Metadata? > > On the other hand I understand that maybe you don't want to document that here though, because whoever changes Metadata won't necessarily change a comment here, and people can just go to the declaration of Metadata. Basically the content of the `PageMetadata` table: provider, imageUrl, descriptionLength. tbh, it's ambiguously named (yeah, what the hell is metadata anyway? Why is it a separate table from BrowserDB?) and perhaps I should have done better than following existing convention. As you mention, I don't think a comment would help here - I think it's best for devs to look at the definition of the class themselves.

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s a5ea44262c9b -d 845dd7955f8a: rebasing 411929:a5ea44262c9b "Bug 1382036: Use provider in top sites tile. r=liuche" (tip) merging mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/Highlight.java merging mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/Metadata.java warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/Highlight.java! (edit, then use 'hg resolve --mark') warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/Metadata.java! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by michael.l.comella@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5d6714d6cd74 Use provider in top sites tile. r=liuche

FYI: bug 1386902 is going to replace this. I won't back this out to avoid churn.