Langpack support for b2g/gaia

RESOLVED FIXED in Firefox 38, Firefox OS v2.2

Status

Core Graveyard
DOM: Apps
RESOLVED FIXED
4 years ago
7 months ago

People

(Reporter: gandalf, Assigned: fabrice)

Tracking

unspecified
mozilla38
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2?, tracking-b2g:backlog, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

4 years ago
We need some platform magic to support Langpacks in Gaia.

Spec is at: https://github.com/stasm/spec/blob/master/webapp-langpacks.markdown
(Reporter)

Comment 1

4 years ago
Dummy POC implementation lives here: 

https://github.com/zbraniecki/gaia/tree/langpacks

In particular:

1) https://github.com/zbraniecki/gaia/blob/langpacks/shared/js/weblangpack.js
2) https://github.com/zbraniecki/gaia/blob/langpacks/shared/js/l10n.js
3) https://github.com/zbraniecki/gaia/blob/langpacks/apps/settings/index.html

where 2/3 are modified master versions of the files to work with 1 which is a shim of the platform code.
(Reporter)

Updated

4 years ago
Blocks: 1107341
(Assignee)

Updated

4 years ago
Component: Runtime → DOM: Apps
Product: Firefox OS → Core
(Assignee)

Comment 2

4 years ago
Created attachment 8535936 [details] [diff] [review]
wip
Assignee: nobody → fabrice
Is this in scope of 2.2?

Comment 4

4 years ago
[Tracking Requested - why for this release]:
Strongly recommend taking this, particularly since other L10N items for language support are not currently listed in 2.2. This helps decouple language support from and reduces dependency on release schedules and string freeze dates, and continues language support even when priorities change (as they so often do). Please ping me with questions.

Minimal UX for this is complete.
feature-b2g: --- → 2.2?
tracking-b2g: --- → ?

Comment 5

4 years ago
Can we finish this one in 2.2? If we're okay with it, I can mark this one as feature-b2g:2.2+. Thanks.
Flags: needinfo?(fabrice)
(Assignee)

Comment 6

4 years ago
That's the plan, yes.
Flags: needinfo?(fabrice)
(Assignee)

Comment 7

4 years ago
Created attachment 8539621 [details] [diff] [review]
patch v1

Gandalf, can you test on your side, especially to know if the getSelf() call doesn't slow us down too much (I think we can speed it up if it's a bottleneck).

I think the only change that is still to be done API wise is to move from a CustomEvent to a dedicated AddionnalLanguageChangeEvent. I'll do it in the next patch.
Attachment #8535936 - Attachment is obsolete: true
Attachment #8539621 - Flags: feedback?(gandalf)
Attachment #8539621 - Flags: feedback?(ferjmoreno)
How will Marketplace know about languages that are preinstalled on the device? Will all pre-installed languages be converted to lang packs and set as if they've been installed from Marketplace?

PRD:https://docs.google.com/a/mozilla.com/document/d/1ZxMklFdJSAC0oh1hTpzSI5F9fbVFI5XmdnCl-iniy9Y/edit#
(Reporter)

Updated

3 years ago
Blocks: 1115796
(Reporter)

Updated

3 years ago
Blocks: 1115797
(Reporter)

Comment 9

3 years ago
Comment on attachment 8539621 [details] [diff] [review]
patch v1

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

This patch looks really good. I was able to use it to build l10n.js and Settings support for langpacks, install a custom langpack and switch to use it!

There are three areas that require more work:

1) The path expected by getLocalizationResource should use the value of languages-provided.apps KVP as a root for that app. I currently have to manually concatenate the URL provided by the app with this value.

2) The additional languages returned by getAdditionalLanguages don't contain the localized locale name. It is necessary for Settings>Languages to be able to display that.

I'm not sure how to do that properly. The simplest way would be to add a key to manifest.webapp under languages-provided[ab-CD].name

3) The first version of my code renders FOUCs when using locale from langpack.

That is the major one. I don't know how easy it will be to overcome it. From my experience of working with Gecko<-->L10n bootstrap, we have limited number of async calls before Gecko firstPaint.

So far we've been doing (for non-default locales):
 - sync loadLanguagesList
 - async loadJSON

and that worked well.

