Closed Bug 1236014 Opened 4 years ago Closed 4 years ago

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


(Firefox :: Pocket, defect)

Not set



Firefox 46
Tracking Status
firefox46 --- fixed


(Reporter: Pike, Assigned: mixedpuppy)


(Blocks 1 open bug)



(3 files, 1 obsolete file)

bug 1215694 comment 106:

I'm pretty sure this breaks in language packs.

Seems there's nothing hooking up browser/features/

Fuchsia:locale-fr axelhecht$ find . -name \*manif\*
Fuchsia:locale-fr axelhecht$ find . -name \*manif\* -exec grep -H getpocket {} \;
./browser/features/chrome.manifest: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/ 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, when doing a repack.
No longer depends on: 1235845
No longer depends on: 1235873
none of these are related to l10n, there is no reason to block on them.
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?
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(l10n) 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.
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(l10n)
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

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.
oh, using locale switcher addon to change to the locale, which then requires a restart.
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:


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.
The problem I am seeing is that pocket specific strings, such as context menu, are not translated.
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.
Attached image 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.
(In reply to Axel Hecht [:Pike] from comment #9)
> in English in the localized file. Tsktsktsk, flod. 

Fudge! Copying and pasting strings around does that.
(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. 
> >
> 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...
(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
I can make a language pack that works with this.  Not sure it is "correct".
Assignee: nobody → mixedpuppy
Attachment #8706656 - Flags: review?(mh+mozilla)
Attachment #8706656 - Attachment is patch: true
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/
@@ +6,5 @@
>  [features/] @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
Attachment #8706656 - Flags: review?(mh+mozilla) → feedback+
Attached patch l10repackSplinter Review
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/ will allow for evolution of what is in the features addons without possible breakage.
Attachment #8706656 - Attachment is obsolete: true
Attachment #8708052 - Flags: review?(mh+mozilla)
Comment on attachment 8708052 [details] [diff] [review]

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

::: browser/locales/
@@ +113,5 @@
>      locale/pdfviewer/             (%pdfviewer/
> +
> +#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.
Attachment #8708052 - Flags: review?(mh+mozilla) → review+
Blocks: 1240628
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46


STR: Not clear.
Developer specific testing

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
You need to log in before you can comment on or make changes to this bug.