Closed Bug 1115807 Opened 9 years ago Closed 9 years ago

Use <meta> keys in favor of manifest.webapp for localization metadata

Categories

(Firefox OS Graveyard :: Gaia::L10n, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::L10n
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(
URL
)

Details

Attachments

(3 files, 1 obsolete file)

pull request
46 bytes, text/x-github-pull-request
stas
: review+
Details | Review
l10n.patch
6.28 KB, patch
stas
: review+
Details | Diff | Splinter Review
build.patch
9.74 KB, patch
ochameau
: review+
Details | Diff | Splinter Review 
After extended discussions with W3C Manifest group, we decided that putting manifest.webapp in critical path for app loading may not be the right way to go.

The alternative is to rely on two <meta>s in document's <head> in form of:

<meta name="defaultLanguage" content="en-US">
<meta name="availableLanguages" content="en-US, zh-TW">

where optionally (for language packs), availableLanguages may contain build datetime:

<meta name="availableLanguages" content="en-US:201411141259, zh-TW:201411141259">

This will remove an async XHR call from runtime l10n path and remove the dependency on W3C Manifests.
This should be considered high priority to land before 2.2 in order to avoid having a deprecated solution baked into stable release.
Depends on: 1068382
Branch: https://github.com/zbraniecki/gaia/tree/1115807-move-l10n-metadata-to-meta

For now, I'm just moving stuff and removing obsolete bits from webapp-optimize.

Once bug 1068382 land, I'll add meta localization in multilocale.js.
Assignee: nobody → gandalf
URL: https://github.com/zbraniecki/gaia/tr...
Status: NEW → ASSIGNED
Attached file pull request — 
pull request
Attached patch build.patch (obsolete) — — Splinter Review 
Alexandre, this is the patch I mentioned in bug 1068382. It is based on that patch.

What it does is basically make us localize defaultLanguage and availableLanguages keys in every HTML file that has them.

It will remove the dependency on manifest.webapp and get the timestamp on locale will get us ready for langpacks (bug 1115796) without any further build system changes.
Attachment #8541994 - Flags: review?(stas)
Attachment #8541994 - Flags: review?(poirot.alex)
Attached patch l10n.patch — — Splinter Review 
Stas, this is a subset of changes I've been working on in branch for bug 1115796 that make us use l10n <meta> keys and remove the dependency on manifest.webapp.
Attachment #8541995 - Flags: review?(stas)
Comment on attachment 8541992 [details] [review]
pull request

Finally, if you could give me r+ on the HTML files changes. I preserved availableLanguages from apps' manifest.webapp
Attachment #8541992 - Flags: review?(stas)
Comment on attachment 8541994 [details] [diff] [review]
build.patch

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

Looks good, but it would be really great if we could get reproducible builds!
Otherwise, do you have a green pull request with this patch?

::: build/multilocale.js
@@ +204,5 @@
>        localizeManifest(webapp);
> +    }
> +
> +    htmlFiles.forEach(function(htmlFile) {
> +      var doc = buildL10nMeta(htmlFile, webapp);

Given that both buildL10nMeta and getL10nResources depends on having a doc reference, I would have create `doc` here, in the forEach function, instead of fetching it from buildL10nMeta.
Then getL10nResources would always get doc reference and wouldn't have to create one:
var doc = ...
buildL10nMeta(..., doc);
...
  getL10nResources(..., doc);
(And please cleanup getL10nResources by considering you always get a non-null doc argument.)

@@ +240,5 @@
> +      metas.defaultLanguage.setAttribute('content', self.defaultLocale);
> +    }
> +
> +    if (metas.availableLanguages) {
> +      var timestamp = getTimestamp(new Date());

Could we use something else than "now"?
It makes the build system non-reproducible and it is quite painful.
A build should always be the same for a given git changeset.
Could we use some specific file timestamp instead?
Or, do we really have to use a timestamp? (I don't know "availableLanguages" specification...) If we just need something random and unique, we should use a git changeset instead!
Last option I have in mind would be to use git changeset date, but I don't know exactly how to pull it from here. (we somehow pull the changeset id for the settings app)
Attachment #8541994 - Flags: review?(poirot.alex)
Thanks for superquick review! That's awesome :)

(In reply to Alexandre Poirot [:ochameau] from comment #7)
> Comment on attachment 8541994 [details] [diff] [review]
> build.patch
> 
> Review of attachment 8541994 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but it would be really great if we could get reproducible builds!
> Otherwise, do you have a green pull request with this patch?

Good point

> Given that both buildL10nMeta and getL10nResources depends on having a doc
> reference, I would have create `doc` here, in the forEach function, instead
> of fetching it from buildL10nMeta.

Sweet, will update the patch in a minute.

> @@ +240,5 @@
> > +      metas.defaultLanguage.setAttribute('content', self.defaultLocale);
> > +    }
> > +
> > +    if (metas.availableLanguages) {
> > +      var timestamp = getTimestamp(new Date());
> 
> Could we use something else than "now"?
> It makes the build system non-reproducible and it is quite painful.
> A build should always be the same for a given git changeset.
> Could we use some specific file timestamp instead?
> Or, do we really have to use a timestamp? (I don't know "availableLanguages"
> specification...) If we just need something random and unique, we should use
> a git changeset instead!
> Last option I have in mind would be to use git changeset date, but I don't
> know exactly how to pull it from here. (we somehow pull the changeset id for
> the settings app)

I agree that it would be better to have reproducible builds and I just didn't think about it. I'll see how much work would it be to get the changeset timestamp.
(In reply to Zibi Braniecki [:gandalf] from comment #8)
> I agree that it would be better to have reproducible builds and I just
> didn't think about it. I'll see how much work would it be to get the
> changeset timestamp.

So, unfortunately it doesn't seem to be anything easy.

That is how Settings do this: https://github.com/mozilla-b2g/gaia/blob/322ef5116a5827a30c9a3cd9b842449a9c66a5b3/apps/settings/build/build.js#L134-L140 - yuck :(

Now, on top of that, Stas pointed out that git changeset date may not fly well. Imagine a partner building their build based on a branch with a recent changeset. Suddenly, all langpacks will be older even if they shouldn't be.

Now, the solution here might be to pull changeset date from each locale dir separately, but that requires HG command and ties our buildsystem to hg :(

I think it will be easiest for now to stick to using build time date.

What's your opinion :ochameau?
Flags: needinfo?(poirot.alex)
Comment on attachment 8541994 [details] [diff] [review]
build.patch

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

The build is green
Attachment #8541994 - Flags: review?(poirot.alex)
(In reply to Zibi Braniecki [:gandalf] from comment #9)
> (In reply to Zibi Braniecki [:gandalf] from comment #8)
> > I agree that it would be better to have reproducible builds and I just
> > didn't think about it. I'll see how much work would it be to get the
> > changeset timestamp.
> 
> So, unfortunately it doesn't seem to be anything easy.
> 
> That is how Settings do this:
> https://github.com/mozilla-b2g/gaia/blob/
> 322ef5116a5827a30c9a3cd9b842449a9c66a5b3/apps/settings/build/build.js#L134-
> L140 - yuck :(

It sounds gross if you are not used to gecko-js, but that's fine doing such thing.
The challenge is more about our CI/automation tools and how they are building the project.
We had to come up with gaia_commit.txt to accomodate them :s

> 
> Now, on top of that, Stas pointed out that git changeset date may not fly
> well. Imagine a partner building their build based on a branch with a recent
> changeset. Suddenly, all langpacks will be older even if they shouldn't be.
> 
> Now, the solution here might be to pull changeset date from each locale dir
> separately, but that requires HG command and ties our buildsystem to hg :(

Using hg/git is nice as it give us a unified/unique date meaningfull for all files,
but may be in your case you can just use the manifest file date or something?
Could you describe what this date would ideally describe?
Does it has to be a date? Or can it be just something unique? i.e. something else than incremental number? (changeset, md5, ...)

> 
> I think it will be easiest for now to stick to using build time date.

"Now()" just sounds random. I'm fine handling that in a followup,
I would like to continue thinking a bit about that and see if we can find something reasonable.
Flags: needinfo?(poirot.alex)
(In reply to Zibi Braniecki [:gandalf] from comment #10)
> Comment on attachment 8541994 [details] [diff] [review]
> build.patch
> 
> Review of attachment 8541994 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The build is green

and

> Sweet, will update the patch in a minute.

Is the updated patch somewhere in a PR? You requested the review on the exact same patch file?
(In reply to Alexandre Poirot [:ochameau] from comment #11)

> Could you describe what this date would ideally describe?
> Does it has to be a date? Or can it be just something unique? i.e. something
> else than incremental number? (changeset, md5, ...)

This piece of information should describe a version of the localization.  We need something that can be compared in a meaningful manner so that the l10n.js library is able to tell which langpack is more recent.

Dates seem like a good implicit candidates for this:  if the Arabic localization is built from the following changeset:

 https://hg.mozilla.org/gaia-l10n/ar/rev/2eb05a81fbae

…we can use its timestamp as the version number.  Paired with the branch information we can then decide if another langpack has a newer version of Arabic or not.  A more explicit alternative would be to use semver in a sepcial metadata file in each l10n repo, but I wouldn't like to add another thing to remember about for the localizers when they work on translations.

For en-US, using the Gaia commit timestamp is okay since the canonical version of the en-US localization is in the Gaia repo itself.
Pike, would it be possible to have an hg hook in our l10n repos (https://hg.mozilla.org/gaia-l10n/* and others) which on every push puts the time of the push into a special meta file in that repository?

For non-h.m.o repos or non-vcs folders, the meta file would need to be maintained manually.
Flags: needinfo?(l10n)
Given that most of our builds use the git mirrors, it's tricky. I think that adding a changeset with meta data would be revolting, and using meta data probably doesn't carry over to git.

Partner repos will likely generate broken data either way.

I'd go for hg/git timestamp of the commit, and create code that works for both (and detects which to use without requiring that either is installed).
Flags: needinfo?(l10n)
(In reply to Axel Hecht [:Pike] from comment #15)
> Given that most of our builds use the git mirrors, it's tricky. I think that
> adding a changeset with meta data would be revolting, and using meta data
> probably doesn't carry over to git.
> 
> Partner repos will likely generate broken data either way.
> 
> I'd go for hg/git timestamp of the commit, and create code that works for
> both (and detects which to use without requiring that either is installed).

What should we use as a fallback in case l10n dir or gaia dir is not a git/hg repo?
Comment on attachment 8541995 [details] [diff] [review]
l10n.patch

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

r=me, I'm really liking how much simpler the code gets with this change!

::: shared/js/l10n.js
@@ +1614,3 @@
>      if (this.ctx.availableLocales.length) {
>        return initLocale.call(this);
>      }

Should we do

  this.ctx.registerLocales(this.ctx.defaultLocale);

if the condition here is falsy?

@@ +1626,5 @@
> +        meta.availableLanguages = node.getAttribute('content').split(',').map(
> +          function(lang) {
> +            lang = lang.trim().split(':');
> +            return lang[0];
> +          }

Maybe move that to its own function definition for clarity?
Attachment #8541995 - Flags: review?(stas) → review+
Comment on attachment 8541994 [details] [diff] [review]
build.patch

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

I had a look at this patch but I'm not ready yet to review it in the full extent, and I don't want to block you from landing if you get an r+ from ochameau.  Canceling the review request for now.
Attachment #8541994 - Flags: review?(stas)
Sorry, forgot to NI Axel (Comment 16)
Flags: needinfo?(l10n)
Attached patch build.patch — — Splinter Review 
Updated patch. Sorry Alexandre, juggling between PRs and patches :)

(In reply to Alexandre Poirot [:ochameau] from comment #11)
> "Now()" just sounds random. I'm fine handling that in a followup,
> I would like to continue thinking a bit about that and see if we can find
> something reasonable.


Yes please, I'll be happy to file a follow up. I'd like to land this ASAP and it touches a lot of files and in result has enough vector for regressions that I don't want to wait any closer to branch time.

Then we can work on a proper strategy for building the timestamp that gracefully falls back.
Attachment #8541994 - Attachment is obsolete: true
Attachment #8541994 - Flags: review?(poirot.alex)
Attachment #8542707 - Flags: review?(poirot.alex)
No idea. I guess that using a timestamp is somewhat of a hack to begin with, I'd suggest to use some hack on top of that. Maybe in that case, the current timestamp is good enough. We'd loose the reproducible result, but at least it's incrementing.
Flags: needinfo?(l10n)
Agree. Filed a follow up bug 1116613.
Comment on attachment 8542707 [details] [diff] [review]
build.patch

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

::: build/multilocale.js
@@ +199,3 @@
>  
> +      var doc = utils.getDocument(content);
> +      

nit: trailing space
Attachment #8542707 - Flags: review?(poirot.alex) → review+
Comment on attachment 8541992 [details] [review]
pull request

LGTM, r=me!
Attachment #8541992 - Flags: review?(stas) → review+
L20n.js: https://github.com/l20n/l20n.js/commit/d215f462c0bd59b58fed6ef78aa17c6fc5452a41
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: