Closed Bug 1555950 Opened 5 months ago Closed 4 months ago

Localize Top Sites

Categories

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

Firefox 68
enhancement

Tracking

()

VERIFIED FIXED
Firefox 69
Tracking Status
firefox-esr68 68+ verified
firefox67 --- wontfix
firefox67.0.1 --- wontfix
firefox68 --- wontfix
firefox69 --- fixed

People

(Reporter: petru, Assigned: petru)

References

(Depends on 1 open bug)

Details

(Whiteboard: [bcs:p1] [trailhead:fennec] [fennec68.0.2])

Attachments

(3 files)

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
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
Type: defect → enhancement
Depends on: 1500795
Priority: -- → P1
Whiteboard: [bcs:p1]

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.

Whiteboard: [bcs:p1] → [bcs:p1] [trailhead:fennec]

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.

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

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

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

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

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.

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?

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

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

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

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

Updated the list of localized TopSites - https://docs.google.com/spreadsheets/d/1DYuCQj6ZAEI77ePz1ntJ6BBrnPSV9Mm82wRoffHTeL8/edit?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.

Flags: needinfo?(francesco.lodolo)

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.

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

Flags: needinfo?(francesco.lodolo)

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

Andreas says we're good to go.

Keywords: checkin-needed

Pushed by aiakab@mozilla.com:
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

Keywords: checkin-needed

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:
Attachment #9071548 - Flags: approval-mozilla-beta?
Attachment #9071550 - Flags: approval-mozilla-beta?
Attachment #9073046 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

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

Flags: needinfo?(ryanvm)
QA Contact: diana.rus

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

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

Flags: needinfo?(ryanvm)

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?

Flags: needinfo?(jcristau)
Flags: needinfo?(chris.lonnen)

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.

Flags: needinfo?(chris.lonnen) → needinfo?(l10n)

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

Flags: needinfo?(jcristau)

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.

Flags: needinfo?(l10n)

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

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.

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

Attachment #9071548 - Flags: approval-mozilla-beta? → approval-mozilla-esr68?
Attachment #9071550 - Flags: approval-mozilla-beta? → approval-mozilla-esr68?
Attachment #9073046 - Flags: approval-mozilla-beta? → approval-mozilla-esr68?
Depends on: 1564319

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.

Attachment #9071548 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9071550 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9073046 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Blocks: 1566733
See Also: → 1566733
No longer blocks: 1566733
See Also: 1566733

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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

Whiteboard: [bcs:p1] [trailhead:fennec] → [bcs:p1] [trailhead:fennec] [fennec68.0.2]

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.

You need to log in before you can comment on or make changes to this bug.