The current algorithm for fetching the title for a tile is based on an early Desktop algorithm, but the string doesn't make sense. We should copy the algorithm from Firefox iOS, because that's been tuned and works.

For a top sites item, the iOS algorithm is [1][2]: if let provider = site.metadata?.providerName { titleLabel.text = provider.lowercased() } else { titleLabel.text = site.tileURL.hostSLD } And the hostSLD algo is available at [2]. --- For highlights, there are three views: title, domain, and description [3]: self.domainLabel.text = site.tileURL.hostSLD self.titleLabel.text = site.title.characters.count <= 1 ? site.url : site.title if let bookmarked = site.bookmarked, bookmarked { self.descriptionLabel.text = "Bookmarked" ... } else { self.descriptionLabel.text = "Visited" [1]: https://github.com/mozilla-mobile/firefox-ios/blob/deb9736c905cdf06822ecc4a20152df7b342925d/Client/Frontend/Home/ActivityStreamTopSitesCell.swift#L147 [2]: https://github.com/mozilla-mobile/firefox-ios/blob/deb9736c905cdf06822ecc4a20152df7b342925d/Shared/Extensions/NSURLExtensions.swift#L152 [3]: https://github.com/mozilla-mobile/firefox-ios/blob/deb9736c905cdf06822ecc4a20152df7b342925d/Client/Frontend/Widgets/ActivityStreamHighlightCell.swift#L164

Note to self: these titles/labels are also used in the BottomSheet.

Partial review request: this exposes iOS' `NSUrl.hostSLD` functionality in Android. Still TODO: - Update the UI (including using ^ function) - Make it so PublicSuffix methods don't read from the disk every time they're called.

Comment on attachment 8887285 [details] Bug 1377287: Rename HighlightItem internal views to something easier. https://reviewboard.mozilla.org/r/158082/#review163524 I'm currently refactoring this UI. I agree with your change here. But I'd like to avoid having conflicts in this part of the code. Can we delay this change?

Comment on attachment 8887287 [details] Bug 1377287: Add URIUtils.getHostSecondLevelDomain and friends. https://reviewboard.mozilla.org/r/158086/#review163526 ::: mobile/android/base/java/org/mozilla/gecko/util/URIUtils.java:17 (Diff revision 1) > +/** Utilities for operating on URLs. */ > +public class URIUtils { > + private URIUtils() {} We have a bunch of those UR(I/L) related helpers in StringUtils currently. Should we move them (later)? ::: mobile/android/base/java/org/mozilla/gecko/util/URIUtils.java:58 (Diff revision 1) > + * https://github.com/mozilla-mobile/firefox-ios/blob/deb9736c905cdf06822ecc4a20152df7b342925d/Shared/Extensions/NSURLExtensions.swift#L243 > + * > + * @return the host without the prefixes "www.", "mobile.", or "m.", or null if there is no host. > + */ > + @Nullable > + public static String getNormalizedHost(@NonNull final URI uri) { I think we have such a helper in StringUtils too?

Comment on attachment 8887285 [details] Bug 1377287: Rename HighlightItem internal views to something easier. https://reviewboard.mozilla.org/r/158082/#review163524 > But I'd like to avoid having conflicts in this part of the code. Can we delay this change? I was expecting you to land your changes before I landed mine and I would deal with the refactoring but sure, I can pull it out of this patch series. I did this now because the names were *so* confusing that I couldn't understand the code enough to see what I needed to change. x_x

Comment on attachment 8887287 [details] Bug 1377287: Add URIUtils.getHostSecondLevelDomain and friends. https://reviewboard.mozilla.org/r/158086/#review163526 > We have a bunch of those UR(I/L) related helpers in StringUtils currently. Should we move them (later)? I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1381990.

> For a top sites item, the iOS algorithm is [1][2]: if let provider = site.metadata?.providerName { titleLabel.text = provider.lowercased() } else { titleLabel.text = site.tileURL.hostSLD } It looks like the site metadata is stored in the database, which we don't currently fetch for top sites. The DB is more work and I'm stumbling through the DB code so I broke it up into a separate bug 1382036.

The bottom sheet algo on iOS is [1]: self.titleLabel.text = site.title.characters.count <= 1 ? site.url : site.title self.descriptionLabel.text = site.tileURL.baseDomain [1]: ActionOverlayTableViewController.configureWithSite - GH is down atm so I can't grab the link

> - Make it so PublicSuffix methods don't read from the disk every time they're called. Nevermind, this isn't true: we read once and cache. I filed bug 1382072 to investigate how efficiently we care to handle this.

Comment on attachment 8887285 [details] Bug 1377287: Rename HighlightItem internal views to something easier. https://reviewboard.mozilla.org/r/158082/#review163524 That's okay too. I'll just r+ and you can decide whether you wait for the other patches (which might take some days, they are still in review) or land the other two commits now. :) [Or steal the review :D]

Comment on attachment 8887285 [details] Bug 1377287: Rename HighlightItem internal views to something easier. https://reviewboard.mozilla.org/r/158082/#review164112

I copied the UI formatting for the titles from Firefox for iOS but it looks like UI does not handle duplicates very well – see the attached screenshot. Brian/Aaron, how did you handle this for iOS? Or perhaps their algorithm hits upon fewer duplicates? Sebastian, wrt ^, perhaps we need to do better deduping? I assume that's not something we'd want to change for 57?

Also see bug 1382332 for tile dupes - probably continue the conversation in that bug rather than here.

Comment on attachment 8887287 [details] Bug 1377287: Add URIUtils.getHostSecondLevelDomain and friends. https://reviewboard.mozilla.org/r/158086/#review164374 ::: mobile/android/base/java/org/mozilla/gecko/util/URIUtils.java:41 (Diff revision 1) > + > + final URI uri = new URI(uriString); > + final String baseDomain = getBaseDomain(context, uri); > + if (baseDomain == null) { > + final String normalizedHost = getNormalizedHost(uri); > + return !TextUtils.isEmpty(normalizedHost) ? normalizedHost : uriString; If you return uriString, will this be of the form of a SLD? I know the iOS code does that, but maybe it's not totally correct.

Comment on attachment 8887286 [details] Bug 1377287: Add PublicSuffix.getPublicSuffix. https://reviewboard.mozilla.org/r/158084/#review164376 ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/publicsuffix/PublicSuffix.java:68 (Diff revision 1) > + @WorkerThread // This method might need to load data from disk > + public static String getPublicSuffix(@NonNull final Context context, @NonNull final String domain, final int additionalPartCount) { > + if (context == null) { throw new NullPointerException("Expected non-null Context argument"); } > + if (domain == null) { throw new NullPointerException("Expected non-null domain argument"); } > + > + if (additionalPartCount < 0) { Exception text should be >= 0 (to be consistent with check), unless 0 additionalPartCount is acceptable.

Comment on attachment 8888066 [details] Bug 1377287: Update highlights label algorithm to match FFiOS. https://reviewboard.mozilla.org/r/158966/#review166226 ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/HighlightItem.java:105 (Diff revision 1) > - pageTitleView.setText(highlight.getTitle()); > + final String backendHightlightTitle = highlight.getTitle(); > + final String uiHighlightTitle = !TextUtils.isEmpty(backendHightlightTitle) ? backendHightlightTitle : highlight.getUrl(); > + pageTitleView.setText(uiHighlightTitle); This is in the area where my refactoring is happening too. I really hope I can land that soon. :( ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/HighlightItem.java:169 (Diff revision 1) > + super(contextReference, uriString); > + this.pageDomainViewWeakReference = new WeakReference<>(pageDomainView); > - } > + } > > - @Override > + @Override > - public void onIconResponse(IconResponse response) { > + protected void onPostExecute(final String hostSLD) { I assume we somehow need to make sure that this view is still the right target for this hostSLD? Once onPostExecute() runs this view could already be recycled to show a different highlight (user is scrolling). Most image loader libraries will set a tag (setTag(), for image loaders usually the URL) on the view and only update the view in the callback if the tag is still the same.

Comment on attachment 8888067 [details] Bug 1377287: Update TopSitesCard label to iOS algorithm. https://reviewboard.mozilla.org/r/158968/#review166228

Comment on attachment 8888068 [details] Bug 1377287: Use FFiOS label algorithm for BottomSheetContextMenu. https://reviewboard.mozilla.org/r/158970/#review166232

Comment on attachment 8888069 [details] Bug 1377287: Rm now unused ActivityStream.extractLabel. https://reviewboard.mozilla.org/r/158972/#review166234

Comment on attachment 8887287 [details] Bug 1377287: Add URIUtils.getHostSecondLevelDomain and friends. https://reviewboard.mozilla.org/r/158086/#review164374 > If you return uriString, will this be of the form of a SLD? > > I know the iOS code does that, but maybe it's not totally correct. Good call - I added a review commit on top.

Comment on attachment 8887286 [details] Bug 1377287: Add PublicSuffix.getPublicSuffix. https://reviewboard.mozilla.org/r/158084/#review164376 > Exception text should be >= 0 (to be consistent with check), unless 0 additionalPartCount is acceptable. 0 additional parts is acceptable – this would return the PublicSuffix directly.

Comment on attachment 8888066 [details] Bug 1377287: Update highlights label algorithm to match FFiOS. https://reviewboard.mozilla.org/r/158966/#review166226 > I assume we somehow need to make sure that this view is still the right target for this hostSLD? Once onPostExecute() runs this view could already be recycled to show a different highlight (user is scrolling). Most image loader libraries will set a tag (setTag(), for image loaders usually the URL) on the view and only update the view in the callback if the tag is still the same. Good find! I did this & added a commit on top.

Comment on attachment 8890576 [details] Bug 1377287 - review: getHostSLD returns empty str on error. https://reviewboard.mozilla.org/r/161724/#review167062

Comment on attachment 8890577 [details] Bug 1377287 - review: Only update views if they're the same as when asyncTask began. https://reviewboard.mozilla.org/r/161726/#review167064 r=trivial for these two.

Pushed by michael.l.comella@gmail.com: https://hg.mozilla.org/integration/autoland/rev/f981c0d4269f Rename HighlightItem internal views to something easier. r=sebastian https://hg.mozilla.org/integration/autoland/rev/be3b083e82b8 Add PublicSuffix.getPublicSuffix. r=liuche https://hg.mozilla.org/integration/autoland/rev/e25baed29efd Add URIUtils.getHostSecondLevelDomain and friends. r=liuche https://hg.mozilla.org/integration/autoland/rev/cdc31f6fc084 Update highlights label algorithm to match FFiOS. r=sebastian https://hg.mozilla.org/integration/autoland/rev/e244d146570c Update TopSitesCard label to iOS algorithm. r=sebastian https://hg.mozilla.org/integration/autoland/rev/93c07d38f880 Use FFiOS label algorithm for BottomSheetContextMenu. r=sebastian https://hg.mozilla.org/integration/autoland/rev/4da81d3d8c65 Rm now unused ActivityStream.extractLabel. r=sebastian https://hg.mozilla.org/integration/autoland/rev/7a3e7c2caaf7 review: getHostSLD returns empty str on error. r=mcomella https://hg.mozilla.org/integration/autoland/rev/e677ddb05965 review: Only update views if they're the same as when asyncTask began. r=mcomella

https://hg.mozilla.org/mozilla-central/rev/f981c0d4269f https://hg.mozilla.org/mozilla-central/rev/be3b083e82b8 https://hg.mozilla.org/mozilla-central/rev/e25baed29efd https://hg.mozilla.org/mozilla-central/rev/cdc31f6fc084 https://hg.mozilla.org/mozilla-central/rev/e244d146570c https://hg.mozilla.org/mozilla-central/rev/93c07d38f880 https://hg.mozilla.org/mozilla-central/rev/4da81d3d8c65 https://hg.mozilla.org/mozilla-central/rev/7a3e7c2caaf7 https://hg.mozilla.org/mozilla-central/rev/e677ddb05965