1377287 - Activity Stream: Replace label algorithm with something that makes sense

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, enhancement, P1)

Comment 1

[Chenxia Liu [:liuche]]

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.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1377287: Rename HighlightItem internal views to something easier.

I could not make sense of the existing names at all.

Bug 1379021 is going to:
- Remove the vTimeSince view, so I didn't change the name
- Create a new layout file, so I didn't change the resource IDs accordingly

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

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1377287: Add PublicSuffix.getPublicSuffix.

This should not be run in AS yet because each time it's called, it reads from
the disk and even the tests on desktop take 3s+ to run.

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

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1377287: Add URIUtils.getHostSecondLevelDomain and friends.

This is used by the iOS implementation of AS to get the highlight titles.

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

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 8

[Sebastian Kaspari (:sebastian; :pocmo)]

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 9

[Sebastian Kaspari (:sebastian; :pocmo)]

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 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> 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.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> - 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 15

[Sebastian Kaspari (:sebastian; :pocmo)]

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 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8887285 [details]
Bug 1377287: Rename HighlightItem internal views to something easier.

https://reviewboard.mozilla.org/r/158082/#review164112

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Top site dupes (gizmodo)

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?

Comment 18

[Chenxia Liu [:liuche]]

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

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1377287: Update highlights label algorithm to match FFiOS.

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

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1377287: Update TopSitesCard label to iOS algorithm.

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

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1377287: Use FFiOS label algorithm for BottomSheetContextMenu.

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

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1377287: Rm now unused ActivityStream.extractLabel.

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

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8887285 [details]
Bug 1377287: Rename HighlightItem internal views to something easier.

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

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8887286 [details]
Bug 1377287: Add PublicSuffix.getPublicSuffix.

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

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8887287 [details]
Bug 1377287: Add URIUtils.getHostSecondLevelDomain and friends.

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

Comment 26

[Chenxia Liu [:liuche]]

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 27

[Chenxia Liu [:liuche]]

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 28

[Sebastian Kaspari (:sebastian; :pocmo)]

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 29

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8888067 [details]
Bug 1377287: Update TopSitesCard label to iOS algorithm.

https://reviewboard.mozilla.org/r/158968/#review166228

Comment 30

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8888068 [details]
Bug 1377287: Use FFiOS label algorithm for BottomSheetContextMenu.

https://reviewboard.mozilla.org/r/158970/#review166232

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8888069 [details]
Bug 1377287: Rm now unused ActivityStream.extractLabel.

https://reviewboard.mozilla.org/r/158972/#review166234

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1377287 - review: getHostSLD returns empty str on error.

In response to https://bugzilla.mozilla.org/show_bug.cgi?id=1377287#c26:
getHostSLD should only return hosts.

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

Comment 36

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1377287 - review: Only update views if they're the same as when asyncTask began.

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

Comment 37

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8887285 [details]
Bug 1377287: Rename HighlightItem internal views to something easier.

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

Comment 38

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8887286 [details]
Bug 1377287: Add PublicSuffix.getPublicSuffix.

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

Comment 39

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8887287 [details]
Bug 1377287: Add URIUtils.getHostSecondLevelDomain and friends.

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

Comment 40

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8888066 [details]
Bug 1377287: Update highlights label algorithm to match FFiOS.

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

Comment 41

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8888067 [details]
Bug 1377287: Update TopSitesCard label to iOS algorithm.

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

Comment 42

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8888068 [details]
Bug 1377287: Use FFiOS label algorithm for BottomSheetContextMenu.

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

Comment 43

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8888069 [details]
Bug 1377287: Rm now unused ActivityStream.extractLabel.

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

Comment 44

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8890576 [details]
Bug 1377287 - review: getHostSLD returns empty str on error.

https://reviewboard.mozilla.org/r/161724/#review167062

Comment 45

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
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

Comment 47

[Carsten Book [:Tomcat]]

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