With this change we are actually doing three async calls:

 - async load mozApps.getSelf
 - async load getAdditionalLanguages
 - async loadJSON

I don't know if getSelf is slow or fast (there have been rumors that it is slow, but no hard data yet). It may be fast, but because it is async it pushes us beyond firstPaint.

Now, that I have running code, I'll try to experiment with enforced asyncs in mockup Langpack API and see if the problem is about the number of asyncs or the time it takes to create getSelf. If limiting the number of async calls to two (getAdditionalLanguages and loadJSON) will fix it, I may want to ask to move getAdditionalLanguages to mozApps.

If that will not be enough and even two async calls push us beyond firstPaint, we have a major problem here.
I'll report progress.
Attachment #8539621 - Flags: feedback?(gandalf) → feedback-
(Reporter)

Comment 10

3 years ago
(In reply to Zibi Braniecki [:gandalf] from comment #9)

> 3) The first version of my code renders FOUCs when using locale from
> langpack.


An update on that. 

We actually have five async calls using the above API:

1) async call to mozApps.getSelf in bindings code to register extra locales
2) async call to getAdditionalLanguages in bindings code to register extra locales
3) async call to mozApps.getSelf in Locale to load langpack resource file
4) async call to getLocalizationResource in Locale to load langpack resource file
5) async call to Blob.loadend in Locale to read from Blob

The result is a FOUC because we end up with the entry table (and mozL10n ready) past firstPaint.

I tested how many async calls can I keep in order to not get FOUC.

It seems that two works:

1) async call to mozApps.getAdditionalLanguages in bindings code to register extra locales
2) async call to getLocalizationResource in Locale to load langpack resource file

I guess we could cache getSelf().onsuccess and trigger it synchronously to avoid 1) and 3), but even without them, using Blob.loadend brings FOUCs.

Can we (at least for now) use mozApps API and use string instead of Blob?
(Assignee)

Comment 11

3 years ago
(In reply to Zibi Braniecki [:gandalf] from comment #9)
> Comment on attachment 8539621 [details] [diff] [review]
> patch v1
> 
> Review of attachment 8539621 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch looks really good. I was able to use it to build l10n.js and
> Settings support for langpacks, install a custom langpack and switch to use
> it!
> 
> There are three areas that require more work:
> 
> 1) The path expected by getLocalizationResource should use the value of
> languages-provided.apps KVP as a root for that app. I currently have to
> manually concatenate the URL provided by the app with this value.

The current patch does the following, with this manifest and installing the app from http://mochi.test:8888/tests/dom/apps/tests/langpack/lang1.manifest:

"languages-provided": {
  "de": {
    "version": 201411051234,
    "apps": {
      "http://mochi.test:8888/tests/dom/apps/tests/langpack/manifest.webapp": "tests/dom/apps/tests/langpack/de/"
    }
  }
}

- Resolve tests/dom/apps/tests/langpack/de/ against the app origin (here http://mochi.test:8888), leading to http://mochi.test:8888/tests/dom/apps/tests/langpack/de/.
- Use that uri as a base uri to resolve paths from getLocalizationResource().


> 2) The additional languages returned by getAdditionalLanguages don't contain
> the localized locale name. It is necessary for Settings>Languages to be able
> to display that.
> 
> I'm not sure how to do that properly. The simplest way would be to add a key
> to manifest.webapp under languages-provided[ab-CD].name

I have no strong opinion there, but that looks reasonable (and easy). We could default on ab-CD if the local name is not explicitly specified.

> I don't know if getSelf is slow or fast (there have been rumors that it is
> slow, but no hard data yet). It may be fast, but because it is async it
> pushes us beyond firstPaint.

It's not as fast as it could be. I'll write a patch to save us the IPC roundtrip.
(Assignee)

Comment 12

3 years ago
(In reply to Zibi Braniecki [:gandalf] from comment #10)

> It seems that two works:
> 
> 1) async call to mozApps.getAdditionalLanguages in bindings code to register
> extra locales
> 2) async call to getLocalizationResource in Locale to load langpack resource
> file
> 
> I guess we could cache getSelf().onsuccess and trigger it synchronously to
> avoid 1) and 3), but even without them, using Blob.loadend brings FOUCs.
> 
> Can we (at least for now) use mozApps API and use string instead of Blob?

