Last Comment Bug 1022767 - Use WebApp's manifests instead of locales.ini
: Use WebApp's manifests instead of locales.ini
Status: RESOLVED FIXED
:
Product: Firefox OS
Classification: Client Software
Component: Gaia::L10n (show other bugs)
: unspecified
: All All
P2 normal (vote)
: ---
Assigned To: Zibi Braniecki [:gandalf][:zibi]
:
:
Mentors:
https://github.com/zbraniecki/gaia/tr...
: 1065175 (view as bug list)
Depends on: 1064832 1072797 1079865
Blocks: 809600 999778 999779 1005965 1068380 1030035 1066347 1068382 1068723
  Show dependency treegraph
 
Reported: 2014-06-09 10:06 PDT by Staś Małolepszy :stas
Modified: 2014-12-26 17:00 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1, l20n part (14.31 KB, patch)
2014-06-16 14:48 PDT, Zibi Braniecki [:gandalf][:zibi]
stas: feedback+
Details | Diff | Splinter Review
patch, v1, gaia part (21.99 KB, patch)
2014-06-16 14:52 PDT, Zibi Braniecki [:gandalf][:zibi]
no flags Details | Diff | Splinter Review
patch v2, l20n part (15.45 KB, patch)
2014-06-18 17:28 PDT, Zibi Braniecki [:gandalf][:zibi]
no flags Details | Diff | Splinter Review
patch v2, gaia part (23.27 KB, patch)
2014-06-18 17:37 PDT, Zibi Braniecki [:gandalf][:zibi]
no flags Details | Diff | Splinter Review
patch, l10n part (15.58 KB, patch)
2014-07-02 13:27 PDT, Zibi Braniecki [:gandalf][:zibi]
stas: review-
Details | Diff | Splinter Review
patch, gaia part (34.87 KB, patch)
2014-07-02 17:47 PDT, Zibi Braniecki [:gandalf][:zibi]
no flags Details | Diff | Splinter Review
patch, v2, l10n part (17.23 KB, patch)
2014-07-07 16:12 PDT, Zibi Braniecki [:gandalf][:zibi]
stas: review+
Details | Diff | Splinter Review
patch, gaia part (33.73 KB, patch)
2014-07-09 12:11 PDT, Zibi Braniecki [:gandalf][:zibi]
stas: feedback+
yurenju: feedback+
Details | Diff | Splinter Review
pull request (46 bytes, text/x-github-pull-request)
2014-08-29 02:05 PDT, Zibi Braniecki [:gandalf][:zibi]
no flags Details | Review | Splinter Review
new pull request (46 bytes, text/x-github-pull-request)
2014-09-01 04:27 PDT, Staś Małolepszy :stas
stas: review+
yurenju: review+
l10n: review+
l10n: feedback+
Details | Review | Splinter Review
PR, fix relative path for device_type (46 bytes, text/x-github-pull-request)
2014-09-16 10:48 PDT, Zibi Braniecki [:gandalf][:zibi]
yurenju: review+
Details | Review | Splinter Review

Description User image Staś Małolepszy :stas 2014-06-09 10:06:40 PDT
L20n uses manifests to define localization resources; see here:

https://github.com/l20n/l20n.js/blob/master/docs/html.md#create-manifest

Let's stop using the semi-INI format for locales.ini adn use that instead.
Comment 1 User image Zibi Braniecki [:gandalf][:zibi] 2014-06-16 00:33:25 PDT
Stas, if you dont plan to work on that today, I'lk be happy to take it as my in-flight project.
Comment 2 User image Staś Małolepszy :stas 2014-06-16 02:49:17 PDT
Go for it :)
Comment 3 User image Zibi Braniecki [:gandalf][:zibi] 2014-06-16 14:48:27 PDT
Created attachment 8440965 [details] [diff] [review]
patch, v1, l20n part

Version 1, l20n part of the patch.

I did not use

<link rel="localization" href="locales/manifest.json"> because that's exactly what we use for JSON resources.

For now I use <link type="application/l10n" href="locales/locales.manifest"> for manifest, but I'm open to any other suggestions.
Comment 4 User image Zibi Braniecki [:gandalf][:zibi] 2014-06-16 14:52:24 PDT
Created attachment 8440968 [details] [diff] [review]
patch, v1, gaia part

This is just for reference to the first patch. Corresponding gaia changes.
Comment 5 User image Staś Małolepszy :stas 2014-06-17 07:35:53 PDT
Comment on attachment 8440965 [details] [diff] [review]
patch, v1, l20n part

Review of attachment 8440965 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not done with the initial review yet, but here are a few questions that maybe you can answer before I finish.

::: examples/regular.html
@@ +3,5 @@
>      <meta charset="utf-8">
>      <title data-l10n-id="title"></title>
>  
> +    <link rel="resource" type="application/l10n" href="locales/locales.manifest">
> +    <link rel="resource" type="application/l10n" href="locales/example.{{locale}}.properties">

The *.ini files serve three purposes:

 1. define the default resources,
 2. define the available locales,
 3. define resources for locales.

It looks like the manifest only serves the first two purposes, while additional <link> elements are required to define resources.  Would it be possible to combine these resource links into the manifest?

::: lib/l20n/context.js
@@ +108,4 @@
>      this._emitter.emit('ready');
>    }
>  
> +  this.requestLocales = function requestLocales(requested, defaultLocale) {

I don't like the fact that we're passing defaultLocale to requestLocales. As a method, requestLocales is intended to change the ordering of preferred locales when a new preference is available.  The default locale the app is available in is a static piece of information that doesn't change during runtime.

I suggest we either introduce registerLocales(defLoc, allLocsArray) or overload the Context() constructor to define the default locale as well as all available ones.

This will also open the way to bug 1005965.

@@ +117,4 @@
>  
> +    var supported = negotiate(requested.concat(defaultLocale),
> +                              requested,
> +                              defaultLocale);

With my comment above, this line could read:

  var supported = negotiate(available, requested, defaultLocale);
Comment 6 User image Zibi Braniecki [:gandalf][:zibi] 2014-06-17 10:57:50 PDT
(In reply to Staś Małolepszy :stas from comment #5)
> ::: examples/regular.html
> @@ +3,5 @@
> >      <meta charset="utf-8">
> >      <title data-l10n-id="title"></title>
> >  
> > +    <link rel="resource" type="application/l10n" href="locales/locales.manifest">
> > +    <link rel="resource" type="application/l10n" href="locales/example.{{locale}}.properties">
> 
> The *.ini files serve three purposes:
> 
>  1. define the default resources,
>  2. define the available locales,
>  3. define resources for locales.
> 
> It looks like the manifest only serves the first two purposes, while
> additional <link> elements are required to define resources.  Would it be
> possible to combine these resource links into the manifest?

So, the understanding I got from our conversation and previous attempts to define the HTML API is that we want to separate two things:

1) The list of resources required to make the webapp work (this is the list that will be sent to LangPackService)
2) The list of resources required to be loaded at the startup.

The former list will be added to manifest file. I didn't add it there yet, because there's no use case for this and the JSON is easily expandable.

The latter is defined in <head> of the document much like CSS links are.

> ::: lib/l20n/context.js
> @@ +108,4 @@
> >      this._emitter.emit('ready');
> >    }
> >  
> > +  this.requestLocales = function requestLocales(requested, defaultLocale) {
> 
> I don't like the fact that we're passing defaultLocale to requestLocales. As
> a method, requestLocales is intended to change the ordering of preferred
> locales when a new preference is available.  The default locale the app is
> available in is a static piece of information that doesn't change during
> runtime.
> 
> I suggest we either introduce registerLocales(defLoc, allLocsArray) or
> overload the Context() constructor to define the default locale as well as
> all available ones.

I'm glad you don't like it. I didn't like it either ;)

So, I'd prefer not to block the creation of Context on langneg (because I'm worried we'll later start introducing pseudo contexts until the real one is called).

I much prefer registerLocales. I'll add it in the next version of the patch.
Comment 7 User image Staś Małolepszy :stas 2014-06-18 07:45:39 PDT
(In reply to Zibi Braniecki [:gandalf] from comment #6)

> So, I'd prefer not to block the creation of Context on langneg (because I'm
> worried we'll later start introducing pseudo contexts until the real one is
> called).

You wouldn't have to.  You could do Context(id, defLoc, allLocs) which doesn't do any lang nego yet.  Then a call to ctx.requestLocales(userPrefLocs) would trigger the language negotiation.
Comment 8 User image Zibi Braniecki [:gandalf][:zibi] 2014-06-18 08:06:35 PDT
(In reply to Staś Małolepszy :stas from comment #7)
> (In reply to Zibi Braniecki [:gandalf] from comment #6)
> 
> > So, I'd prefer not to block the creation of Context on langneg (because I'm
> > worried we'll later start introducing pseudo contexts until the real one is
> > called).
> 
> You wouldn't have to.  You could do Context(id, defLoc, allLocs) which
> doesn't do any lang nego yet.  Then a call to
> ctx.requestLocales(userPrefLocs) would trigger the language negotiation.

But for that I need to get defLoc and allLoc before I can create new context.

I'd prefer to have context available "from the scratch" and registerLocales when manifest is loaded.
Comment 9 User image Staś Małolepszy :stas 2014-06-18 09:15:35 PDT
(In reply to Zibi Braniecki [:gandalf] from comment #6)

> So, the understanding I got from our conversation and previous attempts to
> define the HTML API is that we want to separate two things:
> 
> 1) The list of resources required to make the webapp work (this is the list
> that will be sent to LangPackService)
> 2) The list of resources required to be loaded at the startup.
> 
> The former list will be added to manifest file. I didn't add it there yet,
> because there's no use case for this and the JSON is easily expandable.
> 
> The latter is defined in <head> of the document much like CSS links are.

I like the general idea of declaring #2 in <head> via <link> elements, although I have to admit that I don't know what the semantics would be. If it's like CSS, would you localize the whole document when a new link is added? Would you use rel=prefetch and an onload handler?

Another approach that I'd consider is defining #2 as 'resources' in the manifest, and #1 as the sum of 'resources' and 'extra_resources' (name TBD).

Would you require the manifest in order to make the app compatible with the language pack service (LPS)?  Or would there be a path of least resistance which makes the app operate in a single locale mode when LPS is not available, but is compatible with it if it is?  For instance, defining <link rel="…" type="…" href="./en-US.resource"> without any manifest could be enough to run the app in the single locale mode, but could also make it localizable via the LPS.


(In reply to Zibi Braniecki [:gandalf] from comment #8)

> But for that I need to get defLoc and allLoc before I can create new context.

You mentioned being blocke dby language negotiation, so my reply was specifically about that.  However, you're right, we'd still be blocked by the manifest loading.

> I'd prefer to have context available "from the scratch" and registerLocales
> when manifest is loaded.

Yeah, this makes sense.  This way you can have the default context available early, even when the manifest hasn't loaded yet.
Comment 10 User image Staś Małolepszy :stas 2014-06-18 09:16:44 PDT
Comment on attachment 8440965 [details] [diff] [review]
patch, v1, l20n part

Review of attachment 8440965 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is generally a good direction, but let's discuss the exact semantics of the manifest especially wrt. language pack services as well as link types and rels.
Comment 11 User image Zibi Braniecki [:gandalf][:zibi] 2014-06-18 17:28:03 PDT
Created attachment 8442511 [details] [diff] [review]
patch v2, l20n part
Comment 12 User image Zibi Braniecki [:gandalf][:zibi] 2014-06-18 17:37:16 PDT
Created attachment 8442514 [details] [diff] [review]
patch v2, gaia part

I applied your feedback and introduced ctx.registerLocales.

I'd like you now to look at the gaia part of my patch. There are some BTO changes regarding INI->manifest files.

With this patch I was able to launch an app on the device using the manifest.

STR:

1) make reset-gaia
2) apply the patch
3) apply this patch https://pastebin.mozilla.org/5432536
4) add apps/sms/locales/locales.manifest
4) make install-gaia APP=sms

