Closed Bug 1377287 Opened 7 years ago Closed 7 years ago

Activity Stream: Replace label algorithm with something that makes sense

Categories

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

enhancement

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: sebastian, Assigned: mcomella)

References

Details

(Whiteboard: [MobileAS] 1.26)

Attachments

(10 files)

59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
liuche
: review+
Details
59 bytes, text/x-review-board-request
liuche
: review+
Details
80.72 KB, image/png
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
      No description provided.
Priority: -- → P2
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.
Assignee: nobody → michael.l.comella
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?
Attachment #8887285 - Flags: review?(s.kaspari) → review-
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
Attachment #8887285 - Flags: review- → review+
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?
Flags: needinfo?(s.kaspari)
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
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.
Attachment #8887287 - Flags: review?(liuche) → review+
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.
Attachment #8887286 - Flags: review?(liuche) → review+
Whiteboard: [MobileAS] → [MobileAS] 1.26
Iteration: --- → 1.26
Priority: P2 → P1
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.
Attachment #8888066 - Flags: review?(s.kaspari) → review+
Comment on attachment 8888067 [details]
Bug 1377287: Update TopSitesCard label to iOS algorithm.

https://reviewboard.mozilla.org/r/158968/#review166228
Attachment #8888067 - Flags: review?(s.kaspari) → review+
Comment on attachment 8888068 [details]
Bug 1377287: Use FFiOS label algorithm for BottomSheetContextMenu.

https://reviewboard.mozilla.org/r/158970/#review166232
Attachment #8888068 - Flags: review?(s.kaspari) → review+
Comment on attachment 8888069 [details]
Bug 1377287: Rm now unused ActivityStream.extractLabel.

https://reviewboard.mozilla.org/r/158972/#review166234
Attachment #8888069 - Flags: review?(s.kaspari) → review+
Flags: needinfo?(s.kaspari)
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
Attachment #8890576 - Flags: review?(michael.l.comella) → review+
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.
Attachment #8890577 - Flags: review?(michael.l.comella) → review+
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
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.