I guess we can get rid of the getSelf() calls by making getAdditionalLanguages() and getLocalizationResource() implicitly work on the current app. For the Blob, nothing guarantees that we return strings (eg. you could fetch localized versions of images) so I'm on the fence there. Maybe with an additional parameter (eg. { binary: false })?
(Reporter)

Comment 13

3 years ago
(In reply to Fabrice Desré [:fabrice] from comment #11)
> The current patch does the following, with this manifest and installing the
> app from http://mochi.test:8888/tests/dom/apps/tests/langpack/lang1.manifest:
> 
> "languages-provided": {
>   "de": {
>     "version": 201411051234,
>     "apps": {
>      
> "http://mochi.test:8888/tests/dom/apps/tests/langpack/manifest.webapp":
> "tests/dom/apps/tests/langpack/de/"
>     }
>   }
> }
> 
> - Resolve tests/dom/apps/tests/langpack/de/ against the app origin (here
> http://mochi.test:8888), leading to
> http://mochi.test:8888/tests/dom/apps/tests/langpack/de/.
> - Use that uri as a base uri to resolve paths from getLocalizationResource().

Correct, which means that it resolves the path provided to getLocalizationResource as relative to 'tests/dom/apps/tests/langpack/de/' while it should treat 'tests/dom/apps/tests/langpack/de/' as a root of the path and resolve '/locales-obj/de.json' as 'tests/dom/apps/tests/langpack/de/locales-obj/de.json' internally. 

> I have no strong opinion there, but that looks reasonable (and easy). We
> could default on ab-CD if the local name is not explicitly specified.

Sounds good. I'll update lp-builder to include the name of the locale probably taken from some manifest.properties in locale repository.
(Reporter)

Comment 14

3 years ago
(In reply to Fabrice Desré [:fabrice] from comment #12)
> (In reply to Zibi Braniecki [:gandalf] from comment #10)
> 
> > It seems that two works:
> > 
> > 1) async call to mozApps.getAdditionalLanguages in bindings code to register
> > extra locales
> > 2) async call to getLocalizationResource in Locale to load langpack resource
> > file
> > 
> > I guess we could cache getSelf().onsuccess and trigger it synchronously to
> > avoid 1) and 3), but even without them, using Blob.loadend brings FOUCs.
> > 
> > Can we (at least for now) use mozApps API and use string instead of Blob?
> 
> I guess we can get rid of the getSelf() calls by making
> getAdditionalLanguages() and getLocalizationResource() implicitly work on
> the current app.

That sounds good.

> For the Blob, nothing guarantees that we return strings
> (eg. you could fetch localized versions of images) so I'm on the fence
> there. Maybe with an additional parameter (eg. { binary: false })?

Yeah, for now langpacks can handle only textual (and, in fact in production cases, json) data. { binary: false} sounds good to me. As we move more into the platform we will be able to block rendering and then we can use cleaner methods without risking FOUCs.
(Assignee)

Comment 15

3 years ago
Created attachment 8544107 [details] [diff] [review]
patch v2

Changes in this patch are that methods are now available on navigator.mozApps and will hook up to the current app, or reject the promise if we are not in an app context.
Attachment #8539621 - Attachment is obsolete: true
Attachment #8539621 - Flags: feedback?(ferjmoreno)
(Assignee)

Comment 16

3 years ago
Created attachment 8544156 [details] [diff] [review]
patch v3

This adds a "binary" parameter to getLocalizationResource.
Attachment #8544107 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
Created attachment 8544808 [details] [diff] [review]
patch v4

Gandalf, I changed the `binary` boolean to be an enum with possible values of "binary", "json" or "text".
Attachment #8544156 - Attachment is obsolete: true
Attachment #8544808 - Flags: review?(jonas)
Attachment #8544808 - Flags: review?(ferjmoreno)
(Reporter)

Comment 18

3 years ago
I updated l10n.js to use the API (rev 4) in bug 1115796.

While testing the performance impact of the API I encountered a fully reproducible FOUC in Settings app and significant performance penalty in all tested apps.

Digging deeper and syncing with Fabrice on IRC I got a reproducible testcase:


