1555950 - Localize Top Sites

Status: VERIFIED FIXED

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

Description

[Petru-Mugurel Lingurar [:petru]]

Following up on https://bugzilla.mozilla.org/show_bug.cgi?id=1546999#c4 we can start the process of localizing the Top Sites suggestions.

The work for this comprises of two parts:

1. the mobile team will add the websites images in mobile/android/app/src/main/res/drawable
2. the l10n team will override the topsites configuration in each locales' package - mobile/locales/AB_CD/chrome/region.properties

Comment 1

[Petru-Mugurel Lingurar [:petru]]

The plan is for us to resolve any needed dependencies (like bug 1500795), start adding the needed imagery and smoke test the implementation.
If everything works, based on the list of localized Top Sites (TBD) we'll post a patch to add the needed imagery and the l10n team will do the topsites overrides in region.properties.

Comment 2

[Chris Peterson [:cpeterson]]

Mike Kaply says we have no agreements with partners on URLs, just when there are codes. So we can change region.properties' "https://m.wikipedia.org" and "https://m.twitter.com" links to "https://www.wikipedia.org" and "https://mobile.twitter.com" respectively to avoid that extra page redirect.

Comment 3

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1555950 - Locale changing will rebuild suggested sites lookup map; r?VladBaicu

Comment 4

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1555950 - Add/update Top Sites favicons; r?VladBaicu

Updated existing "suggestedsites_" drawables to better match current websites
favicons.
Added new "suggestedsites_" drawables that are to be used by the localized
Top Sites.
All images are added in density qualified drawable folders which based on my
testing need to range between 90x90px for mdpi to 360x360px for xxxhdpi.

Depends on D34680

Comment 5

[Francesco Lodolo [:flod]]

It looks like all these PNG should be optimized? I've checked a random one (Twitter), and it goes from 4403 to 2311 bytes.

Comment 6

[Petru-Mugurel Lingurar [:petru]]

With the above patches Fennec should support the following localized top sites - https://docs.google.com/spreadsheets/d/1DYuCQj6ZAEI77ePz1ntJ6BBrnPSV9Mm82wRoffHTeL8/edit?usp=sharing

The second patch updates existing Top Sites icons and adds the ones needed for the newly supported Top Sites.
The configuration for using the new Top Sites can be found here - https://drive.google.com/file/d/1cDzcVe3eq8IoLTmw1h3O_OQkMIMhQhs8/
A multilocale APK to test can be found here - https://drive.google.com/open?id=1EF42-v6JuugH7KMq1hmsB0iHvCDSq6Ya

Comment 7

[Petru-Mugurel Lingurar [:petru]]

