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

Status: RESOLVED FIXED

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

Description

[Mike Kaply [:mkaply]]

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.

Comment 1

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

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.

Comment 2

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

NI self to follow-up after bug 1390372 lands (soft dependency).

Comment 3

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

Mike, do you see a change in behavior for sites with distributions? Or how might I test this myself?

Comment 4

[Mike Kaply [:mkaply]]

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.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-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?

Comment 6

[Mike Kaply [:mkaply]]

Attachment: Here's a few screenshots comparing old and new

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.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-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.

Comment 8

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

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.

Comment 9

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

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.

Comment 10

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

Attachment: Sample: 2 line page title

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

Comment 11

[Mike Kaply [:mkaply]]

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

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-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.

Comment 13

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

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.

Comment 14

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

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

Comment 15

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

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

Comment 16

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

Attachment: Page titles: 2 lines everywhere

Comment 17

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

Attachment: Page titles: 1 line everywhere

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.

Comment 18

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

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.

Comment 19

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

Attachment: Bug 1391413: Remove unused Site constructor.

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

Comment 20

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

Attachment: Bug 1391413: Add SuggestedSites.cachedDistributionSites and accessors.

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

Comment 21

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

Attachment: Bug 1391413: Always show page titles for distributions.

This is the desired behavior but as per comment 8, it breaks a few existing
distributions' titles so we'll display two lines for those distributions in
the upcoming changesets.

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

Comment 22

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

Attachment: Bug 1391413: Add comment clarifying use of PREFS_IS_FIRST_RUN.

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

Comment 23

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

Attachment: Bug 1391413: Show 2 lines of top sites titles for distributions.

We updated the distribution spec for FF57 and we only want to show multiple
title lines for old distributions (pre-FF57) so this will be used for that.

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

Comment 24

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

Attachment: Bug 1391413: Display 2 top site title lines for old distributions.

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

Comment 25

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

Attachment: Post-patch old dist screenshot: mailcom

Comment 26

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

Attachment: Post-patch old dist screenshot: webde

Comment 27

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

Attachment: Post-patch old dist screenshot: 1and1

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.

Comment 28

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

Attachment: Post-patch old dist screenshot: gmx

And here are all the post-patch screenshots for the old distributions with the biggest issues.

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8905723 [details]
Bug 1391413: Remove unused Site constructor.

https://reviewboard.mozilla.org/r/177520/#review183256

Comment 30

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8905725 [details]
Bug 1391413: Always show page titles for distributions.

https://reviewboard.mozilla.org/r/177524/#review183258

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8905726 [details]
Bug 1391413: Add comment clarifying use of PREFS_IS_FIRST_RUN.

https://reviewboard.mozilla.org/r/177526/#review183260

Comment 32

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 33

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

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.

Comment 34

[Mike Kaply [:mkaply]]

There are no preloaded distributions that have suggested sites.

So this is really only an OTA thing.

Comment 35

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

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.

Comment 36

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

Comment on attachment 8905727 [details]
Bug 1391413: Show 2 lines of top sites titles for distributions.

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

Comment 37

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8905727 [details]
Bug 1391413: Show 2 lines of top sites titles for distributions.

https://reviewboard.mozilla.org/r/177528/#review183836

Comment 38

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8905724 [details]
Bug 1391413: Add SuggestedSites.cachedDistributionSites and accessors.

https://reviewboard.mozilla.org/r/177522/#review183838

Comment 39

[Pulsebot]

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

Comment 40

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9de7101c856f
https://hg.mozilla.org/mozilla-central/rev/6ea6536b380a
https://hg.mozilla.org/mozilla-central/rev/5a113bac7dd9
https://hg.mozilla.org/mozilla-central/rev/6f9e8b34fc59
https://hg.mozilla.org/mozilla-central/rev/4a0a1836410d