Closed Bug 1391413 Opened 3 years ago Closed 3 years ago

Improve fennec top sites title for distributions: use page title and display two lines

Categories

(Firefox for Android :: General, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Iteration:
1.30
Tracking Status
fennec + ---
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: mkaply, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MobileAS])

Attachments

(13 files, 1 obsolete file)

131.32 KB, image/png
Details
144.35 KB, image/png
Details
167.75 KB, image/png
Details
158.25 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
sebastian
: review+
Details
144.42 KB, image/png
Details
145.07 KB, image/png
Details
139.45 KB, image/png
Details
145.81 KB, image/png
Details
Currently on Fennec, we provide the ability for our partners to create custom distributions.

As part of that customization, they can provide tiles to use for top sites specified like this in a file called suggestedsites.json

  {
    url: "http://r.mail.ru/n259522819",
    title: "Главная Mail.Ru",
    imageurl: "gecko.distribution://suggestedsites/res/mailru",
    bgcolor: "#228FDF"
  },

Under activity steam now, the tile shows up as n259522819. Even if it showed r.mail.ru, it would be indistinguishable from their other tile:

  {
    url: "http://r.mail.ru/n259522832",
    title: "Почта Mail.Ru",
    imageurl: "gecko.distribution://suggestedsites/res/mail",
    bgcolor: "#F0F0F0"
  }

If a distribution specifies a title, it should be used as the name of the top site.
tracking-fennec: --- → ?
In bug 1390372, we're changing the title such that pages with a path (both of these) will display a page title instead of a domain name: I wonder if this issue will *just* work.

If not, we'll have to add some distribution specific code.
NI self to follow-up after bug 1390372 lands (soft dependency).
Depends on: 1390372
Flags: needinfo?(michael.l.comella)
tracking-fennec: ? → +
Priority: -- → P2
Rank: 2
Mike, do you see a change in behavior for sites with distributions? Or how might I test this myself?
Flags: needinfo?(mozilla)
You can test this by deleting data for Firefox nightly and then running this from adb shell:

am broadcast -a com.android.vending.INSTALL_REFERRER \
             -n org.mozilla.fennec_aurora/org.mozilla.gecko.distribution.ReferrerReceiver \
             -f 32 \
             --es "referrer" "utm_source=mozilla\&utm_content=mailru\&utm_campaign=distribution"

Upon restart, it should be using a mailru test with suggested sites.
Flags: needinfo?(mozilla)
Mike, I'm having difficulty getting the test environment in comment 4 to work, in either Nightly or my local builds. The ReferrerReceiver starts (I checked the log statements) but Distribution.onReceivedReferrer does not seem to (I added some log statements, which never seem to be called). Any ideas?
Flags: needinfo?(mozilla)
For local builds, you would need to change org.mozilla.fennec_aurora to match the appname in your build, but for a nightly downloaded from the play store, the above line should work.

Did you make sure to clear all Firefox data first?

I have a helper util that makes the process easy. I'll see about getting you an APK.
Flags: needinfo?(mozilla)
> For local builds, you would need to change org.mozilla.fennec_aurora to match the appname in your build

I did this.

> Did you make sure to clear all Firefox data first?

And this.

I'm not sure why I didn't get the distributions. :\

---

In any case, to fix this bug, I think we'll need to explicitly look for a distributions and set their top sites title to the provided title. In the screenshot, everything is correct but for the case where the url is just a domain, we'd use the domain name, rather than the provided title. Note: icon fixes should happen in bug 1388379 or bug 1394641.
Flags: needinfo?(michael.l.comella)
After we switch over distributions' suggestedsites to using titles, existing distributions' installations may have some problems with titles (I discovered this investigating bug 1394641). This can be fixed when distributions update to the new format (bug 1394641).

The following distributions will work well with the titles:
- mailru

The following distributions might be okay:
- gmx ("GMX Ho...", "GMX Fr..." - "Homepage" and "Freemail" respectively; icons distinguish)
- 1and1 ("1&1 Kun...", "1&1 Hil..." - "Kundenlogin" and "Hilfe-Center" respectively; icons distinguish)

And the following distributions will have identical titles between both entries (not okay!):
- webde ("WEB.DE..." shown for both but the icons distinguish)
- mailcom ("mail.co..." shown with identical icons)
- [test distributions] testsigned & testsigned2 ("Distributio..." shown but the icons distinguish)

