1133725 - Langpacks: rename languages-provided.ab-CD.version to "revision"

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Staś Małolepszy :stas]

For consistency with all other "version" fields in the manifest, it might make sense to change the version of the language resources included in the langpack to be a string.

This way it's up to the user-code to decide how to compare to language versions.

Relevant code:

  http://mxr.mozilla.org/mozilla-central/source/dom/apps/Langpacks.jsm#195

Comment 1

[Staś Małolepszy :stas]

It looks like we do the comparison of language versions in platform, too:

  http://mxr.mozilla.org/mozilla-central/source/dom/apps/Langpacks.jsm#251

We'd need to convert the version to an int for this to work if we go ahead with this change proposed in this bug.  Same in l10n.js, where we're already doing it implicitly:

  https://github.com/mozilla-b2g/gaia/blob/e4956b2ecd5357ba4aa7604ca2ab244b01366f7a/shared/js/l10n.js#L1708

Zibi, Fabrice -- what do you think about this?  Is it worth it to keep the "version" field of "langpacks-provided" consistent wrt. type with other version fields in the manifest?

Comment 2

[Staś Małolepszy :stas]

In order to avoid more confusion in the future, would it make sense to rename this field to "timestamp", of type string?

Comment 3

[Mathieu Pillard [:mat]]

"timestamp" or something similar would make more sense IMHO. Having a "version" field that's actually an integer and not compatible with a lot of version numbering schemes developers use is confusing (I made the mistake when implementing langpacks in Marketplace, I thought that the version field not being a string was a typo, because of the inconsistency with the other "version" field at the root of the manifest).

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Staś Małolepszy :stas from comment #1)

> Zibi, Fabrice -- what do you think about this?  Is it worth it to keep the
> "version" field of "langpacks-provided" consistent wrt. type with other
> version fields in the manifest?

I have no preference. The langpack version is the only one we actually use on the platform side so I'm fine we doing changes there if needed.

Comment 5

[Staś Małolepszy :stas]

The W3C spec [1] doesn't even define the "version" field and MDN explicitly states that it's not used by the runtime [2].  This is different from what we're doing with languages-provided.ab-CD.version: it *is* used by the runtime in places which I listed in comment 1.  This is bound to create confusion.

The runtime needs to compare two language versions and in order to do so it has expectations re. the type of the version field. It needs to know how to compare them.  Making it a string would complicate this by letting third-party developers believe they can use any versioning scheme they like.

My suggestion is to:

  - rename languages-provided.ab-CD.version to languages-provided.ab-CD.revision to avoid confusion caused by the fact that other "version" fields in the manifest are usually of string type,
  - keep it as an integer type,
  - explicitly state in the langpack spec that the revisions need to be comparable with the "<" operator.

This will involve a little bit of juggling since we'd need to land this in Gecko 37 and 38 and Gaia 2.0 and master.

[1] https://w3c.github.io/manifest/
[2] https://developer.mozilla.org/en-US/Apps/Build/Manifest#version

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] stasm:1133725-langpack-revision > mozilla-b2g:master

Comment 7

[Staś Małolepszy :stas]

Attachment: [spec] rename language version to revision

Comment 8

[Staś Małolepszy :stas]

Attachment: [gecko] rename language versions to revisions

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebedf02e07d1

Comment 9

[Staś Małolepszy :stas]

Comment on attachment 8565884 [details] [review]
[gaia] stasm:1133725-langpack-revision > mozilla-b2g:master

th: https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=5c0bd04c3068

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 8565897 [details] [diff] [review]
[gecko] rename language versions to revisions

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

lgtm, thanks Staś !

Comment 11

[Staś Małolepszy :stas]

Comment on attachment 8565897 [details] [diff] [review]
[gecko] rename language versions to revisions

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

::: dom/webidl/Apps.webidl
@@ +10,5 @@
>  };
>  
>  dictionary LanguageDesc {
>    DOMString target;
> +  DOMString revision;

Fabrice, since we want revision to be an int, do I need to change this here somehow?

Comment 12

[[:fabrice] Fabrice Desré]

(In reply to Staś Małolepszy :stas from comment #11)
> Comment on attachment 8565897 [details] [diff] [review]
> [gecko] rename language versions to revisions
> 
> Review of attachment 8565897 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/Apps.webidl
> @@ +10,5 @@
> >  };
> >  
> >  dictionary LanguageDesc {
> >    DOMString target;
> > +  DOMString revision;
> 
> Fabrice, since we want revision to be an int, do I need to change this here
> somehow?

Ha you're right. Change that to be a `long` instead.

Comment 13

[Staś Małolepszy :stas]

Attachment: [gecko] rename language versions to revisions, patch 2

I changed the webidl definition of "revision" to long.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc50c134d29c

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8565884 [details] [review]
[gaia] stasm:1133725-langpack-revision > mozilla-b2g:master

lgtm. Thanks stas!

Comment 15

[Staś Małolepszy :stas]

Adding checkin-needed for both Gecko and Gaia patches.

Comment 16

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 17

[GitHub Autolander Bot]

Attachment: [gaia] stasm:1134536-langpack-revision > mozilla-b2g:master

Comment 18

[Staś Małolepszy :stas]

Comment on attachment 8566426 [details] [review]
[gaia] stasm:1134536-langpack-revision > mozilla-b2g:master

Wrong bug, sorry.

Comment 19

[Staś Małolepszy :stas]

Comment on attachment 8565884 [details] [review]
[gaia] stasm:1133725-langpack-revision > mozilla-b2g:master

Moved to bug 1134536.

Comment 20

[Carsten Book [:Tomcat]]

this needs dom peer review for the idl change

Comment 21

[Staś Małolepszy :stas]

Comment on attachment 8565928 [details] [diff] [review]
[gecko] rename language versions to revisions, patch 2

Jonas, can you review the change to dom/webidl/Apps.webidl?  This bug is about changing the name of the "version" property of languages inside langpacks to "revision" to avoid confusion with string-typed "version" fields in other places of the manifest.  The type stays the same (int), but the webidl file didn't reflect that so I'm taking the opportunity here to fix that, too.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8565928 [details] [diff] [review]
[gecko] rename language versions to revisions, patch 2

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

r=me on the Apps.webidl part

Comment 23

[Staś Małolepszy :stas]

Comment on attachment 8565887 [details] [review]
[spec] rename language version to revision

Marking this pull request to the spec as obsolete as it's not really related to the Gecko implementation worked on in this bug.  (Merged in https://github.com/stasm/spec/commit/f3c4d8476279da6c1905bd6eb43c57115125a6b3)

Comment 24

[Staś Małolepszy :stas]

Comment on attachment 8565928 [details] [diff] [review]
[gecko] rename language versions to revisions, patch 2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]

Bug caused by (feature/regressing bug #): langpack spec change: "version" becomes "revision"  (langpacks are a FxOS 2.2 feature)

User impact if declined:  langpacks' manifests will have two different "version" fields of two different types: a string one for the version of the langpack and an integer type for the version of the language included in the langpack;  this is confusing and makes it easy to make mistakes

Testing completed: unit tests

Risk to taking this patch (and alternatives if risky):  low; it's basically a search & replace patch

String or UUID changes made by this patch:  none

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2c5ad87e7db

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d2c5ad87e7db

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d69d6897419e