1) Install Gecko with the patch
2) Apply this patch on Gaia: http://pastebin.mozilla.org/8166809
3) Flash your build
4) open adb logcat

The result is:
"PERFLOG getAdditionalLanguages took: 649"

The value ranges from 620ms to 900ms.
The very same test performed after app load (setTimeout 5000ms) gives 25ms instead.

The conclusion is that the code competes with the app being loaded.

Unfortunately, the nature of runtime l10n requires us to load the code during loading/interactive state of the document loading. For non-default languages we are able to get the whole logic from the initialization to ctx.requestLocales within 100ms.

I suspect that we can take a bit more time here, but not 700ms. This pushes us beyond firstPaint.

All other default apps draw their content only after app-controlled initialization, so we're not flashing, but Settings does not, so we use Settings as a litmus test.

We need to find a fix for that.
(Reporter)

Comment 19

3 years ago
Would it be possible to cache it? The list of available langpacks only changes at langpack install/uninstall, so if we can cache it there and only return it when getAdditionalLanguages is called, it should make it super-cheap even in crowded scenario like Settings.

Does it make any sense?
Flags: needinfo?(fabrice)
(Reporter)

Comment 20

3 years ago
Two more nits:

1) We need to have a user-readable name of the locale to show in Settings language selection list.

I suggest that we add

languages-provided: {
  ab-CD: {
    version: 201501050843,
    name: "Polski",
    apps: {}
  }
}

and getAdditionalLanguages should return the name along version/target.

2) We still have the relative path in the langpack issue.

It results in our temporary code here: https://github.com/zbraniecki/gaia/blob/8d6d8bfd44e8957a99d6958ab0c32a5fdfad7357/shared/js/l10n.js#L1171-L1195

Because many links in Gaia are absolute, like "/shared/locales/foo" and "/locales/dialer.properties" linked both from communications/dialer/index.html and callscreen/index.html, we would much prefer not to have to compose this path from within l10n.js.

The preferred solution for us is that path argument for getLocalizationResource is 'chrooted' to the path provided in the manifest for the given app within langpack.

Example:
navigator.mozApps.getLocalizationResource('pl', '2.2', '/shared/locales/keypad.pl.properties', 'json');

launched in sms.gaiamobile.org should load file from path:

"/pl/apps/sms/shared/locales/keypad.pl.properties"
(Assignee)

Comment 21

3 years ago
Created attachment 8545562 [details] [diff] [review]
patch v5

Gandalf,

Let me know if getAdditionalLanguages is fast enough now. It still returns a Promise, which is handy to test for failure. I also added the "name" property and changed the way we resolve the url.
Attachment #8544808 - Attachment is obsolete: true
Attachment #8544808 - Flags: review?(jonas)
Attachment #8544808 - Flags: review?(ferjmoreno)
Flags: needinfo?(fabrice)
(Reporter)

Comment 22

3 years ago
Performance results:

App: Settings
Gaia: 20150107
Gecko: 20150107 + patch
Runs: Avg(3x10)

locale:               |      default    |    non-default  |     langpack    |
                      |-----------------------------------------------------|
                      | master |  patch | master |  patch | master |  patch |
======================|=====================================================|
memory (USS)          | 15.593 | 15.443 | 15.666 | 15.660 |        | 16.186 |
content-interactive   |    968 |    980 |    977 |   1006 |        |   1026 |
chrome-dom-loaded     |   1236 |   1288 |   1058 |   1092 |        |   1156 | 
chrome-interactive    |   1236 |   1288 |   1058 |   1092 |        |   1156 | 
app-visually-complete |   2247 |   2236 |   2178 |   2177 |        |   2344 |
app-loaded            |   5074 |   5092 |   4994 |   5021 |        |   5158 |


First results seems promising. Notably:

1) We seem to use slightly more memory when using a langpack locale. That's on Gaia to fix it, Gecko patch does not affect.
2) We seem to be slightly slower for langpack than non-default. This probably comes from the fact that getAdditionalLanguages is still taking ~125ms. I expect that if we move this call to the beginning of our execution we should shave ~50ms and get on par with non-default scenario. Alternatively, Fabrice indicated that he could make getAdditionalLanguages sync. That could help as well.
3) Non-default scenario is within range and slightly faster than default scenario. Time to explore removal of pretranslate optimization? :)