We should consider adding a workaround, especially for mailcom which has two indistinguishable tiles, to help users who use these sites from these distributions.
Mike, do you have an opinion on what we would be an appropriate workaround for these existing distributions?

Preferably, this workaround would be generic, e.g. if the distribution titles begin with the same string ("GMX"), take off the shared string and use those for the titles (Homepage & Freemail). However, this could remove the important branding for some distributions.

Some suggestions:
- Take off shared prefix (explained above) for legacy distributions (if it's possible to identify them)
- For distributions (legacy? all?), maybe we can allow page titles to have more space, e.g. two lines.
Flags: needinfo?(mozilla)
For reference, here is what it might look like to have at most 2 lines to display the page title (it covers about half the favicon on my device).
> - For distributions (legacy? all?), maybe we can allow page titles to have more space, e.g. two lines.

I prefer this option. I think it looks great in your screenshot. IT would also work with existing configs in the wild.
Flags: needinfo?(mozilla)
Spoke with bbell: he's alright with the having 2 lines for legacy distributions (and maybe 3 or 4! but that seems unnecessary given the strings I've seen), as I've shown here. He mentioned we might even consider this for all sites (but perhaps that'd be for another bug, given the experimental nature).

Additionally, he wants to try to move the pin out of the row with the text (to one of the top corners) so that we gain an additional few characters – mocks (and maybe a follow-up bug) to follow.
Assignee: nobody → michael.l.comella
There are two changes here:
- Always use titles for distributions
- Old distributions should get two lines

