488936 - Store the install locale in its own file

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

I think it is best to store the locale in its own file that it provided by toolkit. The reasons not to store it in platform.ini or application.ini are that they are not locale specific. The reason to make it part of toolkit is for xulrunner. I'm not keen on adding a file to accomplish this but I believe the benefits outweigh it.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2 - use GreD for ini file location

Comment 2

[Robert Strong (they/them - no direct email)]

A couple of other options are to:

1. add the locale to platform.ini and figure out a decent way to modify it during repackaging

2. make updater.ini a toolkit locale file which would require that all apps define MOZ_APP_DISPLAYNAME

I personally prefer either having a separate file or adding the locale to platform.ini

Comment 3

[Robert Strong (they/them - no direct email)]

Comment on attachment 373417 [details] [diff] [review]
patch rev2 - use GreD for ini file location

Ted, asking for your review as well since bsmedberg is away

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 373417 [details] [diff] [review]
patch rev2 - use GreD for ini file location

Not great, but probably the best solution overall.

Comment 5

[Benjamin Smedberg]

The value in this new file is the "install locale". If we ever had a multi-locale install package, the value here might be "all" or "multi-europe" or some other arbitrary string?

We should try really hard to make sure that the values here aren't used to actually localize the app, just provide for correct update selection.

Also we should verify that this gets repacked correctly, perhaps as part of the l10n verification steps.

Comment 6

[Robert Strong (they/them - no direct email)]

(In reply to comment #5)
> The value in this new file is the "install locale". If we ever had a
> multi-locale install package, the value here might be "all" or "multi-europe"
> or some other arbitrary string?
The app.update.url pref could then omit the %LOCALE% param and have a custom param or the app could provide its own installLocale.ini if it really wanted to. I'll add an explanatory note to the top of the file as well.

> We should try really hard to make sure that the values here aren't used to
> actually localize the app, just provide for correct update selection.
I'm not sure how this value could be used to localize the app. Could you provide some details?

> Also we should verify that this gets repacked correctly, perhaps as part of the
> l10n verification steps.
I'll look into this.

Comment 7

[Benjamin Smedberg]

I don't understand why this file would be in GreD and not the application directory: this is app-update, not platform-update.

> I'm not sure how this value could be used to localize the app. Could you
> provide some details?

I just mean we should tell people not to use this value for anything except application update.

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: patch rev3

With this patch it first looks for an update.locale file in the app dir and fall back to GreD if it doesn't exist. It also just uses a text file named update.locale instead of an ini file which makes it clearer that this is only used by app update.

One reason to add this file to GreD is that most XULRunner apps just use the toolkit locale as their locale so this simplifies things for XULRunner apps as well as Firefoxm etc. since they no longer have to add this at the app level except in packages static in the locale section. This also makes it so app update xpcshell tests pass on XULRunner and app's that don't provide an updater.ini with locale info which happened with fennec.

One reason to get it out of updater.ini is that the updater binary doesn't require the updater.ini to apply an update. When there is no updater.ini it just doesn't display UI.

If a repackaging check is done I'd prefer to add it to an app provided Makefile since the file isn't required unless the app's update url contains %LOCALE%. I could add a check when the updater.ini is preprocessed but it seems like a bit of overkill since there are other files required by locales that don't have these checks.

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: patch comm-central

This adds the new file to the packages-static files so app update doesn't break for trunk builds when this lands. The removal of the locale entry from the updater.ini can be done later in a separate bug.

Comment 10

[Robert Strong (they/them - no direct email)]

Attachment: mozilla-central patch to be checked in

Makes the locale test more resilient and restores throwing an error when the file with the locale is not found

Comment 11

[Phil Ringnalda (:philor)]

Comment on attachment 377725 [details] [diff] [review]
patch comm-central

r=me with an #ifndef MOZILLA_1_9_1_BRANCH wrapped around it so we don't panic about a missing file at 4am while respinning our next release :)

Comment 12

[Robert Strong (they/them - no direct email)]

(In reply to comment #11)
> (From update of attachment 377725 [details] [diff] [review])
> r=me with an #ifndef MOZILLA_1_9_1_BRANCH wrapped around it so we don't panic
> about a missing file at 4am while respinning our next release :)
Per your suggestion I'll do this for all apps with the caveat that I will remove the ifdefs if this bug gets approved for 1.9.1

Comment 13

[Martin Schröder [:mschroeder]]

Comment on attachment 377725 [details] [diff] [review]
patch comm-central

r=mschroeder for calendar

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 377725 [details] [diff] [review]
patch comm-central

kairo, can I get you to review this? I'll add ifdefs per comment #11 and comment #12

Comment 15

[Robert Kaiser]

Comment on attachment 377725 [details] [diff] [review]
patch comm-central

doesn't comm-central also need the same updater_append.ini changes that browser/locales gets in the other patch?

Comment 16

[Robert Strong (they/them - no direct email)]

This adds the new file to the packages-static files so app update doesn't break
for trunk builds when this lands. The removal of the locale entry from the
updater.ini can be done later in a separate bug.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 377729 [details] [diff] [review]
mozilla-central patch to be checked in

a191=beltzner, fixes a fennec bug, needs to bake on m-c for a green cycle before heading to 1.9.1

Comment 18

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/7bb72121c2a7

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/c9e081937cc4

Comment 19

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e8fc03d9a29e

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/e470fc7f0aa1

Comment 20

[Robert Strong (they/them - no direct email)]

Attachment: followup fix in Makefile.in for Mac and Linux (checked in)

Comment 21

[Robert Strong (they/them - no direct email)]

Comment on attachment 379249 [details] [diff] [review]
followup fix in Makefile.in for Mac and Linux (checked in)

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/dd5c8e01c741

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 379249 [details] [diff] [review]
followup fix in Makefile.in for Mac and Linux (checked in)

a191=beltzner

Comment 23

[Robert Strong (they/them - no direct email)]

pushed to mozilla-1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ae314e1051b6

Comment 25

[Axel Hecht [:Pike]]

Plausible, to speak in mythbusters.

We're updating the source dir to the source stamp of the nightly before repackaging, so we need a new nightly post ae314e1051b6 to verify.

PS: feel free to CC me on bugs like this right away.

Comment 26

[Robert Strong (they/them - no direct email)]

fyi: I also just re-verified that it is fixed