Closed Bug 1108096 Opened 5 years ago Closed 5 years ago

Langpack support for b2g/gaia

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set

Tracking

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

RESOLVED FIXED
mozilla38
feature-b2g 2.2?
tracking-b2g backlog
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: zbraniecki, Assigned: fabrice)

References

Details

Attachments

(2 files, 5 obsolete files)

We need some platform magic to support Langpacks in Gaia.

Spec is at: https://github.com/stasm/spec/blob/master/webapp-langpacks.markdown
Component: Runtime → DOM: Apps
Product: Firefox OS → Core
Attached patch wip (obsolete) — Splinter Review
Assignee: nobody → fabrice
Is this in scope of 2.2?
[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: --- → ?
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)
That's the plan, yes.
Flags: needinfo?(fabrice)
Attached patch patch v1 (obsolete) — Splinter Review
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#
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-
(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?
(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.
(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 })?
(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.
(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.
Attached patch patch v2 (obsolete) — Splinter Review
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)
Attached patch patch v3 (obsolete) — Splinter Review
This adds a "binary" parameter to getLocalizationResource.
Attachment #8544107 - Attachment is obsolete: true
Attached patch patch v4 (obsolete) — Splinter Review
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)
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.
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)
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"
Attached patch patch v5Splinter Review
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)
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! :)
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)
Gandalf, here's a followup that turns getAdditionalLanguages() into a sync method. Untested!
Blocks: 1107346
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.
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+
damn, and I thought this was an infra issues on try :(
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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?
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 → ---
Sorry, that's Fixed. We'll work on the App name thing in bug 1118946.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Attachment #8545562 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
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).
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.