The latter really breaks legacy distributions which is why I'm assigning it now despite other items of higher triaged priority.
(In reply to Michael Comella (:mcomella) from comment #13)
> - Always use titles for distributions

We'll have to do a little work to determine which tiles are from a distribution:

Currently, we get our top sites from the DB, which contains `type == SUGGESTED` only if it hasn't been visited before - we can't use this to determine suggested sites. As such, to identify suggested sites, we must cross-reference BrowserApp.mSuggestedSites, like SuggestedSitesLoader does.

However, there is no information to distinguish between default suggested sites and distributions. When we initially load the suggested sites, we write a cached version to disk (that does not distinguish) and access that in the future. Options:
- We forceably reload distributions, storing the source (default or distribution) to disk, and access that directly
- We load SuggestedSites from the resource without distributions and perform `BrowserApp.mSuggestedSites - suggestedSitesFromResource == suggestedSitesFromDistribution`

> - Old distributions should get two lines

We can determine an old distribution by:
- Cross referencing against BrowserApp.mSuggestedSites (like above)
- (hacky!) Checking the provided icon size to see if it matches the old format (before bug 1388379). Afaik, since the format has not otherwise changed between implementations, this is the only way to truly determine if we have a distribution in the old format. Alternatively, we could set a flag such as, "wasDistributionInstalledBeforeFirefox57", but since the distribution release can be out of sync with the Firefox 57 release, it is less perfect.
> - (hacky!) Checking the provided icon size

It'd be a lot easier just to add two lines for all page titles, which bbell suggested could be an option if we experimented and tried it out.
Summary: Fennec top sites should use the title especially for distributions → Improve fennec top sites title for distributions: use page title and display two lines for legacy distributions
Bryan, it'd be pretty hacky to show 2 lines only for old distributions so I thought I'd try them everywhere, like you mentioned in our conversation – what do you think?

Personally, I like the extra level of disambiguation and that the text no longer feels crammed.
Flags: needinfo?(bbell)
Spoke with Bryan on Slack: he doesn't like the way the centered titles look and that many pages will move to the two line appearance so we're going to keep it for old distributions only.
Flags: needinfo?(bbell)
Note: bbell also requested we move the pins down on a 2 line row but I think I've already spent enough time on this so I should fix other issues.
And here are all the post-patch screenshots for the old distributions with the biggest issues.
Iteration: --- → 1.29
Rank: 2
Priority: P2 → P1
Comment on attachment 8905723 [details]
Bug 1391413: Remove unused Site constructor.

https://reviewboard.mozilla.org/r/177520/#review183256
Attachment #8905723 - Flags: review?(s.kaspari) → review+
Comment on attachment 8905725 [details]
Bug 1391413: Always show page titles for distributions.

https://reviewboard.mozilla.org/r/177524/#review183258
Attachment #8905725 - Flags: review?(s.kaspari) → review+
Comment on attachment 8905726 [details]
Bug 1391413: Add comment clarifying use of PREFS_IS_FIRST_RUN.

https://reviewboard.mozilla.org/r/177526/#review183260
Attachment #8905726 - Flags: review?(s.kaspari) → review+
Comment on attachment 8905728 [details]
Bug 1391413: Display 2 top site title lines for old distributions.

https://reviewboard.mozilla.org/r/177530/#review183264

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java:157
(Diff revision 1)
> +        // With Activity Stream, we updated the top sites titles such that they were short and one line.
> +        // However, some existing distributions have titles that are too long to display well there so
> +        // we want to give them more visible lines. There are two ways to determine if we have an old version:
> +        // if the icons are the old size or the distribution, and thus the application, were first run before
> +        // Firefox 57. The former is hacky so I went with the latter. Caveat: the latter is imperfect because the
> +        // distributions are released on a server so 1) they won't be released for FF57 users until FF57
> +        // hits release and 2) the release on the server won't necessarily happen exactly simultaneously as FF57.
> +        // This should impact an insignificant number of users, however.
> +        return isFromDistribution &&
> +                !BrowserMetadataUtil.isFirstRunVersionOrNewer(itemView.getContext(), 57);

Isn't this assumption only true for OTA distributions?

"System distributions" might never change. APK distributions are build manually and I guess can be coordinates like OTA distributions.

https://wiki.mozilla.org/Mobile/Distribution_Files
Iteration: 1.29 → 1.30
Comment on attachment 8905728 [details]
Bug 1391413: Display 2 top site title lines for old distributions.

https://reviewboard.mozilla.org/r/177530/#review183264

> Isn't this assumption only true for OTA distributions?
> 
> "System distributions" might never change. APK distributions are build manually and I guess can be coordinates like OTA distributions.
> 
> https://wiki.mozilla.org/Mobile/Distribution_Files

> Isn't this assumption only true for OTA distributions?

Which assumption specifically? I think you're saying OTA can update for existing users, but they can't – they're stored in the profile. These changes would only affect new users.

That being said, you're making me realize that these distributions are really complicated and perhaps I don't understand it well enough myself. Maybe we should just give all distributions 2 lines but encourage them to use 1 to simplify the code.
There are no preloaded distributions that have suggested sites.

So this is really only an OTA thing.
Comment on attachment 8905728 [details]
Bug 1391413: Display 2 top site title lines for old distributions.

https://reviewboard.mozilla.org/r/177530/#review183264

> > Isn't this assumption only true for OTA distributions?
> 
> Which assumption specifically? I think you're saying OTA can update for existing users, but they can't – they're stored in the profile. These changes would only affect new users.
> 
> That being said, you're making me realize that these distributions are really complicated and perhaps I don't understand it well enough myself. Maybe we should just give all distributions 2 lines but encourage them to use 1 to simplify the code.

I spoke with bbell and we're going to move to 2 lines for all distributions, as we discussed on IRC.
Attachment #8905728 - Attachment is obsolete: true
Attachment #8905728 - Flags: review?(s.kaspari)
Comment on attachment 8905727 [details]
Bug 1391413: Show 2 lines of top sites titles for distributions.

https://reviewboard.mozilla.org/r/177528/#review183836
Attachment #8905727 - Flags: review?(s.kaspari) → review+
Comment on attachment 8905724 [details]
Bug 1391413: Add SuggestedSites.cachedDistributionSites and accessors.

https://reviewboard.mozilla.org/r/177522/#review183838
Attachment #8905724 - Flags: review?(s.kaspari) → review+
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9de7101c856f
Remove unused Site constructor. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/6ea6536b380a
Add SuggestedSites.cachedDistributionSites and accessors. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/5a113bac7dd9
Always show page titles for distributions. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/6f9e8b34fc59
Add comment clarifying use of PREFS_IS_FIRST_RUN. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/4a0a1836410d
Show 2 lines of top sites titles for distributions. r=sebastian
Summary: Improve fennec top sites title for distributions: use page title and display two lines for legacy distributions → Improve fennec top sites title for distributions: use page title and display two lines
You need to log in before you can comment on or make changes to this bug.