Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Description • 8 years ago

      No description provided.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 1 • 8 years ago

This is the very first stepping stone that we have to implement in Gecko to get L20n. It also is the one with the highest question mark factor, so I'd like to start tackling it now.

My initial approach is to build a singleton JSM Registry module that allows Sources to be plugged into it.

The Sources may look like this:

 - toolkit bundle Source
 - browser bundle Source
 - pl langpack Source
 - live-update Source

The app will then add Sources to the Registry and prioritize them. It means that the same resource ID may be registered in toolkit and browser (allowing for overrides), or in browser and live-update allowing for live updates.

The Registry API has the following methods:

L20nRegistry.getResources(requestedLangs, resIds) -> { availableLangs, resourceBundles }

L20nRegistry.registerSource(Source);

In order to start, I need to write the `registerSource` method and the first Source (File/Jar) that will allow the Registry to load sources from the app itself.

We decided to avoid Chrome Registry, so I'm going to use nsIZipReader directly.

This allows me to freely choose the resource ID scheme. My proposal is to do "package:path" style, for example: "browser:menu/menu.ftl".

The package part will allow us to combine groups of resources under common component that may have separate language negotiation strategy (devtools in en-US, browser in pl) and overrides: browser bundle Source may override toolkit's files.

Internally, the FileSource will have to define the path template inside the zip package. For example browser Source may have sth like this: "{package}/locales/{locale}/{path}" which when given "browser:menu/menu.ftl" and "en-US" will attempt to load the file from the browser zip package using path: "browser/locales/en-US/menu/menu.ftl".

Questions:

1) Does it sound like a sane strategy?
2) How do we map files into zip files?

I seem to see omni.ja file in my Firefox bundle and it has "chrome/en-US/locale/browser/aboutDialog.dtd" file.

I'd like this file to become "browser/locales/en-US/aboutDialog.dtd". Can I do this?

And is there always omni.ja? Or is there sometimes browser.jar? Or sometimes there is no jar and I have to write the whole logic to check if I need to load from zip file, or from directory?

:bsmedberg, can you advise me?

	Axel Hecht [:Pike]
	Comment 2 • 8 years ago

I'd put the path mapping to actual paths into the source object, not the registry.

For packaging and build, I suspect it's going to be easier to start with really different paths, that don't smell like chrome registry, inside the omni.ja. /localization/... might be one.

I'd also put locale code up front, and make the locale be part of the registration process. I.e., this Source for this lang:
L10nService.register(new Source..., 'en-US')

Re reading from omni.ja, you can use the jar:// protocol, I suspect. No reason to use zipreader?

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 3 • 8 years ago

> I'd put the path mapping to actual paths into the source object, not the registry.

Yes, that's my plan!

> I'd also put locale code up front, and make the locale be part of the registration process. I.e., this Source for this lang:
L10nService.register(new Source..., 'en-US')

Can a source be multi-lingual? For example, omni.jar may have multiple languages? Langpack may have multiple languages? Live-Update Source will have "all" languages :)

> Re reading from omni.ja, you can use the jar:// protocol, I suspect. No reason to use zipreader?

Can I? Will it do the directory vs zip guessing for me?

	Axel Hecht [:Pike]
	Comment 4 • 8 years ago

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)
> > I'd also put locale code up front, and make the locale be part of the registration process. I.e., this Source for this lang:
> L10nService.register(new Source..., 'en-US')
> 
> Can a source be multi-lingual? For example, omni.jar may have multiple
> languages? Langpack may have multiple languages? Live-Update Source will
> have "all" languages :)

I expect that we'll have only a few languages in each breed of shipping per user, and that's why I think it's going to be more straightforward to announce the supported language of a source at registration time.

> > Re reading from omni.ja, you can use the jar:// protocol, I suspect. No reason to use zipreader?
> 
> Can I? Will it do the directory vs zip guessing for me?

The source knows if it's a zip or not, so no guessing involved.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 5 • 8 years ago

Follow up question for :bsmedberg:

I see "chrome/en-US/locale/browser/aboutDialog.dtd" in my omni.ja.

How do I load this file from inside of my JSM module? Should I use XHR? What's the path? I see sth like "jar:resource://gre/chrome.browser.jar!/chrome/en-US/locale/browser/aboutDialog.dtd", but not sure how am I supposed to get the "gre/chrome.browser.jar!" part and how to load it.

	Benjamin Smedberg
	Comment 6 • 8 years ago

I don't have the ability to provide detailed feedback on the code, but general guidance (including some things I said on IRC):

* don't troll through files or directories at startup. That will always cause startup performance regressions. Instead use some system (perhaps the existing registration manifests?) to register files from known locations.
* The manifests solve your problem of not knowing whether we're in an omnijar or not: they would just pass a URL to L10nService.register
* The question of whether you should use XHR or something else probably depends on how you'll be processing the data. If you're processing in JS, and especially if you're using TypedArrays/Blob, then XHR is the obvious choice.

	Staś Małolepszy :stas
	Comment 7 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/68316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68316/

	Staś Małolepszy :stas
	Comment 8 • 8 years ago

Comment on attachment 8776600 [details]
Bug 1280671 - Add L10nRegistry.

Hey Zibi -- here's what I used to make my changes in bug 1288406 work.  This is essentially https://github.com/zbraniecki/gecko-dev/blob/l20n-zibi/toolkit/modules/L10nRegistryNoIndex.jsm renamed to L10nRegistry and without 'en-US' hardcoded in Source's base paths.

Do you feel like this is close to what you'd like to land on larch?  How can I help?

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 9 • 8 years ago

Comment on attachment 8776600 [details]
Bug 1280671 - Add L10nRegistry.

Yeah, that looks good. I'm still working on several small things that could potentially improve performance, but I can land them incrementally as updates.

	Staś Małolepszy :stas
	Comment 10 • 8 years ago

Comment on attachment 8776600 [details]
Bug 1280671 - Add L10nRegistry.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68316/diff/1-2/

	Staś Małolepszy :stas
	Comment 11 • 8 years ago

https://reviewboard.mozilla.org/r/68314/#review65514

Zibi, I made two small edits and I think we can land this now and then work on improvements in next iterations.  Please consult https://reviewboard.mozilla.org/r/68314/diff/1-2/ for the view of the chnages I made since your f+.  Thanks!

::: toolkit/modules/L10nRegistry.jsm:103
(Diff revisions 1 - 2)
>  }
>  
>  function fetchFirstBundle(bundles) {
> +  // worst-case scenario: the list of bundles to return to the client is empty
> +  if (bundles.length === 0) {
> +    return bundles;

I added to protect against scenarios when L10nRegistry finds no resources at all.  We'll still need to handle the empty list of bundles which is returned here on the clientside.  I think we can repurpose bug 908780 for this.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 12 • 8 years ago

Comment on attachment 8776600 [details]
Bug 1280671 - Add L10nRegistry.

https://reviewboard.mozilla.org/r/68316/#review65588

	Staś Małolepszy :stas
	Comment 13 • 8 years ago

https://hg.mozilla.org/projects/larch/rev/375ae3ccf50bc90621263cb202e6cc2e7cd7c028
Bug 1280671 - Add L10nRegistry. r=gandalf

	Staś Małolepszy :stas
	Comment 14 • 8 years ago

The L10nRegistry landed on larch according to the design decisions made in bug 1288665.  Please file new bugs for improvements.

	Axel Hecht [:Pike]
	Comment 15 • 8 years ago

Mass change dependency tree for bug 1279002 into a whiteboard keyword.