1236014 - Pocket as a built-in add-on not supported by language packs, manifest problems

Status: RESOLVED FIXED

(Firefox :: Pocket, defect)

Description

[Axel Hecht [:Pike]]

bug 1215694 comment 106:

I'm pretty sure this breaks in language packs.

Seems there's nothing hooking up browser/features/[e-mail]/chrome.manifest.

Fuchsia:locale-fr axelhecht$ find . -name \*manif\*
./browser/chrome/fr.manifest
./browser/chrome.manifest
./browser/features/chrome.manifest
./browser/features/[e-mail]/chrome.manifest
./browser/features/[e-mail]/fr.manifest
./chrome/fr.manifest
./chrome.manifest
./webapprt/chrome/fr.manifest
./webapprt/chrome.manifest
Fuchsia:locale-fr axelhecht$ find . -name \*manif\* -exec grep -H getpocket {} \;
./browser/features/chrome.manifest:manifest [e-mail]/chrome.manifest application={ec8030f7-c20a-464f-9b0e-13a3a9e97384}

I haven't run a localized nightly yet that actually has their locale files moved, but the chrome.manifest in /Applications/FirefoxNightly.app/Contents/Resources/browser/features/[e-mail] looks OK to me.

bug 1215694 comment 110:

Ah, that's because jarmaker puts the extra manifest in browser/features... and that one is not referenced anywhere. Please file a followup bug. A quick way around is to forcefully add a "manifest features/chrome.manifest" in "browser/chrome.manifest", using buildlist.py, when doing a repack.

Comment 1

[Shane Caraveo (:mixedpuppy)]

none of these are related to l10n, there is no reason to block on them.

Comment 2

[Shane Caraveo (:mixedpuppy)]

I tried a build and am not sure a) got the correct build instructions, and b) what problem I'm expecting to run into.

Built DE, it has other missing files on nightly
Built FR, it seems to work just fine

I'm building by modifying .mozconfig with:

mk_add_options MOZ_CO_LOCALES=fr
ac_add_options --enable-ui-locale=fr
ac_add_options --with-l10n-base=/Users/shanec/moz/l10n/

But get:  configure: WARNING: unrecognized options: --enable-ui-locale, --with-l10n-base

This also causes a clobber so this takes a long time, is there a simpler repack method?

Comment 3

[Axel Hecht [:Pike]]

https://developer.mozilla.org/en/docs/Creating_a_Language_Pack is the best doc. Best in the sense of "not perfect"

Sadly, I don't think there's a way to avoid the reconfig.

There's no MOC_CO_LOCALES, that probably died with CVS.

--enable-ui-locale may or may not work, but it's really left-over from pre-Firefox days that we should get rid of some day, I think.

Note, you don't want to do a build, you want to create a language pack, and install that in an en-US build, and then French should not work, despite it working in a full build.

Comment 4

[Shane Caraveo (:mixedpuppy)]

I seem to be able to build a working fr langpack on nightly.  I'm not certain what I'm missing.  One thing I notice as a difference in docs you pointed to is that the xpi does not show up under dist/install, but under dist/mac64/xpi.

For my testing I am doing the following

obj/browser/locales: 
make merge-it LOCALE_MERGEDIR=$(PWD)/mergedir
make langpack-it LOCALE_MERGEDIR=$(PWD)/mergedir

mach run

then install the xpi from obj/dist/mac64/xpi

Everything seems to work just fine, though strings of course are not translated.

Comment 5

[Shane Caraveo (:mixedpuppy)]

oh, using locale switcher addon to change to the locale, which then requires a restart.

Comment 6

[Axel Hecht [:Pike]]

For mozilla-central of revision 29258f59e545 and l10n-central/it of revision 6ef2139979dc, which maps to nightly of 46.0a1 (2016-01-05), I do:

.mozconfig:

mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-firefox-build
ac_add_options --with-l10n-base=/src/central/l10n-central/
ac_add_options --enable-application=browser
ac_add_options --with-macbundlename-prefix=Firefox


./mach configure

cd obj-firefox-build/browser/locales

make langpack-it

You probably need to 

./mach build config

and kill that at some point, to get nsinstall built.

Then open the file in dist/mac64/xpi, open about:config, set general.useragent.locale to it, restart.

Then I get the pocket UI in English, though it's localized.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: Screen Shot 2016-01-05 at 3.06.10 PM.png

The problem I am seeing is that pocket specific strings, such as context menu, are not translated.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Ok, got this now.  full builds are fine, but the manifests in the xpi's are not correct, so the pocket strings are not showing up translated.

Comment 9

[Axel Hecht [:Pike]]

Attachment: Italian Pocket UI

Here's the pocket UI I do see translated into Italian when using the Italian Nightly, but not when using the Italian language pack.

That the context menu don't show up a localized string is that it's actually in English in the localized file. Tsktsktsk, flod.

http://hg.mozilla.org/l10n-central/it/rev/6000f0fa38a4#l4.40

Comment 10

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #9)
> in English in the localized file. Tsktsktsk, flod. 
> http://hg.mozilla.org/l10n-central/it/rev/6000f0fa38a4#l4.40

Fudge! Copying and pasting strings around does that.

Comment 11

[:Gijs (out for now; he/him)]

(In reply to Francesco Lodolo [:flod] from comment #10)
> (In reply to Axel Hecht [:Pike] from comment #9)
> > in English in the localized file. Tsktsktsk, flod. 
> > http://hg.mozilla.org/l10n-central/it/rev/6000f0fa38a4#l4.40
> 
> Fudge! Copying and pasting strings around does that.

Are there no automated tests for non-en-* locales having identical strings to the source strings? Could even make a commit/push hook. We're all humans and make mistakes, but automation could help us here. Probably a different bug though...

Comment 12

[Francesco Lodolo [:flod]]

(In reply to :Gijs Kruitbosch from comment #11)
> Are there no automated tests for non-en-* locales having identical strings
> to the source strings? 

That's not possible. Think about Dutch and the amount of terms shared with English (or en-GB), plenty of English strings that belong to a localization.

These are all valid for Italian
https://transvision.mozfr.org/unchanged/?locale=it&repo=central

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: make sure browser/features/chrome.manifest is registered

I can make a language pack that works with this.  Not sure it is "correct".

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8706656 [details] [diff] [review]
make sure browser/features/chrome.manifest is registered

Review of attachment 8706656 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/pocket/locales/jar.mn
@@ +6,5 @@
>  [features/[e-mail]] @AB_CD@.jar:
>  %   locale pocket @AB_CD@ %locale/@AB_CD@/
>      locale/@AB_CD@/ (%*)
> +
> +# Bug 1236014, make sure l10n repacks work with feature addons

I'd feel better about such a hack if it were limited to #ifdef XPI_NAME

Comment 15

[Shane Caraveo (:mixedpuppy)]

Attachment: l10repack

I think it also makes more sense for this to be in a jar that does not belong to an addon.  Moving it to browser/locale/jar.mn will allow for evolution of what is in the features addons without possible breakage.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8708052 [details] [diff] [review]
l10repack

Review of attachment 8708052 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/jar.mn
@@ +113,5 @@
>      locale/pdfviewer/chrome.properties             (%pdfviewer/chrome.properties)
> +
> +#ifdef XPI_NAME
> +# Bug 1236014, make sure l10n repacks work with feature addons
> +# kind of hacky, but ensures the chrome.manifest chain is complete

s/kind of/Completely/ :)

Could you file a separate bug to fix it in a non-hacky way and mention the number of /that/ bug instead of this one? Even if we don't fix it in the short term, it's better to have an open bug and a reference to it.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/60b475de847a

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60b475de847a

Comment 19

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected