935290 - Update sdk/l10n to support use of packaged `.properties` files

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Hey Alex, I wanted to see if you saw any issues with this or had any advice on implementing it?

I'm curious if we lose something by parsing properties files with https://developer.mozilla.org/docs/XPCOM_Interface_Reference/nsIStringBundleService or if there is a better alternative.

Comment 2

[Alexandre Poirot [:ochameau]]

I already gave my green light to Irakli on some pull request talking about various things in order to simplify jetpack and make it work without build script directly from the addon manager.

There shouldn't be major issue in doing that. You should be able to use nsIStringBundleService, but you will have to interpret its result in order to implement plural rules correctly. (you need to identify and merges all keys that match one entry, like foo[one]=... and foo[other]=...)

Otherwise, from what I understood from the plan is that it shouldn't change anything for addons developers and that's really great! We will just not save json files as intermediate format.
If for some reason you want to change any developer facing behavior features regarding localization, I would highly suggest you to just merge with what is being done in Gaia. (ie. please, avoid at all cost to add any jetpack specifics)
For the story, we (jetpack and gaia) used to start with the exact same implementation/features for l10n, but gaia evolved and became more featurefull, mature and tested.

I do not see any particular issue in this project, but do not hesitate to ping me if you see/encounter some.

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

Since we are updating a built-in module here, which will be used for add-ons based on the sdk 1.15 build (using json), and with sdk add-ons that use `.properties` files (for bug 915376) the update here will have to support both formats afaict.

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

So based on comment 3 I think that we will need to support both json and properties formats, and have tests for both cases.

Do you agree Irakli?

If this is the case then I suggest we hold off on this bug until bug 935233 is resolved.

It will be easier to first support the json case, then add support for the `.properties` format afterwards when closing bug 915376.

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #4)
> So based on comment 3 I think that we will need to support both json and
> properties formats, and have tests for both cases.
> 
> Do you agree Irakli?
> 

If I recall correctly bootstrap.js or add-on/runner does part of the work for l10n, namely loads and parses
localization files. l10n module than just makes use of data given to it. Since for new gen add-on's we'll be
moving bootstrap.js stuff to an add-on manager, I can imagine we could just change it slightly so that it
will load and parse properties files instead of JSON's. Older add-ons will still use old bootstrap.js so it maybe doable to support just one use case.

That being said I'm not implying it's a best way to go about it, it may or may not be.


> 
> If this is the case then I suggest we hold off on this bug until bug 935233
> is resolved.
> 

Feel free to do things in order that make more sense to you, although I don't really see direct
connection with a loader here.


>
> It will be easier to first support the json case, then add support for the
> `.properties` format afterwards when closing bug 915376.

I'm not sure that is a case, because if we change cfx to just zip files and maybe generate json's
I don't really see how are we going to migrate such add-ons to .properties files, unless of course
decide to keep supporting both json and property files forever. On the other hand if we have property
files in `install.rdf`-less add-ons and `json` files in old add-ons with `install.rdf` it's a lot more
clear migration path.

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Alexandre Poirot (:ochameau) from comment #2)
> I already gave my green light to Irakli on some pull request talking about
> various things in order to simplify jetpack and make it work without build
> script directly from the addon manager.
> 
> There shouldn't be major issue in doing that. You should be able to use
> nsIStringBundleService, but you will have to interpret its result in order
> to implement plural rules correctly. (you need to identify and merges all
> keys that match one entry, like foo[one]=... and foo[other]=...)
> 
> Otherwise, from what I understood from the plan is that it shouldn't change
> anything for addons developers and that's really great! We will just not
> save json files as intermediate format.
> If for some reason you want to change any developer facing behavior features
> regarding localization, I would highly suggest you to just merge with what
> is being done in Gaia. (ie. please, avoid at all cost to add any jetpack
> specifics)
> For the story, we (jetpack and gaia) used to start with the exact same
> implementation/features for l10n, but gaia evolved and became more
> featurefull, mature and tested.
> 
> I do not see any particular issue in this project, but do not hesitate to
> ping me if you see/encounter some.

I was just under impression that we wouldn't be able to parse our .properties
files because of extensions you've described. It looks like it's not a case
which is great. And no we're not going to invent another localization format :P

Alex is there some code from gaia we can use for parsing and doing normalizations
you've described ? If we can just use whatever gaia does that would be perfect.

Comment 7

[Alexandre Poirot [:ochameau]]

Gaia implementation is over here:
  https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l10n.js
Keep in mind that it evolved since then. The plural rule is a now slightly different and there is a powerfull attribute and logical expression mechanisms.

Otherwise, parsing the properties files is something really simple. As I said you should be able to use the .properties reader xpcom. I may even have just reimplemented in python what this xpcom does!
The plural normalization is something really simple:
https://github.com/mozilla/addon-sdk/blob/master/python-lib/cuddlefish/property_parser.py#L77-L111

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1345

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8359509 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1345

I would really like if Alex could take a look at this too. 

Erik, I also wish you would reply my comment in regards to the need of keeping
both JSON and properties file support: https://bugzilla.mozilla.org/show_bug.cgi?id=935290#add_comment cause I fail to understand why do we want
support both formats, I would very much prefer to make a switch rather than
having ever growing code base.

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #9)
> Comment on attachment 8359509 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1345
> 
> I would really like if Alex could take a look at this too. 
> 
> Erik, I also wish you would reply my comment in regards to the need of
> keeping
> both JSON and properties file support:
> https://bugzilla.mozilla.org/show_bug.cgi?id=935290#add_comment cause I fail
> to understand why do we want
> support both formats, I would very much prefer to make a switch rather than
> having ever growing code base.