==========================================================================================

All in all, I believe that this patch is good performance wise for our needs. 

I suspect that a sync version of getAdditionalLanguages could give us some perf win so I'd be happy to measure against such patch, because my expected 50ms win at bootstrap is meaningful for our perf-goals. I leave it to you guys to decide if it's worth exploring this path.

Either way, I'm happy with this patch, there are no FOUCs, performance is within range and the I'd consider the API fully operational. Thanks Fabrice! :)
(Assignee)

Comment 23

3 years ago
Comment on attachment 8545562 [details] [diff] [review]
patch v5

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

Fernando & Jonas, we are trying to land that for 2.2, ie. before next Monday. Thanks!
Attachment #8545562 - Flags: review?(jonas)
Attachment #8545562 - Flags: review?(ferjmoreno)
(Assignee)

Comment 24

3 years ago
Created attachment 8545690 [details] [diff] [review]
getAdditionalLanguages-sync.patch

Gandalf, here's a followup that turns getAdditionalLanguages() into a sync method. Untested!
Blocks: 1107346
(Reporter)

Comment 25

3 years ago
I improved my measuring method to mitigate the first result outlier.

This time I run 31 times, 3 sets and took an avg of 30 results of each run minus the first one.

Here are the results:

                      |---------------------------|
                      | rev 5  | rev 5+e |  sync  |
======================|========|=========|========|
content-interactive   |    876 |    874  |   885  |
chrome-dom-loaded     |   1008 |   1003  |  1113  |
chrome-interactive    |   1008 |   1003  |  1113  |
app-visually-complete |   2198 |   2197  |  2213  |
app-loaded            |   5019 |   5022  |  5052  |

rev 5+e is my experiment with an early Promise call.

Conclusion: there's no performance advantage to sync version of the patch.
(Reporter)

Comment 26

3 years ago
Comment on attachment 8545562 [details] [diff] [review]
patch v5

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

just to be clear. From my teams perspective this patch works great. We want it for 2.2 :)
Attachment #8545562 - Flags: feedback+
No longer blocks: 1107346
Comment on attachment 8545562 [details] [diff] [review]
patch v5

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

Thanks Fabrice! LGTM

::: dom/apps/Langpacks.jsm
@@ +55,5 @@
> +  registerRegistryFunctions: function(aBroadcaster, aIdGetter) {
> +    this._broadcaster = aBroadcaster;
> +    this._appIdFromManifestURL = aIdGetter;
> +  },
> +

nit: no need for extra line

