594017 - localize android java files

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

we have some strings in the embedding/android/*.java files that need to be localized. Information about the android l10n system can be found here http://developer.android.com/guide/topics/resources/localization.html and here http://developer.android.com/resources/tutorials/localization/index.html.

From Axel on irc:
Pike: blassey: seems like they hard-core discourage their old .properties style, which is OK, they came up with this strings xml replacement. I suspect that you should be fine with putting the xml in an xml.in, and use a localized strings.inc or so and the PreProcessor.py to fold them together into a localized dir
Pike: for the multi build, for single locale builds, you can probably just go with the default dir

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to use strings.xml

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Axel, one issue with this patch is it just uses Fennec in the splash screen string. We couldn't figure out how to stick the content of MOZ_APP_DISPLAYNAME in there.

Comment 3

[Axel Hecht [:Pike]]

I'm wondering if we could just make the xml file #include android.dtd and brand.dtd, something like

<!DOCTYPE resources [
#include brand.dtd
#include android.dtd
]>

where you'd copy both files to the working dir, with picking up the right file in the l10n-merge case.

Then you could just use &brandShortName; in android.dtd. Depends a bit on how the java parser is set up.

Comment 4

[Michael Wu [:mwu]]

Comment on attachment 476925 [details] [diff] [review]
patch to use strings.xml

The general approach looks fine to me. The opinion of someone doing l10n is probably more important. Just two things:

1. Can we use MOZ_APP_DISPLAYNAME in splash_screen_label?
2. Can we use AB_CD instead of NO_JA_JP_MAC_AB_CD?

Comment 5

[Axel Hecht [:Pike]]

I don't see AB_CD used at all?

Also, what does 

b.setText("Launch"); // don't need to localize

say? I lack context on that comment.

Comment 6

[Michael Wu [:mwu]]

(In reply to comment #5)
> I don't see AB_CD used at all?
> 
This is true.

> Also, what does 
> 
> b.setText("Launch"); // don't need to localize
> 
> say? I lack context on that comment.
This is text for a button that is showed in a special debugging mode. It lets us control when gecko gets started so we can attach gdb and debug startup problems.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #6)
> (In reply to comment #5)
> > I don't see AB_CD used at all?
> > 
> This is true.
> 
> > Also, what does 
> > 
> > b.setText("Launch"); // don't need to localize
> > 
> > say? I lack context on that comment.
> This is text for a button that is showed in a special debugging mode. It lets
> us control when gecko gets started so we can attach gdb and debug startup
> problems.

This is a debug only screen used for attaching gdb. I don't think we localize those sorts of things.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 476925 [details] [diff] [review]
patch to use strings.xml

I'm noting this as a feedback-, as I think we'll need to answer Brad's question about the brand names differently.

Re the comment on "Lauch", make that more explicit so folks can understand that without reading the whole method? I do agree with not localizing that string, though.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #3)
> I'm wondering if we could just make the xml file #include android.dtd and
> brand.dtd, something like
> 
> <!DOCTYPE resources [
> #include brand.dtd
> #include android.dtd
> ]>
> 
> where you'd copy both files to the working dir, with picking up the right file
> in the l10n-merge case.
> 
> Then you could just use &brandShortName; in android.dtd. Depends a bit on how
> the java parser is set up.

This works if I use an absolute path to brand.dtd. How do I set the search path so it can just being "brand.dtd" rather than "/home/blassey/src/mozilla-central/mobile/branding/nightly/locales/en-US/brand.dtd"

Comment 10

[Axel Hecht [:Pike]]

Basically two options:

copy all three files into one dir, as PreProcessor.py checks relative to DIRECTORY, 

or

pass the full paths via -DBRAND_PATH=..., and use 
#includesubst @BRAND_PATH@

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.2

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 477580 [details] [diff] [review]
patch v.2

carrying mwu's review

Comment 13

[Axel Hecht [:Pike]]

Comment on attachment 477580 [details] [diff] [review]
patch v.2

r-.

In this case, strings.inc should be a DTD, and included like brand.dtd. That way, we can deduce the variable format from the file.

Also, @MOZ_ABS_BRANDING_DIRECTORY@/locales/@AB_CD@/brand.dtd is not going to work for l10n. Look at http://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in#51 or http://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in#89 on how to resolve those right, and make l10n-merge builds work right. The first is for something like

res/values/strings.xml: strings.dtd brand.dtd strings.xml.in
	mkdir -p res/values
	cp $^ .
	PreProcessor.py strings.xml.in > res/values/strings.xml

with vpaths set for %.dtd,

the other is for 
BRANDPATH = $(firstword $(wildcard $(LOCALE_MERGEDIR)/brand.dtd) 
                        $(wildcard $(LOCALE_SRCDIR)/brand.dtd) 
                        $(srcdir)/en-US/brand.dtd )
(this needs to be tweaked to actually resolve brand.dtd)
and then do
res/values/strings.xml:
	PreProcessor.py -DBRANDPATH=$(BRANDPATH) strings.xml.in > res/values/strings.xml

I tend to think the vpath one is slicker for your use case.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

the vpath thing doesn't work for dtd's because the preprocessor looks in the srcdir for dtd's and we really don't want to be copying things into the srcdir.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 477745 [details] [diff] [review]
patch


>+        outFile.setLastModified(fileEntry.getTime());
>     }
> 
>-        outFile.setLastModified(fileEntry.getTime());

please ignore this

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 477745 [details] [diff] [review]
patch

r-, for a few:

+abs_topsrcdir   = `cd $(topsrcdir) && pwd`

is frowned upon, there are other places where we need abs topsrc dirs that do .... forgot. Ask ted or khuey?

+BRANDPATH = $(firstword $(wildcard $(LOCALE_MERGEDIR)/brand.dtd) \
+	$(wildcard $(LOCALE_SRCDIR)/brand.dtd) \
+	$(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/locales/$(AB_CD)/brand.dtd)

should be in an #ifdef LOCALE_MERGEDIR and not merge else.

You'll want another one like this for strings.dtd, and get rid of strings.inc, like I said in my earlier review. 

Also, don't define AB_CD in the preprocessor line if you don't use it.

And, the generation of strings.xml should be done always, the dependencies aren't strong enough to only do that "when needed". In particular, when locale changes or such, you end up depending on different files than before.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 18

[Axel Hecht [:Pike]]

Comment on attachment 478312 [details] [diff] [review]
patch

grrr, I suck at this review. I find more and more stuff piecemeal-wise, sorry.

strings.dtd will need to be in locales/en-US, I guess locales/en-US/android/strings.dtd or installer/android.dtd?

The target path of strings.xml should probably depend on the locale code. How exactly depends on the locale lookup code on android. Probably some s/-/_/ business, at least. Probably wanna use the no-ja-JP-mac one, too. Maybe just for en-US copy it to res/values/strings.xml, too, as fallback?

/locales/$(AB_CD)/brand.dtd
won't work for anything but en-US. you wanna hardcode that to en-US, LOCALE_SRCDIR is the one that depends on AB_CD (and RELATIVE_SRCDIR (should be mobile)).

The rule to put the strings.xml in the right place may want to be in locales/Makefile.in so that we can use it for repacks.

Not exactly sure how this will go for the multi-locale builds.

Did we at all start to repack android builds yet? At least locally in some proof-of-concept? Really hard to flesh this out without a build to test this in, I guess.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

Not sure if this is the question your asking, but for android packages the "defaults" go in values/*. Other languages go in values-AB_CD/* and mask what's in values/*.

I would assume for a multilocale build we'd have our defaults in values/strings.xml and all the other languages in values-AB_CD/strings.xml.

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 21

[Axel Hecht [:Pike]]

Comment on attachment 478967 [details] [diff] [review]
patch

This patch doesn't work, for reasons stated before (dtd file in a non-localizable path, for example) and new ones.

I don't see us getting anywhere by guessing on what might work and what might not.

Can we actually have some localized builds to test this in before making more iterations?

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Axel, given that the android specific bits have landed in bug 600214, can you own this bug?

Comment 23

[Axel Hecht [:Pike]]

Sorry, can't. I don't have free cycles, nor a setup for android builds, nor something to test the results. I'm hacking on bug 525438 to make l10n-merge less cumbersome, but I'm currently running into a wall with the fx windows installer, so no eta on that patch. It'd bring macros for picking up the right files from the right dirs, possibly I can factor out a patch-let of that to re-use.

Comment 24

[Brian Crowder]

I'd be willing to own this, if no one else is moving forward on it...

Comment 25

[Axel Hecht [:Pike]]

That'd be great from my POV.

http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry&rev=4cfb3752fab4 isn't looking too bad right now, fyi.

Comment 26

[Michael Wu [:mwu]]

Taking this bug for now since I'm writing the patches that will make l10n repacks on Android possible (bug 567873), so I might know how to get this part going too.

Comment 27

[Axel Hecht [:Pike]]

FWIW, the patches in bug 525438 won't land in time for this bug, I don't have the cycles ad-hoc, nor am I confident that it's good to land this while B7 is on the bastard branch.

https://bugzilla.mozilla.org/attachment.cgi?id=482213&action=diff#a/config/config.mk_sec1 shows the code snippet to get the right fallback code though, so you can use the result of the MERGE_FILE macro to just do the right thing for the android DTD file.

Comment 29

[Michael Wu [:mwu]]

Android unfortunately compiles all strings and code together into a binary blob. This requires us to move more of the build process to package time. Additionally, I am having problems getting compare-locales to pick up the android_strings dtd which I guess may be related to Pike's comment. Too tired to figure it out right now.

Comment 30

[Michael Wu [:mwu]]

Attachment: 1. Add embedding/android localization stuff to fennec

Patch produced with pike's help.

Comment 31

[Michael Wu [:mwu]]

Attachment: 2. Support localization of strings in embedding/android code

This moves the generation of gecko.ap_ to packaging so we can pick up new strings added with make chrome-%. It also creates a fake chrome directory for these strings so our current l10n code dealing with LOCALE_MERGEDIR can automatically drop in the appropriate localized dtds when chrome-% is run.

Comment 32

[Michael Wu [:mwu]]

Attachment: 3. Localize the title in the crash reporter

Comment 33

[(not currently active) Ted Mielczarek]

Comment on attachment 494216 [details] [diff] [review]
2. Support localization of strings in embedding/android code

rs=me. I already hate this Makefile and I hate l10n in the build system, so whatever.

Comment 34

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/efc1b8bed8b9
http://hg.mozilla.org/mozilla-central/rev/5afc616dc96f
http://hg.mozilla.org/mobile-browser/rev/65cae343e720

Comment 35

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 494216 [details] [diff] [review]
2. Support localization of strings in embedding/android code

>-DEFAULT_BRANDPATH = $(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/locales/en-US/brand.dtd
>+DEFAULT_BRANDPATH = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/locales/en-US/brand.dtd
This broke the build for any objdir which isn't a sibling of the src dir

Comment 36

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: fix for non-sibling object and source dirs

Comment 37

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 499673 [details] [diff] [review]
fix for non-sibling object and source dirs

pushed http://hg.mozilla.org/mozilla-central/rev/6e5f1f44eb3b