Improve activity stream topsites title handling

RESOLVED FIXED in Firefox 52

Status

()

Firefox for Android
Awesomescreen
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ahunt, Assigned: sebastian)

Tracking

Trunk
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [MobileAS])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
We want to be able to do the following:

www.mozilla.org -> mozilla
foobar.blogspot.com -> foobar
m.spiegel.de -> spiegel

I think it's probably enough to use stripCommonSubdomains(), then take the first part of the url (split on "."), but maybe there's some utility that would allow us to do this?
(Reporter)

Updated

2 years ago
Assignee: nobody → ahunt
Depends on: 1293790
Whiteboard: [MobileAS]
Comment hidden (mozreview-request)
(Reporter)

Updated

2 years ago
Priority: -- → P1
(Assignee)

Comment 2

2 years ago
What do we need this for? Sounds like something that could be pretty complicated (or impossible) to do right for all edge cases.
(Reporter)

Comment 3

2 years ago
Created attachment 8786789 [details]
topsites_paged2.png

(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> What do we need this for? Sounds like something that could be pretty
> complicated (or impossible) to do right for all edge cases.

The mockups seem to use strings like:
"ask" for ask.com
"slate" for slate.com
"arstechnica" for arstechnica.com

The full page title doesn't fit in the topsites card (see attached screenshot), using the URL has a similar issue. I guess a compromise would be to strip common prefixes (m./mobile./www.) and then show as much of the URL as possible?

I agree that this seems tricky to get right, I wonder if there are alternative solutions to displaying enough descriptive text?

Looking at Firefox iOS: they seem to use the full page title in a 3x3 topsites grid, albeit with smaller text: maybe we just need to use a similar or smaller text size for our 4x1 grid?
(Reporter)

Comment 4

2 years ago
I'm renaming this bug since there might be alternative solutions. E.g. using the full title spread over 2 lines of text seems to provide sufficient information for me, which might be the best solution and/or an interim solution.
Summary: Extract most significant part of domain for topsites → Improve activity stream topsites title handling

Updated

2 years ago
Priority: P1 → P2

Updated

2 years ago
Priority: P2 → P1

Updated

2 years ago
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 8

2 years ago
Created attachment 8788597 [details]
favicon_2line.png

Here's a screenshot with 2 lines of text in the title. This seems to be a touch more legible than sticking to one line, while still using the page title.
(Reporter)

Updated

2 years ago
Priority: P1 → P2

Updated

2 years ago
Priority: P2 → P1
(Reporter)

Updated

2 years ago
Status: ASSIGNED → NEW
(Assignee)

Comment 9

2 years ago
That's how the desktop add-on wants to improve the algorithm they are using:
https://github.com/mozilla/activity-stream/issues/1311

We should try to follow their lead here, I guess. It will still be hard or near to impossible to make this work for the whole of the web. But we can explore/improve this together with the desktop team going forward.
(Assignee)

Updated

2 years ago
Blocks: 1306608
(Assignee)

Updated

2 years ago
Depends on: 1306611
(Reporter)

Updated

2 years ago
Priority: P1 → P2
(Reporter)

Updated

2 years ago
Assignee: ahunt → nobody
(Assignee)

Updated

2 years ago
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Priority: P2 → P1
(Assignee)

Updated

2 years ago
status-firefox51: affected → ---
(Assignee)

Updated

2 years ago
Attachment #8786402 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8788595 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8788596 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8799717 [details]
Bug 1299201 - TestStringUtils: Remove unneeded Assert prefix.

https://reviewboard.mozilla.org/r/84852/#review83542
Attachment #8799717 - Flags: review?(gkruglov) → review+

Comment 15

2 years ago
mozreview-review
Comment on attachment 8799718 [details]
Bug 1299201 - StringUtils: Add helper method for joining strings with a separator.

https://reviewboard.mozilla.org/r/84854/#review83546

All hail java8's String.join and StringJoiner, but I guess we're stuck with this.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/StringUtils.java:282
(Diff revision 1)
> +     * Joining together a sequence of strings with a separator.
> +     */
> +    public static String join(@NonNull String separator, @NonNull List<String> parts) {
> +        final StringBuilder builder = new StringBuilder();
> +
> +        for (int i = 0; i < parts.size(); i++) {

You could unroll the loop a bit to make this a tad faster:
```
if (parts.size() == 0) {
    return "";
}

builder.append(parts.get(0));

for (int i = 1; i < parts.size(); i++) {
     builder.append(separator);
     builder.append(parts.get(i));
}
```
Attachment #8799718 - Flags: review?(gkruglov) → review+

Comment 16

2 years ago
mozreview-review
Comment on attachment 8799719 [details]
Bug 1299201 - Add helper method for stripping the public suffix from a domain.

https://reviewboard.mozilla.org/r/84856/#review83560

I've only briefly looked through the trie stuff, I'm guessing that's what you've copied over?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/publicsuffix/PublicSuffix.java:36
(Diff revision 1)
> +     *
> +     * www.mozilla.org -> www.mozilla
> +     * independent.co.uk -> independent
> +     */
> +    @Nullable
> +    public static String stripPublicSuffix(@Nullable String domain) {

I'm really not a fan of allowing null as either a parameter or (worse) a return value... I'd rather callers of this not do silly things like try and strip a public suffix off of a null in the first place.

Left as is, IDE will now highlight unchecked uses of this function's return value as potential NPEs (and it'll be right to do so), but that seems unnecessary.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/publicsuffix/PublicSuffix.java:81
(Diff revision 1)
> +
> +    /**
> +     * Normalize domain and split into domain parts (www.mozilla.org -> [www, mozilla, org]).
> +     */
> +    private static List<String> normalizeAndSplit(String domain) {
> +        domain = domain.replace("[.\u3002\uFF0E\uFF61]", "."); // All dot-like characters to '.'

I don't think your unit tests cover dot-like character parsing.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/publicsuffix/PublicSuffix.java:82
(Diff revision 1)
> +    /**
> +     * Normalize domain and split into domain parts (www.mozilla.org -> [www, mozilla, org]).
> +     */
> +    private static List<String> normalizeAndSplit(String domain) {
> +        domain = domain.replace("[.\u3002\uFF0E\uFF61]", "."); // All dot-like characters to '.'
> +        domain = domain.toLowerCase();

for clarity i'd put these values into a new string object... Although that won't change anything functionally.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/publicsuffix/PublicSuffix.java:103
(Diff revision 1)
> +     */
> +    private static int joinIndex(List<String> parts, int index) {
> +        int actualIndex = 0;
> +
> +        for (int i = 0; i < index; i++) {
> +            if (i > 0) {

(i guess this is the same comment about unrolling a for loop as I've made on an earlier commit) -> could just start iterating at i=1 and drop this if.
Attachment #8799719 - Flags: review?(gkruglov) → review+

Comment 17

2 years ago
mozreview-review
Comment on attachment 8799720 [details]
Bug 1299201 - Introduce ActivityStream.extractLabel() to extravt a label from a URL.

https://reviewboard.mozilla.org/r/84858/#review83564

Code seems fine.

Speaking more broadly, I'm wondering if this effort isn't misguided in the first place. People extract all sorts of information out of URLs, and do so often in complicated and hard-to-predict ways. They extract meaning according to rules that are by-product of person's interaction with a website, and with the internet at large. We're bound to get this wrong, and annoy a lot of people, all in the name of "our UI doesn't have enough space for longer URLs". Perhaps the problem here are not the long URLs.

My personal pet peeves that come up right away:
- amazon.ca and amazon.com will become amazon. That is simply wrong, and will be very annoying. These are very different websites, as far as I'm concerned.
- reddit.com/r/firefox will become firefox. It _should be_ /r/firefox, but our rules don't know about reddit conventions, of course.

Hard-coding some one-off rules for different websites might work for, perhaps, top 100, but is bound to miss the mark for majority of the web.
Attachment #8799720 - Flags: review?(gkruglov) → review+
(Assignee)

Comment 18

2 years ago
(In reply to :Grisha Kruglov from comment #17)
> Speaking more broadly, I'm wondering if this effort isn't misguided in the
> first place. People extract all sorts of information out of URLs, and do so
> often in complicated and hard-to-predict ways. They extract meaning
> according to rules that are by-product of person's interaction with a
> website, and with the internet at large. We're bound to get this wrong, and
> annoy a lot of people, all in the name of "our UI doesn't have enough space
> for longer URLs". Perhaps the problem here are not the long URLs.
> 
> My personal pet peeves that come up right away:
> - amazon.ca and amazon.com will become amazon. That is simply wrong, and
> will be very annoying. These are very different websites, as far as I'm
> concerned.
> - reddit.com/r/firefox will become firefox. It _should be_ /r/firefox, but
> our rules don't know about reddit conventions, of course.
> 
> Hard-coding some one-off rules for different websites might work for,
> perhaps, top 100, but is bound to miss the mark for majority of the web.

See comments above and discussion here: https://github.com/mozilla/activity-stream/issues/1311

I absolutely agree that it will be nearly impossible to have a solution that scales to the craziness of the web. The desktop team still wants to try and explore - and for the sake of consistency I implemented this.

For other parts of the UI the desktop team is using provider_name from embed.ly - That's something we might be trying to add to our metadata parser too (It seems to be based on og:site_name and similar values). After that the URL based approach might just be a fallback. See https://github.com/mozilla/activity-stream/issues/1473
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

2 years ago
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b9a4f60323cc
TestStringUtils: Remove unneeded Assert prefix. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/8b2efd8c1d86
StringUtils: Add helper method for joining strings with a separator. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/a9f816396d7e
Add helper method for stripping the public suffix from a domain. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/f4d4225c3c0e
Introduce ActivityStream.extractLabel() to extravt a label from a URL. r=Grisha

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b9a4f60323cc
https://hg.mozilla.org/mozilla-central/rev/8b2efd8c1d86
https://hg.mozilla.org/mozilla-central/rev/a9f816396d7e
https://hg.mozilla.org/mozilla-central/rev/f4d4225c3c0e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
See Also: → bug 1311160
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1306611
(Assignee)

Updated

2 years ago
Blocks: 1311099

Updated

2 years ago
Iteration: --- → 1.6
You need to log in before you can comment on or make changes to this bug.