The default bug view has changed. See this FAQ.

Use WebApp's manifests instead of locales.ini

RESOLVED FIXED

Status

Firefox OS
Gaia::L10n
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: stas, Assigned: gandalf)

Tracking

(Blocks: 5 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 7 obsolete attachments)

17.23 KB, patch
stas
: review+
Details | Diff | Splinter Review
33.73 KB, patch
stas
: feedback+
yurenju
: feedback+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
stas
: review+
yurenju
: review+
Pike
: review+
Pike
: feedback+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
yurenju
: review+
Details | Review | Splinter Review
(Reporter)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → stas
Priority: -- → P2
(Assignee)

Updated

3 years ago
Blocks: 809600
(Assignee)

Updated

3 years ago
Blocks: 1005965
(Assignee)

Comment 1

3 years ago
Stas, if you dont plan to work on that today, I'lk be happy to take it as my in-flight project.
(Reporter)

Comment 2

3 years ago
Go for it :)
(Assignee)

Comment 3

3 years ago
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.
Assignee: stas → gandalf
Status: NEW → ASSIGNED
Attachment #8440965 - Flags: feedback?(stas)
(Assignee)

Comment 4

3 years ago
Created attachment 8440968 [details] [diff] [review]
patch, v1, gaia part

This is just for reference to the first patch. Corresponding gaia changes.
(Reporter)

Comment 5

3 years ago
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);
(Assignee)

Comment 6