which comment are you referring to? just needinfo me in the future, sometimes I miss things.

The reason we need both is to support the use case of hacking on an unpacked add-on/jetpack.

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8359509 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1345

I do not have time to review such patch, please try to embrace localization!
I feel like the current review was a per-function review, it would be great to not depend on me for a more wise review.
Having said that I looked briefly at the modification made to l10n modules
and that looks correct localization-wide.

I also feel that maintaining json and properties should be avoided,
but I'm not sure we can drop json support. Wouldn't that mean breaking all existing addons that only ship json files in the xpi?

Also I feel like this patch may be improved to better factorize l10n logics between json and properties implementation. There is still json-speficic in the common codepath and I tend to think we can factorize more l10n logic at this code spot in l10n/core. May be having a getPlural would help that would received the plural form mapping function. We could also factorize the locales fallback being implemented only for properties, by passing a `locale` argument to {json|properties}.{get|getPlural}() functions.
So that maintaining both formats would be really cheap and most importantly format-specific code would contain very few l10n-speficic features.

There is most likely mistakes in following code, but I'm thinking about something like that:
l10n/core:
let fetcher = usingJSON?require(json/core):require(properties/core)
function get(locales, k, args) {
  let locale = locales.shift();
  let localized;
  if (args.length>=1) {
    let pluralForm = getPluralForm(locale...);
    localized = fetcher.getPlural(pluralForm(locale))
  } else {
    localized = fetchget.get(locale, k);
  }
  if (localized)
    return localized;
  return get(locales, k, args);
}
exports.get = function (k) get(preferedLocales, k, args)

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #9)
> Comment on attachment 8359509 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1345
> 
> I would really like if Alex could take a look at this too. 
> 
> Erik, I also wish you would reply my comment in regards to the need of
> keeping
> both JSON and properties file support:
> https://bugzilla.mozilla.org/show_bug.cgi?id=935290#add_comment cause I fail
> to understand why do we want
> support both formats, I would very much prefer to make a switch rather than
> having ever growing code base.

Hmm I think I've addressed this in a few places, but I'll record the reason here.

So there are add-ons in the wild that use the json format atm, and they cannot be changed by us, so we MUST support the json format.  Also in order to support the use case where an add-on developers can sym link an add-on folder to their Fx profile's `extensions` directory, which will only contain `.properties` files, we must support `.properties` files too.  Therefore we have to support both formats for the time being, we can deprecate the json format, wait awhile and remove it after sometime, to give developers a chance to repack their add-ons and avoid breakage, but otherwise we no options besides breaking all existing localized jetpacks.

Comment 13

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Alexandre Poirot (:ochameau) from comment #11)
> Comment on attachment 8359509 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1345
> 
> I do not have time to review such patch, please try to embrace localization!

Oui!

> I feel like the current review was a per-function review, it would be great
> to not depend on me for a more wise review.
> Having said that I looked briefly at the modification made to l10n modules
> and that looks correct localization-wide.
> 
> I also feel that maintaining json and properties should be avoided,
> but I'm not sure we can drop json support. Wouldn't that mean breaking all
> existing addons that only ship json files in the xpi?

Yes, that is exactly correct.

> Also I feel like this patch may be improved to better factorize l10n logics
> between json and properties implementation. There is still json-speficic in
> the common codepath and I tend to think we can factorize more l10n logic at
> this code spot in l10n/core. May be having a getPlural would help that would
> received the plural form mapping function. We could also factorize the
> locales fallback being implemented only for properties, by passing a
> `locale` argument to {json|properties}.{get|getPlural}() functions.
> So that maintaining both formats would be really cheap and most importantly
> format-specific code would contain very few l10n-speficic features.

I'm not sure I follow all of these changes, I agree that the code could be optimized and refactored though, and we should do this, but I don't think it is urgent at the moment and we can save this for later and focus on more important bugs for now.

Comment 14

[Alexandre Poirot [:ochameau]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #13)
> (In reply to Alexandre Poirot (:ochameau) from comment #11)
> > Comment on attachment 8359509 [details] [review]
> > Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1345
> > 
> > I do not have time to review such patch, please try to embrace localization!
> 
> Oui!

ahah, localization is tricky, you need spaces around `!` in french :p
(it took me a while to figure out you shouldn't have a space before in english!)


> > Also I feel like this patch may be improved to better factorize l10n logics
> > between json and properties implementation. There is still json-speficic in
> > the common codepath and I tend to think we can factorize more l10n logic at
> > this code spot in l10n/core. May be having a getPlural would help that would
> > received the plural form mapping function. We could also factorize the
> > locales fallback being implemented only for properties, by passing a
> > `locale` argument to {json|properties}.{get|getPlural}() functions.
> > So that maintaining both formats would be really cheap and most importantly
> > format-specific code would contain very few l10n-speficic features.
> 
> I'm not sure I follow all of these changes, I agree that the code could be
> optimized and refactored though, and we should do this, but I don't think it
> is urgent at the moment and we can save this for later and focus on more
> important bugs for now.

That's not really a refactoring about existing code but more about the new one introduced in this patch. It sounds easier to achieve before the code actually lands. If that can help, we can take some time during the pdx meetup to discuss about l10n.

Comment 15

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/a0c66d0bd3b062cd250426bdb9250198e131f5ec
Bug 935290 - Update sdk/l10n to use .properties files

https://github.com/mozilla/addon-sdk/commit/b3fc77798ebe9c21d7afce33c5075d54484d943b
Merge pull request #1345 from erikvold/935290

Bug 935290 - Update sdk/l10n to support use of packaged `.properties` files r=@gozala