@@ +113,5 @@
> +    debug("getLocalizationResource " + uneval(aData));
> +
> +    function sendError(aMsg, aCode) {
> +       debug(aMsg);
> +        aMm.sendAsyncMessage("Webapps:GetLocalizationResource:Return",

nit: indentation

@@ +119,5 @@
> +    }
> +
> +    // No langpack available for this app.
> +    if (!this._data[aData.manifestURL]) {
> +      return sendError("No langpack for this app.", "NoLangpack");

nit: We are using CAPITAL_SNAKE_CASE for error codes in the webapps implementation.

@@ +122,5 @@
> +    if (!this._data[aData.manifestURL]) {
> +      return sendError("No langpack for this app.", "NoLangpack");
> +    }
> +
> +    // We have lanpack(s) for this app, but not for this language.

typo: langpack(s)

@@ +132,5 @@
> +    // Check that we have the right version.
> +    let item = this._data[aData.manifestURL].langs[aData.lang];
> +    if (item.target != aData.version) {
> +      return sendError("No version " + aData.version + " for this app.",
> +                       "UnavailableVersion");

UNAVAILABLE_LANG_VERSION

@@ +137,5 @@
> +    }
> +
> +    // The path can't be an absolute uri.
> +    if (isAbsoluteURI(aData.path)) {
> +      return sendError("url can't be absolute.", "BadUrl");

BAD_LANG_URL

@@ +142,5 @@
> +    }
> +
> +    /*let base = Services.io.newURI(item.url, null, null);
> +    let href = base.resolve(aData.path);
> +    debug("Resolving " + aData.path + " against " + item.url);*/

It seems that you don't need this, so remove it, please

@@ +166,5 @@
> +      if (xhr.status >= 200 && xhr.status < 400) {
> +        aMm.sendAsyncMessage("Webapps:GetLocalizationResource:Return",
> +          { requestID: aData.requestID, oid: aData.oid, data: xhr.response });
> +      } else {
> +        sendError("Error loading " + href, "UnavailableResource");

UNAVAILABLE_LANG_RESOURCE

@@ +170,5 @@
> +        sendError("Error loading " + href, "UnavailableResource");
> +      }
> +    });
> +    xhr.addEventListener("error", function() {
> +      sendError("Error loading " + href, "UnavailableResource");

Ditto

@@ +229,5 @@
> +  // Check if this app is a langpack and update registration if needed.
> +  register: function(aApp, aManifest) {
> +    debug("register app " + aApp.manifestURL + " role=" + aApp.role);
> +
> +    return new Promise((aResolve, aReject) => {

It seems that we are not using the promise that we return. We can make this sync, right?

@@ +294,5 @@
> +  // the entries from this app.
> +  unregister: function(aApp, aManifest) {
> +    debug("unregister app " + aApp.manifestURL + " role=" + aApp.role);
> +
> +    return new Promise((aResolve, aReject) => {

Ditto.

@@ +302,5 @@
> +        aResolve();
> +        return;
> +      }
> +
> +      if (!this.checkManifest(aManifest)) {

Do we need to recheck the manifest?

::: dom/apps/Webapps.js
@@ +44,5 @@
>  
>    receiveMessage: function(aMessage) {
>      let msg = aMessage.json;
> +    let req;
> +    if (aMessage.name != "Webapps:AdditionalLanguageChange") {

Can we check for msg.oid instead?

@@ +89,5 @@
>          this.removeMessageListeners(aMessage.name);
>          Services.DOMRequest.fireSuccess(req, convertAppsArray(msg.apps, this._window));
>          break;
> +      case "Webapps:AdditionalLanguageChange":
> +        // Check if the current page is the from the app receiving the event.

"page is from the app"

@@ +280,5 @@
>  
>      return request;
>    },
>  
> +  _getCurrentAppManifestURL: function() {

Can we move this to AppsUtils and make it getAppManifestURLFromWindow or such and use it in line 94 as well?

@@ +294,5 @@
> +    let manifestURL = this._getCurrentAppManifestURL();
> +
> +    return new this._window.Promise((aResolve, aReject) => {
> +      if (!manifestURL) {
> +        aReject("NotInApp");

NOT_IN_APP

Also, I believe you can do return Promise.reject("NOT_IN_APP"); and avoid creating the promise and later in line 301 return Promise.resolve(...)

@@ +307,5 @@
> +    let manifestURL = this._getCurrentAppManifestURL();
> +
> +    if (!manifestURL) {
> +      return new Promise((aResolve, aReject) => {
> +        aReject("NotInApp");

return Promise.reject("NOT_IN_APP");

::: dom/apps/tests/test_langpacks.html
@@ +13,5 @@
> +  * Test for Bug 1108096
> +  * This file covers testing langpacks.
> +  *
> +  * The setup is as follows:
> +  * - app is the customizable application.

s/customizable/localizable

@@ +22,5 @@
> +SimpleTest.waitForExplicitFinish();
> +
> +let appManifestURL = "http://mochi.test:8888/tests/dom/apps/tests/langpack/manifest.webapp";
> +let lang1ManifestURL = "http://mochi.test:8888/tests/dom/apps/tests/langpack/lang1.webapp";
> +let lang2ManifestURL = "http://mochi.test:8888/tests/dom/apps/tests/langpack/lang2.webapp";

You can take most part of the common URL path to a variable, create a new one for index.html and then reuse it in openApp calls

@@ +90,5 @@
> +
> +function runTest() {
> +  // Set up.
> +  SpecialPowers.setAllAppsLaunchable(true);
> +  SpecialPowers.allowUnsignedAddons();

I don't think we need this for these tests.

@@ +93,5 @@
> +  SpecialPowers.setAllAppsLaunchable(true);
> +  SpecialPowers.allowUnsignedAddons();
> +  SpecialPowers.pushPrefEnv({'set': [
> +    ["dom.mozBrowserFramesEnabled", true],
> +    ]},continueTest);

nit: space after comma and the indentation of this file is not the usual one, but you can just ignore me :)

@@ +121,5 @@
> +  // Opens the iframe to the test page, initial state.
> +  // No locale is available.
> +  openPage("http://mochi.test:8888/tests/dom/apps/tests/langpack/index.html",
> +    ["{}"]);
> +  yield undefined;

Can we also test the behavior when we have no locales available here, please?

@@ +130,5 @@
> +
> +  // Opens the iframe to the test page.
> +  // Only the French locale is available.
> +  openPage("http://mochi.test:8888/tests/dom/apps/tests/langpack/index.html",
> +    ["{\"fr\":[{\"version\":201411051234,\"name\":\"Français\",\"target\":\"2.2\"}]}"]);

Maybe it's a good a idea to save the langpack description objects and do the escaping in an aux function so you can escape the output of a JSON.stringify of the message objects. It will make easier to add future tests and less error-prone

::: dom/webidl/Apps.webidl
@@ +34,5 @@
> +
> +  // Language pack API.
> +  // These promises will be rejected if the page is not in an app context,
> +  // i.e. they are implicitely acting on getSelf().
> +  // If the binary parameter is `false` the promise resolves to a Blob,

I guess you meant that "If the given LocalResourceType is 'binary' the promise will resolve with a Blob, otherwise it will resolve with a DOMString.

Also, move this last part of the comment on top of the affected method, please.
Attachment #8545562 - Flags: review?(ferjmoreno) → review+
Comment on attachment 8545562 [details] [diff] [review]
patch v5

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

r=me, modulo the big discussion about privileged vs. non-privileged.

::: dom/apps/Langpacks.jsm
@@ +244,5 @@
> +        return;
> +      }
> +
> +      let key = Object.keys(aManifest["languages-target"])[0];
> +      let platformVersion = aManifest["languages-target"][key];

We should probably use property name rather than property position here. So I think change the above two lines to

platformVersion = aManifest["languages-target"]["app://*.gaiamobile.org/manifest.webapp"];

Or really, since this is a 'magic' property name anyway given the wildcard, we could just do 

platformVersion = aManifest["languages-target"].gaia;

But this whole languages-target thing feels weird to me. I'd rather that we clean it up. But we can do it as a followup.
Attachment #8545562 - Flags: review?(jonas)
Attachment #8545562 - Flags: review?(ferjmoreno)
Attachment #8545562 - Flags: review+
Attachment #8545562 - Flags: review?(ferjmoreno) → review+
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1142491&repo=b2g-inbound
(Assignee)

Comment 31

3 years ago
damn, and I thought this was an infra issues on try :(
(Assignee)

Comment 33

3 years ago
Ok, I fixed a bug and had to disable the test on emulator because of our usual issues with embed-apps.
https://hg.mozilla.org/mozilla-central/rev/04ee44a2ae09
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Comment 37

3 years ago
Comment on attachment 8545562 [details] [diff] [review]
patch v5

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 #): New feature that failed to land before branching.
User impact if declined: No nice languages.
Testing completed: Mochitests + gandalf
Risk to taking this patch (and alternatives if risky): Low. At worst we would not use the feature.
String or UUID changes made by this patch: none.
Attachment #8545562 - Flags: approval-mozilla-b2g37?
(Reporter)

Comment 38

3 years ago
We planned to add one more feature to the platform: Ability to load a resource from a langpack from another app. This is needed for homescreen/task manager to load App name when using langpack provided language.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 39

3 years ago
Sorry, that's Fixed. We'll work on the App name thing in bug 1118946.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Attachment #8545562 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/dd628f2544ed
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
status-firefox36: --- → wontfix
status-firefox37: --- → wontfix
status-firefox38: --- → fixed
Depends on: 1124141
You should reject using an error, not a string. This makes it difficult to know where the error comes from...
(source: `aReject("NotInApp")` in [1] and [2])

[1] http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.js#298
[2] http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.js#311

Also with this check we break the "load an app in Firefox Nightly" approach we use in the SMS app development (Bug 1124141).

Updated

7 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.