(In reply to Francesco Lodolo [:flod] from comment #5)

It looks like all these PNG should be optimized? I've checked a random one (Twitter), and it goes from 4403 to 2311 bytes.

Thanks!
Tried a lossless compression on all of the images and got a 18,5 % size reduction overall.

Do this configurations - https://drive.google.com/file/d/1cDzcVe3eq8IoLTmw1h3O_OQkMIMhQhs8/ look ok to you? They are pretty simple, just overriding the values from mobile/locales/en-US/chrome/region.properties.
I've used this exact ones to build the above apk. It just works.

Comment 8

[Francesco Lodolo [:flod]]

I don't think we should send people to www.wikipedia.org, where they need to pick a language. There are localized versions, e.g. https://it.wikipedia.org/

In case, I can fix it when landing.

About landing: Jun 26 is the deadline for localization, and there's one week of all-hands (and 2 days of PTO) in the middle. If we want this in 68, it needs to land very quickly.

Comment 9

[Francesco Lodolo [:flod]]

One more note on the timing: I don't think I can land changes in the l10n repos until this is uplifted to Beta. What happens it the German file references an icon that doesn't exist in tree?

Comment 10

[Petru-Mugurel Lingurar [:petru]]

(In reply to Francesco Lodolo [:flod] from comment #9)

What happens if the German file references an icon that doesn't exist in tree?

Compile error. The build would fail.

I did a multilocale build using this configs - https://drive.google.com/file/d/1cDzcVe3eq8IoLTmw1h3O_OQkMIMhQhs8/ and it worked so I know I have included all the needed images with the right names.

Comment 11

[Francesco Lodolo [:flod]]

(In reply to Petru-Mugurel Lingurar[:petru] from comment #10)

(In reply to Francesco Lodolo [:flod] from comment #9)

What happens if the German file references an icon that doesn't exist in tree?

Compile error. The build would fail.

OK. That definitely means that we need everything landed on Beta, and the timing is really tight with All-Hands in the way.

Comment 12

[Petru-Mugurel Lingurar [:petru]]

Will wait for the initial list of Top Sites to be vetted by :abovens. They are subject to change.

Comment 13

[Chris Peterson [:cpeterson]]

(In reply to Francesco Lodolo [:flod] from comment #8)

I don't think we should send people to www.wikipedia.org, where they need to pick a language. There are localized versions, e.g. https://it.wikipedia.org/

The previous Wikipedia URL was "m.wikipedia.org", which redirected to "www.wikipedia.org". So this change would save a redirect. But using locale-specific Wikipedia sites like "it.wikipedia.org", as you suggest, would be even better.

Comment 14

[Petru-Mugurel Lingurar [:petru]]

Updated the list of localized TopSites - https://docs.google.com/spreadsheets/d/1DYuCQj6ZAEI77ePz1ntJ6BBrnPSV9Mm82wRoffHTeL8/edit?usp=sharing

• Two sites were changed (green background)
• Added explicit list of sites for locales that will use the same as other (yellow background)
  An archive containing said configurations can be found here - https://drive.google.com/file/d/1qx8NMhi7fn8JYY97SBXtcHNKF1zmyZ-Y/view?usp=sharing
  Updated APK based on that configuration - https://drive.google.com/file/d/1adyA5yQaNDpH6K9QokWQP7Yh2-E6BBNe/view?usp=sharing

Waiting for Andreas' green light to merge.

An interesting situation occurs with the wikipedia sites (easy to see because they now have the form [AB_CD].m.wikipedia.org/...) when accessing other wikipedia subdomains. Because the base url will be different there can be distinct entries in the Top Sites section, though the same is also happening on desktop. See https://drive.google.com/file/d/1eenZbbhog2J1qsp-Jj_03F2IlI-gtTaY/view?usp=sharing

@flod - there are some locales in that list (en-IE, nl-NL, nl-BE, fr-BE) for which I don't see l10n packages here - https://hg.mozilla.org/l10n-central/
Don't know how we should proceed regarding them (if we can create new packages just to support this top sites. Andreas said it would be ok to also just ignore packages that don't exist). I have not included the configs for them in the configs archive.

Comment 15

[Petru-Mugurel Lingurar [:petru]]

Andreas does not agree with using those locale-specific Wikipedia pages (in case of [it] we would have https://it.m.wikipedia.org/wiki/Pagina_principale/) so I sticked with using https://www.wikipedia.org/ for all locales.

Updated the list of localized TopSites - https://docs.google.com/spreadsheets/d/1DYuCQj6ZAEI77ePz1ntJ6BBrnPSV9Mm82wRoffHTeL8
Updated archive containing said configurations - https://drive.google.com/file/d/17JeOudzToSPNN8Hv_0FfYDK8e0DliN9G
Updated multilocale APK that uses that - https://drive.google.com/file/d/1ofmh2GllUjh-R79Xx9iD1gJNRXYLdG2m

Waiting for his green light to merge.

Comment 16

[Francesco Lodolo [:flod]]

All the -BE variants are not locales that we ship, so they should be ignored (are they even recognized by Android?).
nl-NL is just nl.

Swedish is sv-SE, and the country is uppercase (en-GB, not en-gb).

Comment 17

[Petru-Mugurel Lingurar [:petru]]

Awesome! This is also how I approached those so all should be ready to go.

(In reply to Francesco Lodolo [:flod] from comment #16)

All the -BE variants are not locales that we ship, so they should be ignored (are they even recognized by Android?).

I'm seeing them (nl-BE & fr-BE), and also more - https://drive.google.com/file/d/18IgEzYqCGNHAL-QAQ3u1H_t-rL3NwV7W

Comment 18

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1555950 - Ignore lint error for drawables used for localized Top Sites; r?VladBaicu

Depends on D34681

Comment 19

[Petru-Mugurel Lingurar [:petru]]

Andreas says we're good to go.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e9151073a0f
Locale changing will rebuild suggested sites lookup map; r=VladBaicu
https://hg.mozilla.org/integration/autoland/rev/278b037a8049
Add/update Top Sites favicons; r=VladBaicu
https://hg.mozilla.org/integration/autoland/rev/cc3c2ca06c27
Ignore lint error for drawables used for localized Top Sites; r=VladBaicu

Comment 21

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9071548 [details]
Bug 1555950 - Locale changing will rebuild suggested sites lookup map; r?VladBaicu

Beta/Release Uplift Approval Request

• User impact if declined: New feature targetting EU users.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: https://wiki.mozilla.org/QA/Fennec/LocalizedTopSites
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patches touch very little code, they just add support for different sets of Top Sites that are to be added by the l10n team.
• String changes made/needed:

Comment 22

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/4e9151073a0f
https://hg.mozilla.org/mozilla-central/rev/278b037a8049
https://hg.mozilla.org/mozilla-central/rev/cc3c2ca06c27

Comment 23

[Francesco Lodolo [:flod]]

Questions:

• When is the last 68 beta for Fennec? The deadline for l10n is next Wednesday, as far as I can tell there's no Fennec beta scheduled next week.
• Is QA going to test all locales that will have top sites?
• Can l10n target 69, and not 68?

I'll be on a plane next Monday and Tuesday (as called out a few comments ago), so I won't be available to land anything in l10n repositories.

Comment 24

[Diana Rus]

Hi, from QA perspective see the answers inline,

From our understanding and based on Releases scheduling from [1] next week on June 24th will be another Fennec beta build (ni here on Ryan to confirm)
[1]https://calendar.google.com/calendar/embed?src=bW96aWxsYS5jb21fZGJxODRhbnI5aTh0Y25taGFiYXRzdHY1Y29AZ3JvdXAuY2FsZW5kYXIuZ29vZ2xlLmNvbQ

The test plan[2] is based on this doc [3] and we added a few more for regression purpose. Test cases can be checked here [4] we still have to update them with the latest links.
[2]https://wiki.mozilla.org/QA/Fennec/LocalizedTopSites
[3]https://docs.google.com/spreadsheets/d/1DYuCQj6ZAEI77ePz1ntJ6BBrnPSV9Mm82wRoffHTeL8/edit#gid=839877291
[4]https://docs.google.com/spreadsheets/d/1jyDDezGeTFsqpGnY4Vk17h27jbgj1fqG599iYn5qHCM/edit#gid=0

Comment 25

[Francesco Lodolo [:flod]]

(In reply to Diana Rus from comment #24)

From our understanding and based on Releases scheduling from [1] next week on June 24th will be another Fennec beta build (ni here on Ryan to confirm)

That's next Monday. Before that, this needs to land in Beta, changes need to land in l10n repos, and need to be signed off. That's not going to happen in this timeline.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

(In reply to Diana Rus from comment #24)

From our understanding and based on Releases scheduling from [1] next week on June 24th will be another Fennec beta build (ni here on Ryan to confirm)

Confirmed.

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Julien, given comment 25 and l10n unavailability (NI Lonnen to see if we can plan back up owners when Flod is on PTO in the future) on Mon, Tues June 24th, 25th, can we plan another Fennec beta this week to verify this fix?

Comment 28

[Francesco Lodolo [:flod]]

Jeff is the right point of contact here (he's back from leave, also we are in a different org now), but his NI are still disabled.

Delphine is the person in charge of Fennec localization, but I don't think her hg access is working at this point. Axel could be a backup. Both are already CCed.

Comment 29

[Julien Cristau [:jcristau]]

(In reply to Francesco Lodolo [:flod] (PTO until Jun 26) from comment #23)

Questions:

• Can l10n target 69, and not 68?

Who can answer that?

Comment 30

[Axel Hecht [:Pike]]

My advice for this bug a month ago would have been:

• First port the datastore from l10n-central into mozilla-central, w/out data changes, confirm that that worked with QA.
• Then modify the dataset on m-c.
• Then uplift those two patches to m-b.

(In reply to Ritu Kothari (:ritu) from comment #27)

Hi Julien, given comment 25 and l10n unavailability (NI Lonnen to see if we can plan back up owners when Flod is on PTO in the future) on Mon, Tues June 24th, 25th, can we plan another Fennec beta this week to verify this fix?

I have the necessary permissions to be back-up here. Flod and I talked briefly about this, but I don't have all the details, here's what I think is what we need to do:

• [x] Land this patch on m-c
• [x] Land this patch on m-b
• [ ] Land all the data from comment 15 ? https://drive.google.com/file/d/17JeOudzToSPNN8Hv_0FfYDK8e0DliN9G/edit still contains data for a locale we don't have (en-IE). I haven't looked at the data outside of the listed locales yet at all.
• [ ] Go through all locales affected, and verify that the changes in there are OK, and update the sign-offs.
• [ ] QA Beta

Given the data doesn't match the test plan, we'll need modifications to the test plan. Also, should a non-affected locale be on the test plan?

A comment on sign-offs, we can only take or not take a locale at any point of time. We have no way to just take the region.properties change. That's why they're in m-c now, for things we care about. We might need localization changes before we can take configuration changes.

Given the complexity and the number of steps, and the lack of try for this, let me put it this way:

Both flod and I would need permission to break the build, with the understanding that we won't work on fixes for them outside of european working hours.

Comment 31

[Petru-Mugurel Lingurar [:petru]]

(In reply to Axel Hecht [:Pike] from comment #30)

[ ] Land all the data from comment 15 ? https://drive.google.com/file/d/17JeOudzToSPNN8Hv_0FfYDK8e0DliN9G/edit still contains data for a locale we don't have (en-IE). I haven't looked at the data outside of the listed locales yet at all.

Touched with ABovens about those [language-country] locales which we don't support at the moment. He was ok with skipping them and only using the [language] locale for those particular cases.
The region.properties configurations from that linked archive are the ones I built from Andreas' list and should contain all the changes needed to be added in each mobile/locales/AB_CD/chrome/region.properties (on top of what's already contained there).
Based on that configurations I've added the appropriate drawables in the app (current implementation requires a particular naming scheme) and built a test apk which in my tests ran fine, with everything as expected.

Comment 32

[Axel Hecht [:Pike]]

I forgot one more step. I'm the back-up, y'know.

After doing the sign-offs, the l10n bumper needs to commit the changes from elmo into the l10n-changesets.json file in tree, still.

Comment 33

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

We can postpone the uplift until after ESR 68 is branched and ship the new top sites in the Fennec 68.1 release. We don't need to crash land this for the Fennec 68.0 release on July 9.

Comment 34

[Julien Cristau [:jcristau]]

Thanks Chris!

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9071548 [details]
Bug 1555950 - Locale changing will rebuild suggested sites lookup map; r?VladBaicu

Approved for Fennec 68.1b2. This will also be in tomorrow's Nightly build if QA wants to get started on verification sooner than that.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/5439af0dc57e
https://hg.mozilla.org/releases/mozilla-esr68/rev/e6d0eceb09dd
https://hg.mozilla.org/releases/mozilla-esr68/rev/22f8773a4dc0

Comment 37

[Sorina Florean [:sflorean]]

We've finished testing all the sites on different Android devices. Here are the results:

Quality status: GREEN
Why is this feature green?
- There are no major bugs unresolved.
- The open issue is not related to this feature and does not have any impact on the functionality that came with this implementation.

Testing summary:
- Test report: link
- New bug: 1566733
- Firefox 68 feature QA status: Link

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Per bug 1557635 comments 19-22, this is needed for Fennec 68.0.2 to address the bustage caused by bumping l10n-changesets.json to a newer version on the 68.0.x relbranch. Approved for 68.0.2. We'll want to have this re-verified by QA at some point too, either with CI builds or after next week's dot release gtb.

FIREFOX_ESR_68_0_X_RELBRANCH:
https://hg.mozilla.org/releases/mozilla-esr68/rev/b8257c25e99276c24e6a766965f245bee863921b
https://hg.mozilla.org/releases/mozilla-esr68/rev/5591318ed206a7f05c7b1be889f4553466525729
https://hg.mozilla.org/releases/mozilla-esr68/rev/0645f30d2bc852c1173ce684e7805f4b71bc8296

Comment 39

[Diana Rus]

Hi, I have re-verified on Firefox RC 68.0.2 with Google Pixel 3 XL (Android 9) and Sony Xperia Z5 (Android 7.0) with the entire suit of test cases and there are no problems.
I have also checked on Firefox RC 68.1 with Google Pixel 3 XL (Android 9) to get it also covered, on this as well there are no problems.
I will change the status-firefox-esr68 to VERIFIED.