3 years ago
(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.
(Reporter)

Comment 7

3 years ago
(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.
(Assignee)

Comment 8

3 years ago
(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.
(Reporter)

Comment 9

3 years ago
(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.
(Reporter)

Comment 10

3 years ago
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.
Attachment #8440965 - Flags: feedback?(stas) → feedback+
(Assignee)

Comment 11

3 years ago
Created attachment 8442511 [details] [diff] [review]
patch v2, l20n part
Attachment #8440968 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
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 :)
Attachment #8440965 - Attachment is obsolete: true
Attachment #8442514 - Flags: feedback?(stas)
(Assignee)

Updated

3 years ago
Attachment #8442511 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8442514 - Attachment is obsolete: true
Attachment #8442514 - Flags: feedback?(stas)
(Assignee)

Comment 13

3 years ago
Created attachment 8449695 [details] [diff] [review]
patch, l10n part

Updated patch against l10n repo
Attachment #8449695 - Flags: review?(stas)
(Assignee)

Comment 14

3 years ago
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.
Attachment #8449887 - Flags: feedback?(stas)
(Reporter)

Comment 15

3 years ago
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?
Attachment #8449695 - Flags: review?(stas) → review-
(Assignee)

Comment 16

3 years ago
(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?
Flags: needinfo?(stas)
(Reporter)

Comment 17

3 years ago
(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.
Flags: needinfo?(stas)
(Assignee)

Comment 18

3 years ago
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.
Attachment #8449695 - Attachment is obsolete: true
Attachment #8452006 - Flags: review?(stas)
(Reporter)

Comment 19

3 years ago
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.
Attachment #8452006 - Flags: review?(stas) → review+
(Reporter)

Comment 20

3 years ago
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?
Attachment #8449887 - Flags: feedback?(stas)
(Assignee)

Updated

3 years ago
Blocks: 999778
(Assignee)

Updated

3 years ago
Blocks: 999779
(Assignee)

Updated

3 years ago
Blocks: 1030035
(Assignee)

Comment 21

3 years ago
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 :)
Attachment #8449887 - Attachment is obsolete: true
Attachment #8453239 - Flags: feedback?(stas)
(Assignee)

Updated

3 years ago
Summary: Use L20n's manifests instead of locales.ini → Use WebApp's manifests instead of locales.ini
(Reporter)

Updated

3 years ago
Attachment #8453239 - Attachment is patch: true
Attachment #8453239 - Attachment mime type: text/x-patch → text/plain
(Reporter)

Comment 22

3 years ago
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)
Attachment #8453239 - Flags: feedback?(stas) → feedback+
(Assignee)

Comment 23

3 years ago
(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 :)
(Assignee)

Comment 24

3 years ago
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.
Attachment #8453239 - Flags: feedback?(yurenju.mozilla)
(Assignee)

Comment 25

3 years ago
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.
Attachment #8453239 - Flags: feedback?(21)
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
Flags: needinfo?(gandalf)
(Reporter)

Comment 27

3 years ago
(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?
(Assignee)

Comment 28

3 years ago
(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)
Flags: needinfo?(gandalf)
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?
Attachment #8453239 - Flags: feedback?(yurenju.mozilla) → feedback+
Flags: needinfo?(gandalf)
(Assignee)

Comment 30

3 years ago
(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.
Flags: needinfo?(gandalf)
(Assignee)

Comment 31

3 years ago
There are 51 l10n ini files in gaia, four of them have more than one URL template. Very straightforward transition.
(Assignee)

Comment 32

3 years ago
We should also track this: https://github.com/w3c/manifest/issues/211
(Assignee)

Comment 33

3 years ago
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.
(Assignee)

Comment 34

3 years ago
I updated my branch to master.
Using the manifest of the app sounds good to me.
Attachment #8453239 - Flags: feedback?(21)
(Assignee)

Comment 36

3 years ago
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.
(Assignee)

Comment 37

3 years ago
Created attachment 8481158 [details] [review]
pull request
(Assignee)

Comment 38

3 years ago
Commit: https://github.com/mozilla-b2g/gaia/commit/b7950aa204681aec9c85104eb2e3e3f1407f2ed4
Merge: https://github.com/mozilla-b2g/gaia/commit/2d16dbb66bc1acdf759e3ad43e5ac9c4c9f9aa2c

L20n.js: https://github.com/l20n/l20n.js/commit/7c8c9e7a2b22ea2c445c4091983d5d8578529da2

Thanks a lot everyone for feedback, reviews and help!
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Blocks: 1060384

Comment 39

3 years ago
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.
(Reporter)

Comment 40

3 years ago
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.
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
(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).
Reverted with https://github.com/mozilla-b2g/gaia/commit/63a8b719e2ec81754b2d350597ce9f54917bb00d
Keywords: checkin-needed
(Reporter)

Comment 43

3 years ago
Thanks, Rik.  Reverted in L20n.js repo as well:

https://github.com/l20n/l20n.js/commit/10c1855238251e6aac267f16fa0ba232aece1e3e
(Assignee)

Comment 44

3 years ago
(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.
(Assignee)

Comment 45

3 years ago
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).
Attachment #8481158 - Flags: review?(stas)
Attachment #8481158 - Flags: review?(l10n)
(Assignee)

Comment 46

3 years ago
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.
(Reporter)

Comment 47

3 years ago
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.
Attachment #8481158 - Attachment is obsolete: true
Attachment #8481158 - Flags: review?(stas)
Attachment #8481158 - Flags: review?(l10n)
Attachment #8482230 - Flags: review?(stas)
Attachment #8482230 - Flags: review?(l10n)
(Reporter)

Updated

3 years ago
No longer blocks: 1060384
(Reporter)

Comment 48

3 years ago
(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.
(Reporter)

Comment 49

3 years ago
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
Attachment #8482230 - Flags: review?(stas) → review-
(Assignee)

Comment 50

3 years ago
Comment on attachment 8482230 [details] [review]
new pull request

Updated the patch to incorporate Stas's feedback. Rerequesting r?
Attachment #8482230 - Flags: review- → review?(stas)
(Assignee)

Comment 51

3 years ago
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?
Attachment #8482230 - Flags: superreview?(yurenju.mozilla)
(Reporter)

Comment 52

3 years ago
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.
Attachment #8482230 - Flags: review?(stas) → review+
(Assignee)

Comment 53

3 years ago
thanks! feedback addressed and the build is green
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+.
Attachment #8482230 - Flags: superreview?(yurenju.mozilla) → superreview-
(Assignee)

Comment 55

3 years ago
Comment on attachment 8482230 [details] [review]
new pull request

Thanks a lot Yuren! I applied your feedback. Requesting review :)
Attachment #8482230 - Flags: superreview- → review?(yurenju.mozilla)

Comment 56

3 years ago
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 on attachment 8482230 [details] [review]
new pull request

that is great, thank you! r=yurenju
Attachment #8482230 - Flags: review?(yurenju.mozilla) → review+

Comment 58

3 years ago
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.
Attachment #8482230 - Flags: review?(l10n) → feedback+
(Assignee)

Comment 59

3 years ago
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?
Attachment #8482230 - Flags: review?(l10n)

Updated

3 years ago
Depends on: 1064832

Comment 60

3 years ago
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.
Attachment #8482230 - Flags: review?(l10n) → review-
(Assignee)

Comment 61

3 years ago
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 :)
Attachment #8482230 - Flags: review- → review?(l10n)
(Assignee)

Comment 62

3 years ago
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
(Assignee)

Updated

3 years ago
Blocks: 1066347

Updated

3 years ago
Attachment #8482230 - Flags: review?(l10n) → review+
(Assignee)

Comment 63

3 years ago
Commit: https://github.com/mozilla-b2g/gaia/commit/02ca67948fe8efd9e07e6d60064b54d0acc29484
Merge: https://github.com/mozilla-b2g/gaia/commit/b9c5dc626e3f2808810c9eb0235ab46a079b4045
(Assignee)

Comment 64

3 years ago
L20n.js: https://github.com/l20n/l20n.js/commit/9324143105fa4ace1bae84dc136dbffd5742d03f
(Assignee)

Comment 65

3 years ago
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.
Attachment #8490190 - Flags: review?(yurenju)
(Assignee)

Comment 66

3 years ago
Filed two follow ups - bug 1068380 and bug 1068382
(Assignee)

Updated

3 years ago
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.
Attachment #8490190 - Flags: review?(yurenju) → review+
(Assignee)

Comment 68

3 years ago
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!
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(gandalf)
(Assignee)

Comment 70

3 years ago
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.
Flags: needinfo?(shchen)
Sorry I was looking on an outdated patch, and the latest one have no problem on branding strings.
Flags: needinfo?(shchen)
Flags: needinfo?(gandalf)
Duplicate of this bug: 1065175
(Assignee)

Updated

3 years ago
Depends on: 1072797
Will this be uplifted to 2.1 or will it be fixed in a different manner?
(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).
Depends on: 1079865
(Assignee)

Comment 75

2 years ago
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.
You need to log in before you can comment on or make changes to this bug.