b2gperf with 62 iterations, median:

master: 1469
manifest: 1472

Seems to be a good start!

The biggest issue I see here, is that extending manifest files to cover qps-ploc and qps-plocm is tricky because currently modifications to manifest happen in multilocale.js which is fired only when LOCALEBASE_DIR is set.
I'd appreciate recommendations on how to make it work :)
Comment 13 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-02 13:27:05 PDT
Created attachment 8449695 [details] [diff] [review]
patch, l10n part

Updated patch against l10n repo
Comment 14 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-02 17:47:06 PDT
Created attachment 8449887 [details] [diff] [review]
patch, gaia part

This is gaia part of the equation.

Most of the changes are small, but one major is a substantial amount of changes in multilocale.js.
Comment 15 User image Staś Małolepszy :stas 2014-07-03 03:56:01 PDT
Comment on attachment 8449695 [details] [diff] [review]
patch, l10n part

Review of attachment 8449695 [details] [diff] [review]:
-----------------------------------------------------------------

I think we can write this code slightly different in a way that makes it clearer and less prone to race conditions (sorry, I know I've been using that word a lot, but I do thikn it's important).  r-, please see comments.

I'll have a look at the gaia part later today.  You may also want to get early feedback from Yuren.

::: bindings/l20n/runtime.js
@@ +11,5 @@
>  var isPretranslated = false;
>  var rtlList = ['ar', 'he', 'fa', 'ps', 'qps-plocm', 'ur'];
>  var nodeObserver = false;
> +var headScanCompleted = false;
> +var manifestLoaded = false;

State flags make it hard to understand what's going on and how the code flows.  Potentially, they introduce risks of race conditions.  Can you try to rewrite this part without using headScanCompleted and manifestLoaded?

One way to achieve this would be to have a separate querySelectorAll('link[rel="localization"]) just for resLinks which is run first.

@@ +179,5 @@
> +  var nodes = document.head
> +                      .querySelectorAll('link[rel="localization"],' +
> +                                        'link[rel="manifest"],' +
> +                                        'meta[name="locales"],' +
> +                                        'meta[name="default_locale"]');

How should we react to both link[rel="manifest"] and metas present in head?  Right now, if I read the code right, we'll registerLocales twice, but we might requestLocales before the second registerLocale happens, so the second one will not always override the first one.

Maybe we should give metas precedence?  If the developer makes the effort to inline the locale information, I would assume she wants us to use it instead of the manifest.  It also would work as an IO optimization.

@@ +204,5 @@
> +  var url = node.getAttribute('href');
> +  var rel = node.getAttribute('rel');
> +  switch (rel) {
> +    case 'manifest':
> +      loadManifest.call(this, url, parseManifest.bind(this));

Maybe simply inline | io.loadJSON | here with a callback to parseManifest which handles the error?
Comment 16 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-03 09:22:39 PDT
(In reply to Staś Małolepszy :stas from comment #15)
> I think we can write this code slightly different in a way that makes it
> clearer and less prone to race conditions (sorry, I know I've been using
> that word a lot, but I do thikn it's important).  r-, please see comments.

So, I'm happy you push for this. What worries me is how much you're willing to increase code complexity to achieve it ;)

> I'll have a look at the gaia part later today.  You may also want to get
> early feedback from Yuren.

Yes, right after your f, I'll ask Yuren.

> ::: bindings/l20n/runtime.js
> @@ +11,5 @@
> >  var isPretranslated = false;
> >  var rtlList = ['ar', 'he', 'fa', 'ps', 'qps-plocm', 'ur'];
> >  var nodeObserver = false;
> > +var headScanCompleted = false;
> > +var manifestLoaded = false;
> 
> State flags make it hard to understand what's going on and how the code
> flows.  Potentially, they introduce risks of race conditions.  Can you try
> to rewrite this part without using headScanCompleted and manifestLoaded?

I don't think it would be a good choice to rewrite it without the semaphores.

The code flow is racy (as in - the nodes in head can be set in any order) and eventually I want this code to be launched before parsing and get the nodes from parser to Mutation Observer.
And this means that the code has to react to the nodes as they come.

I of course can rewrite the way we guard on those two semaphores to make it more robust, but I believe that they are two boolean representations of two states that do exist in the code and we want to launch the next stage of booting once both of them are fulfilled, so I want to represent this in form of two flags.
 
> One way to achieve this would be to have a separate
> querySelectorAll('link[rel="localization"]) just for resLinks which is run
> first.

querySelector code in content's nsINode is by many orders of magnitude more complex than two boolean semaphores. So even if we did not aim to move the whole loading to MO, I'd be against introducing another querySelectorAll to mitigate the need of two flags.

> How should we react to both link[rel="manifest"] and metas present in head? 
> Right now, if I read the code right, we'll registerLocales twice, but we
> might requestLocales before the second registerLocale happens, so the second
> one will not always override the first one.

That's a good point. I'd say that it is a racy condition because the developer provided two (possibly different) manifest's data.
 
> Maybe we should give metas precedence?  If the developer makes the effort to
> inline the locale information, I would assume she wants us to use it instead
> of the manifest.  It also would work as an IO optimization.

I agree. The developer may also want to preserve the <link> to manifest for other purposes.

But because of what I described above - scenario where parser feeds MO with nodes - I believe we should load the <link> and just make sure any <meta> overrides its values.

Does that sound good?
Comment 17 User image Staś Małolepszy :stas 2014-07-07 12:00:00 PDT
(In reply to Zibi Braniecki [:gandalf] from comment #16)

> The code flow is racy (as in - the nodes in head can be set in any order)
> and eventually I want this code to be launched before parsing and get the
> nodes from parser to Mutation Observer.
> And this means that the code has to react to the nodes as they come.

OK, I see what you mean.  Let's leave headScanComplete as a temporary solution.  In the future, in the scenario were we're reacting to new <link> nodes we won't need headScanComplete, because there will be no single cut-off point to indicate the moment to start loading resources.

As for the manifestLoaded flag: maybe check if Object.keys(manifest) == 2 instead of keeping track of a boolean flag?

> > Maybe we should give metas precedence?  If the developer makes the effort to
> > inline the locale information, I would assume she wants us to use it instead
> > of the manifest.  It also would work as an IO optimization.
> 
> I agree. The developer may also want to preserve the <link> to manifest for
> other purposes.
> 
> But because of what I described above - scenario where parser feeds MO with
> nodes - I believe we should load the <link> and just make sure any <meta>
> overrides its values.
> 
> Does that sound good?

I think it depends, actually.  Metas should override the manifest, but there are multiple permutations of the order of elements in head to consider:

  l10n.js script, manifest link, metas
  manifest, l10n.js, metas
  manifest, metas, l10n.js

  l10n.js script, metas, manifest
  metas, l10n.js, manifest
  metas, manifest, l10n.js

The exact position of the l10n.js script will be important for when we have a mutation observer watching <head>, so for now, we can probably simplify this to:

  manifest, metas
  metas, manifest

I would suggest to not event fetch the manifest if we already have both of the meta elements.
Comment 18 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-07 16:12:12 PDT
Created attachment 8452006 [details] [diff] [review]
patch, v2, l10n part

I updated the l10n patch and removed manifestLoaded.

I actually went a different route with loadManifest/parseManifest because I want to make sure that link does not overload any of the metas.

I'd like to document bindings once I land bug 994370.
Comment 19 User image Staś Małolepszy :stas 2014-07-08 07:46:35 PDT
Comment on attachment 8452006 [details] [diff] [review]
patch, v2, l10n part

Review of attachment 8452006 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments below. Thanks!

::: bindings/l20n/runtime.js
@@ +217,2 @@
>      return;
>    }

Metas should be allowed to override themselves if ore than one of a given type is present.  Remove this check?

@@ +236,5 @@
> +  if (Object.keys(manifest).length === 2) {
> +    return;
> +  }
> +
> +  io.loadJSON(url, function(err, json) {

Call this function parseManifest?

@@ +260,5 @@
> +function parseManifest() {
> +  this.ctx.registerLocales(manifest.default_locale,
> +                           manifest.locales);
> +  if (headScanCompleted) {
> +    initLocale.call(this);

It doesn't look like this function does any parsing despite its name.  I'd rename it or better yet, since registerLocales is synchronous right now, I'd move it to the first line of initLocale, remove parseManifest altogether, and use if (headScanCompleted) initLocale.call(this); instead.
Comment 20 User image Staś Małolepszy :stas 2014-07-09 08:13:39 PDT
Comment on attachment 8449887 [details] [diff] [review]
patch, gaia part

Review of attachment 8449887 [details] [diff] [review]:
-----------------------------------------------------------------

I applied this patch on top of master and also checked out your branch at https://github.com/zbraniecki/gaia/commits/1022767-use-json-l10n-manifests, but in both cases I wasn't able to build Gaia with:

  make reset-gaia LOCALES_FILE=locales/languages_all.json LOCALE_BASEDIR=locales

Here's the tail of the output:

run-js-command gaia/post-app
[modified]
[modified]
[modified]
[modified]
[modified]
[modified]
[modified]
Exception: [Exception... "Component returned failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsIFile.remove]"  nsresult: "0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> file:///srv/moz/gaia/build/multilocale.js :: cleanLocaleFiles :: line 46"  data: no]
undefined
make: *** [post-app] Error 3

Would you mind updating the patch and re-requesting feedback, please?
Comment 21 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-09 12:11:12 PDT
Created attachment 8453239 [details] [diff] [review]
patch, gaia part

So, until I get f+ I didn't want to migrate all the apps (maintaining such a patch would be a PITA).

I migrated only the Settings app.

In order to test the patch you need to:

1) switch to master (rev. 4e4e579 is what I test against today)
2) make reset-gaia LOCALES_FILE=locales/languages_my.json LOCALE_BASEDIR=../gaia-l10n/
3) switch to my branch or apply the patch
4) make install-gaia APP=settings LOCALES_FILE=locales/languages_my.json LOCALE_BASEDIR=../gaia-l10n/

This will give you master Gaia with Settings app updated to the new scheme. From there, you can freely play with params for install-gaia as long as you keep APP=settings.

Rerequesting feedback :)
Comment 22 User image Staś Małolepszy :stas 2014-07-11 06:29:59 PDT
Comment on attachment 8453239 [details] [diff] [review]
patch, gaia part

Review of attachment 8453239 [details] [diff] [review]:
-----------------------------------------------------------------

f=me, thanks for spearheading this!  I don't feel comfortable giving feedback on the multilocale part, but from what I could gather, it looks okay and works as expected on the device.  I tested your branch with GAIA_{INLINE,CONCAT}_LOCALES={0,1} and everything was fine.

::: build/webapp-optimize.js
@@ +266,5 @@
>   */
>  HTMLOptimizer.prototype.concatL10nResources = function() {
>    var doc = this.win.document;
> +  var links = doc.querySelectorAll('link[rel="localization"], ' +
> +                                   'link[rel="manifest"]');

I wonder if we should also support a third type/rel here, dedicated to just defining l10n settings.

Consider this scenario with the current patch:  if you don't want your website to have an Open Web App manifest, we're still forcing you to have the following link:

  <link rel="manifest" href="/manifest.webapp">

In there, you didn't want to define all the required fields for a valid Open Web App manifest, and you only specified locales and default_locale.

If user agents start trying to autodiscover manifests for webapps, they might choose to look for this exact link rel as above, and then throw errors at the user since the manifest is not valid.

A solution could be:

 - first, we look for link rel=localization-manifest, which can only contain the locales and default_locale settings,
 - if not found, we look for link rel=manifest, which is the real Open Web App manifest, which should also include the locales and default_locale settings.

An alternative would be to only support linked manifest for webapps, and force the use of metas for all other pages.

@@ +288,5 @@
> +        defaultLocaleMeta.content = manifest.default_locale;
> +
> +        let localesMeta = doc.createElement('meta');
> +        localesMeta.name = 'locales';
> +        localesMeta.content = Object.keys(manifest.locales).join(', ');

Maybe join with " ", to imitate HTML's class attribute syntax?

@@ +309,5 @@
> +    jsonLink.rel = 'localization';
> +    parentNode.insertBefore(jsonLink, links[0]);
> +  }
> +  for (i = 0; i < links.length; i++) {
> +    parentNode.removeChild(links[i]); 

nit: trailing whitespace (also in a few other places)
Comment 23 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-11 10:29:38 PDT
(In reply to Staś Małolepszy :stas from comment #22)
> ::: build/webapp-optimize.js
> @@ +266,5 @@
> >   */
> >  HTMLOptimizer.prototype.concatL10nResources = function() {
> >    var doc = this.win.document;
> > +  var links = doc.querySelectorAll('link[rel="localization"], ' +
> > +                                   'link[rel="manifest"]');
> 
> I wonder if we should also support a third type/rel here, dedicated to just
> defining l10n settings.

Ugh... I like how we avoid introducing another type of manifest.

> Consider this scenario with the current patch:  if you don't want your
> website to have an Open Web App manifest, we're still forcing you to have
> the following link:
> 
>   <link rel="manifest" href="/manifest.webapp">
> 
> In there, you didn't want to define all the required fields for a valid Open
> Web App manifest, and you only specified locales and default_locale.

In such cases I think I'd prefer to recomment <meta> route then introduce new manifest file. That will not be needed in Gaia, so I'd suggest not adding new manifest until we see a need for it.

The proposed scheme allows for adding it later, right?

> @@ +288,5 @@
> > +        defaultLocaleMeta.content = manifest.default_locale;
> > +
> > +        let localesMeta = doc.createElement('meta');
> > +        localesMeta.name = 'locales';
> > +        localesMeta.content = Object.keys(manifest.locales).join(', ');
> 
> Maybe join with " ", to imitate HTML's class attribute syntax?

Hmm, I'd like to get more opinions. Let's see what Vivien will say :)
Comment 24 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-11 10:31:12 PDT
Comment on attachment 8453239 [details] [diff] [review]
patch, gaia part

Review of attachment 8453239 [details] [diff] [review]:
-----------------------------------------------------------------

Yuren, can you take a look at this patch and tell me how it looks to you? I'm mostly removing all .ini related stuff from multilocale.js

I don't consider my patch to be finished, but would like to get your feedback before I start finalizing and adding tests.
Comment 25 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-11 10:32:44 PDT
Comment on attachment 8453239 [details] [diff] [review]
patch, gaia part

Vivien, can you take a look at this approach and let me know what you think?

We basically replace .INI files with our manifest.webapp or two <meta> nodes with the keys we need - default_locale and locales.
Comment 26 User image Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode) 2014-07-14 03:00:44 PDT
trying to understand l20n.

what does <meta> tag for l20n looks like? I can't find document for it in l20n's repository[1]

[1] https://github.com/l20n/l20n.js/blob/master/docs/html.md
Comment 27 User image Staś Małolepszy :stas 2014-07-14 07:41:00 PDT
(In reply to Zibi Braniecki [:gandalf] from comment #23)

> Ugh... I like how we avoid introducing another type of manifest.

Yes, me too.  I just want to make sure we're on the same page here.  For non-webapps, we should promote meta elements as the canonical way to provide the information required for L20n to work.

> > Maybe join with " ", to imitate HTML's class attribute syntax?
> 
> Hmm, I'd like to get more opinions. Let's see what Vivien will say :)

What are the arguments for having commas in there?
Comment 28 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-14 08:45:56 PDT
(In reply to Yuren [:yurenju] from comment #26)
> trying to understand l20n.
> 
> what does <meta> tag for l20n looks like? I can't find document for it in
> l20n's repository[1]
> 
> [1] https://github.com/l20n/l20n.js/blob/master/docs/html.md

L20n (master) does not support <meta> tags for l10n metadata yet. We got to the point where we develop L20n mainly in `gaia` branch and we hope to get to the stage where we will merge master and gaia into one in the next months.

In case the developer uses meta or we use our BTO to inline l10n relevant information in the document using meta, it'll look like this:

<meta name="default_locale" content="en-US">
<meta name="locales" content="fr, it, en-US">

(or, without commas - stas' preference)
Comment 29 User image Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode) 2014-07-16 02:12:27 PDT
Comment on attachment 8453239 [details] [diff] [review]
patch, gaia part

finally I read all of this patch. patch looks good and we can process multilocale easiler without *.ini files.

as my imagination, it will be a really big patch to migrate all l10n work from ini to {locale} mechanism in html tag. do you have any migration plan to land this huge change? can we separate it to different bugs for lower regression risk?
Comment 30 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-16 09:18:30 PDT
(In reply to Yuren [:yurenju] from comment #29)
> as my imagination, it will be a really big patch to migrate all l10n work
> from ini to {locale} mechanism in html tag. do you have any migration plan
> to land this huge change? can we separate it to different bugs for lower
> regression risk?

I would prefer to land it all at once so that we don't have to support two different mechanisms (they are substantially different) and I don't expect the switch to be that massive overall - in almost all cases it's a matter of one .ini -> one URL Template.

Once I get a feedback from Vivien, I'd like to make an attempt to write such patch - if it'll be too big, I will fall back on getting manifest support next to ini and we'll migrate one by one. I'd still like to complete the migration within one cycle.
Comment 31 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-16 09:22:33 PDT
There are 51 l10n ini files in gaia, four of them have more than one URL template. Very straightforward transition.
Comment 32 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-18 14:53:44 PDT
We should also track this: https://github.com/w3c/manifest/issues/211
Comment 33 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-24 13:55:12 PDT
After a conversation with Pike, I decided to go for two patches here. First one will move l10n.js and ./build/ to use manifests, second will move all the gaia apps.

We will land both patches at once.
Comment 34 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-24 14:02:24 PDT
I updated my branch to master.
Comment 35 User image Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2014-08-04 07:43:45 PDT
Using the manifest of the app sounds good to me.
Comment 36 User image Zibi Braniecki [:gandalf][:zibi] 2014-08-27 17:17:21 PDT
Just an update, since there's a lot of going on between Stas and me around this bug, and more people may be interested:

 - Stas is currently reviewing the patch
 - I managed to get TBPL to produce green build
 - I also incorporated the patch for bug 1055328 here

If the review will go well, we intend to land it for 2.1 FC.
Comment 37 User image Zibi Braniecki [:gandalf][:zibi] 2014-08-29 02:05:43 PDT
Created attachment 8481158 [details] [review]
pull request
Comment 39 User image Axel Hecht [:Pike] 2014-08-29 07:46:26 PDT
Some things I found while inspecting a built tree:

https://github.com/mozilla-b2g/gaia/blob/2d16dbb66bc1acdf759e3ad43e5ac9c4c9f9aa2c/build/multilocale.js#L230 resets the entrypoint l10n for each locale, so the built app just ends up with zulu.

The en-US files should be picked up from the gaia repo, and not from gaia-l10n/en-US (as they're after the patch as is). The en-US repo might not exist, or be out of date.

Files like the contacts strings don't end up in the json file, haven't spotted why that is.
Comment 40 User image Staś Małolepszy :stas 2014-08-29 07:48:46 PDT
After the discussion in #gaia and on dev-gaia, I think the best way forward is to back this out for now.  It's clear that it would be preferable to land a change of this format after we branch.  I'm sorry for the trouble this has caused to folks trying to get their work done before the FL.
Comment 41 User image Francesco Lodolo [:flod] 2014-08-29 08:13:12 PDT
(In reply to Axel Hecht [:Pike] from comment #39)
> The en-US files should be picked up from the gaia repo, and not from
> gaia-l10n/en-US (as they're after the patch as is). The en-US repo might not
> exist, or be out of date.

That's the issue we spent hours debugging. Both you and Stas have all locales cloned in /locales, I only have one (never en-US).
Comment 43 User image Staś Małolepszy :stas 2014-08-29 08:34:19 PDT
Thanks, Rik.  Reverted in L20n.js repo as well:

https://github.com/l20n/l20n.js/commit/10c1855238251e6aac267f16fa0ba232aece1e3e
Comment 44 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-01 01:38:36 PDT
(In reply to Axel Hecht [:Pike] from comment #39)
> Some things I found while inspecting a built tree:
> 
> https://github.com/mozilla-b2g/gaia/blob/
> 2d16dbb66bc1acdf759e3ad43e5ac9c4c9f9aa2c/build/multilocale.js#L230 resets
> the entrypoint l10n for each locale, so the built app just ends up with zulu.

I started investigating, but the whole part of multilocale which deals with manifests required a major update.

> The en-US files should be picked up from the gaia repo, and not from
> gaia-l10n/en-US (as they're after the patch as is). The en-US repo might not
> exist, or be out of date.

It fixes this.

> Files like the contacts strings don't end up in the json file, haven't
> spotted why that is.

Fixed that.

> Blocks: 1060384

Interestingly, I can't reproduce it on any other locale than Pseudolocale (and it happens on master too!). I was planning to report it, but if we can confirm that it doesn't happen with the patch, I'll transform that bug into one about pseudolocale.

Overall, all the bugs found so far were related to multilocale not producing the right manifest.webapp. I refactored the code around it, and will add tests once I get reviews.

First round is from Stas and Axel, then from Yuren.
Comment 45 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-01 01:43:17 PDT
Comment on attachment 8481158 [details] [review]
pull request

Updated pull request:
 - rebased on top of master
 - refactored another part of multilocale to:
   - take en-US from gaia dir, not from locale basedir
   - take branding from gaia dir en-US if official==1
   - populate the keys in en-US if l10n is missing
   - prepare the codebase for new structure where l10n repos will live as submodules in apps
   - prepare the codebase for l20n resources

I'd like to make sure that we agree on how multilocale should work here, before I write tests.

p.s. One thing I'd like to consider in the future is enabling multilocale irrelevant of LOCALE_BASEDIR. There's a set of operations that happen in multilocale that should work even in eng. builds (like, official branding from en-US or deviceType).
Comment 46 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-01 01:44:37 PDT
Oh, I kept the changes compared to the patch that I attempted to land on Thursday as a separate patch for now, for your convenience.

I put r? on pull request because it's easy to see the separate patch as well as the combined changes to multilocale there. I doubt it makes sense to look at the regular diff for that file.
Comment 47 User image Staś Małolepszy :stas 2014-09-01 04:27:03 PDT
Created attachment 8482230 [details] [review]
new pull request

The attachement linked to PR #23331 which was the old pull request which was backed out.  The new pull request is https://github.com/mozilla-b2g/gaia/pull/23549.
Comment 48 User image Staś Małolepszy :stas 2014-09-01 05:13:47 PDT
(In reply to Zibi Braniecki [:gandalf] from comment #44)

> Interestingly, I can't reproduce it on any other locale than Pseudolocale
> (and it happens on master too!). I was planning to report it, but if we can
> confirm that it doesn't happen with the patch, I'll transform that bug into
> one about pseudolocale.

Good catch!  I think it's a regression from bug 1012702, I'll take care of it.
Comment 49 User image Staś Małolepszy :stas 2014-09-02 06:34:12 PDT
Comment on attachment 8482230 [details] [review]
new pull request

Gandalf, I think you might have pushed an older version of the first commit to your branch.  It does not include the changes that I proposed in my first review (removing semaphores).

I'm giving this an r- because the isBranding flag in build/multilocale.js needs to be reset in every iteration of the loop, for each resource file separately.  Otherwise, it stays as 'true' for every file defined below a link to a branding resource.  I fixed this on my branch [1] although I think I'd prefer a solution which doesn't use isBranding altogether.  Shouldn't realURL already be the real URL?

TBPL shows a Gb bustage with your patch, likely related to bug 1055328, and a Li failure.

Otherwise it looks good :)

[1] https://github.com/stasm/gaia/tree/1022767-manifests
Comment 50 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-02 14:20:51 PDT
Comment on attachment 8482230 [details] [review]
new pull request

Updated the patch to incorporate Stas's feedback. Rerequesting r?
Comment 51 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-02 15:46:59 PDT
Comment on attachment 8482230 [details] [review]
new pull request

Yuren, Pike and stas are still working on the review, but I believe we all agree that the buildsystem part should work the way it does in this PR.

Can you review it from the build system standpoint?
Comment 52 User image Staś Małolepszy :stas 2014-09-03 08:22:10 PDT
Comment on attachment 8482230 [details] [review]
new pull request

r=me with a few tiny nits which I discussed with Gandalf in github and otp.
Comment 53 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-03 17:15:25 PDT
thanks! feedback addressed and the build is green
Comment 54 User image Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode) 2014-09-03 21:15:42 PDT
Comment on attachment 8482230 [details] [review]
new pull request

gandalf,

Thank you for refactoring multilocale, now its readability is better that before. there are some check points you should do before landing:

1. check if communications apps works as expected since it has sub folder for entry points like dialer and contacts.
2. check everythin works well if GAIA_INLINE_LOCALES=0 and GAIA_CONCAT_LOCALES=0 are set (this is covered by integration test)

the pull request looks good and I left some comments for enhacement. because that is a really big change for multilocale module so I would love to review again for your next change.

marked as sr- just to remind us it should be reviewed again since we have another r+.
Comment 55 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-04 01:33:40 PDT
Comment on attachment 8482230 [details] [review]
new pull request

Thanks a lot Yuren! I applied your feedback. Requesting review :)
Comment 56 User image Axel Hecht [:Pike] 2014-09-04 09:44:21 PDT
I'm leaving some comments on the PR, but also some here:

I'm wondering if one can trigger a race between meta and manifest in shared/js/l10n.js? Or get them to register both?
Comment 57 User image Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode) 2014-09-04 19:25:56 PDT
Comment on attachment 8482230 [details] [review]
new pull request

that is great, thank you! r=yurenju
Comment 58 User image Axel Hecht [:Pike] 2014-09-05 07:15:43 PDT
Comment on attachment 8482230 [details] [review]
new pull request

I think I'm through most of the patch, there are still a few things I'm concerned about.

For one, the requirement to add localizable strings to the manifest just for book keeping still annoys me. I entertained the idea to just do

"locales": {"en-US":{}}

to annotate the "fill me out, but don't bother about the details". That'll require an update to the string extraction automation, though.

I'm also not sure how that /shared/pages/import manifest plays out in practice. Please make sure that's tested in detail, through all the various bits and pieces that we can switch on and off.

I also found a bunch of edits to the manifest.webapp files which claim language support of the app as in the gaia repo, but that language isn't actually supported. Commented on those in the PR.
Comment 59 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-05 15:38:00 PDT
Comment on attachment 8482230 [details] [review]
new pull request

(In reply to Axel Hecht [:Pike] from comment #58)
> Comment on attachment 8482230 [details] [review]
> new pull request
> 
> I think I'm through most of the patch, there are still a few things I'm
> concerned about.

Thanks Pike! Updated the PR.

> For one, the requirement to add localizable strings to the manifest just for
> book keeping still annoys me. I entertained the idea to just do
> 
> "locales": {"en-US":{}}
> 
> to annotate the "fill me out, but don't bother about the details". That'll
> require an update to the string extraction automation, though.

I'll be happy to address this in a follow up bug. I'd prefer not to add more features to this one.

> I'm also not sure how that /shared/pages/import manifest plays out in
> practice. Please make sure that's tested in detail, through all the various
> bits and pieces that we can switch on and off.

I tested it left and right. It seems to work well and is needed because we need a manifest.webapp to provide info to l10n.js about available locales and this manifest has to live in shared because the HTML files for it are called in multiple different apps. So the manifest basically describe the iframe and resources that live in shared/locales/facebook

> I also found a bunch of edits to the manifest.webapp files which claim
> language support of the app as in the gaia repo, but that language isn't
> actually supported. Commented on those in the PR.

Fixed!

Re-requesting r?
Comment 60 User image Axel Hecht [:Pike] 2014-09-10 06:13:55 PDT
Comment on attachment 8482230 [details] [review]
new pull request

The l10n automation can now ignore {} as a value for "locales":{"en-US":{}}, and I'd like us to do that for the manifests that just have their locales entry for bookkeeping.

I'm blocking on that, as I don't want to waste a few dozens people's time on unused strings. Also, this might require changes to the code, probably.
Comment 61 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-10 19:09:29 PDT
Comment on attachment 8482230 [details] [review]
new pull request

I moved all manifest.webapp entries that we add in order to have available locales list to empty objects. I kept is as a separate commit to make it easier for you to review :)

I didn't have to change any code for that.

I also rebased on top of the current master and resolved merge conflicts so we have a build that is as green as green goes.

Re-requesting review from Axel :)
Comment 62 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-11 10:34:08 PDT
I thought about our last conversation a bit more, and added another patch. Because I'm totally not confident in how AppManager is dealing with missing name/desc in case locales key is present in the manifest (see bug 1060384 comment 5), I prefer to build manifests that contain values for locale keys.

The patch 
 - changes the behavior only in the scenario where locales[ab-CD] is an empty object. It takes name/description from root key and applies it to all locales.
 - stops attempting to download manifest file in case GAIA_SOURCE_LOCALE key is empty
 - reports missing manifest files
Comment 64 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-16 09:46:04 PDT
L20n.js: https://github.com/l20n/l20n.js/commit/9324143105fa4ace1bae84dc136dbffd5742d03f
Comment 65 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-16 10:48:20 PDT
Created attachment 8490190 [details] [review]
PR, fix relative path for device_type

There's a minor regression discovered by :flod. With the change to isSubjectToDeviceType, we shouldn't append the type in a loop.

This patch changes the relative path building to only append device_type or branding once.
Comment 66 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-16 16:25:17 PDT
Filed two follow ups - bug 1068380 and bug 1068382
Comment 67 User image Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode) 2014-09-18 17:37:51 PDT
Comment on attachment 8490190 [details] [review]
PR, fix relative path for device_type

looks good but conflict now. since it is a minor fix I would like to give r+ first and please rebase to land it.
Comment 68 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-18 19:07:26 PDT
Commit: https://github.com/mozilla-b2g/gaia/commit/b8065b66b4f8c364338c05959a724ad74d2c40c7
Merge: https://github.com/mozilla-b2g/gaia/commit/b7a5f2eaa929365a04ba7053b3485d54f7811381

Sweet! Thank you guys for reviews and feedback!
Comment 69 User image Sherman Chen [:chens] (inactive) 2014-09-18 19:28:02 PDT
I found this patch couldn't possibly get branding properties. Once you move the logic outside the loop, you may also need to fix regex for |isSubjectToBranding|.

Currently branding regex is |shared[\/\\]?[a-zA-Z]*[\/\\]?branding$|, maybe should be |shared[\/\\]?[a-zA-Z]*[\/\\]?brandind| when we move out from the loop.
Comment 70 User image Zibi Braniecki [:gandalf][:zibi] 2014-09-18 20:51:15 PDT
Keeping NI on myself to look into this tomorrow, but Sherman, can you provide STR? 

I just tested latest master with LOCALE_BASEDIR and I have branding in Settings.
Comment 71 User image Sherman Chen [:chens] (inactive) 2014-09-18 22:47:17 PDT
Sorry I was looking on an outdated patch, and the latest one have no problem on branding strings.
Comment 72 User image Sherman Chen [:chens] (inactive) 2014-09-22 06:08:18 PDT
*** Bug 1065175 has been marked as a duplicate of this bug. ***
Comment 73 User image Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2014-10-06 05:56:28 PDT
Will this be uplifted to 2.1 or will it be fixed in a different manner?
Comment 74 User image Francesco Lodolo [:flod] 2014-10-06 06:25:31 PDT
(In reply to Archaeopteryx [:aryx] from comment #73)
> Will this be uplifted to 2.1 or will it be fixed in a different manner?

This is not the bug you're looking for.
That would be bug 1074166 (has a patch, reviewed, waiting for approval).
Comment 75 User image Zibi Braniecki [:gandalf][:zibi] 2014-12-26 17:00:39 PST
Just a heads up. Long conversations with W3C Manifest group convinced me and stas that manifest.webapp should not be on a critical startup path at all.

So we will want to standardize around the solution that we use on production builds which is to use <meta> keys for this data instead of adding it to manifest.webapp (See bug 1115807)

That's a minor change that should not affect any of you, but may be important for people who followed this bug.

Note You need to log in before you can comment on or make changes to this bug.