811540 - Add l10n support to the app:// protocol

Status: NEW

(Core :: Networking, defect, P5)

Description

[:Margaret Leibovic]

As part of the solution we found to fix bug 809600, we need support to load different html files depending on the locale.

Combined with bug 810448 and bug 810450, this should make loading l10n way faster.

Fabrice has a WIP patch for this, so I'm assigning it to him :)

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: patch

Margaret, let me know how things look with this patch.

Comment 2

[Axel Hecht [:Pike]]

Comment on attachment 681321 [details] [diff] [review]
patch

I'd not post-fix the url with the locale, just because that changes the extension, that might result in new weirdness.

Also, I'd use the complex value of general.useragent.locale, not sure we need that at a point, but we should follow the best practices for getting that value.

I'd ask for super-review on this change, fwiw. I'm personally not excited how deeply we're hacking things, and how little we're able to use that for webapps on the web. I don't have a constructive proposal, though.

Comment 3

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

Yeah, I second Axel - better name-ab-CD.html than name.html.ab-CD

Also, I filed bug 811593 - maybe if we can fix that then we will not have to do this dirty hacking around it. (or at least we just do it for 1.0 and clean up later once we can ensure reasonable resource load times)

Comment 4

[:Margaret Leibovic]

(In reply to Axel Hecht [:Pike] from comment #2)

> I'd not post-fix the url with the locale, just because that changes the
> extension, that might result in new weirdness.

Hm, what happens to window.location? Ideally, this magical file swap should be totally hidden from the app.

> I'd ask for super-review on this change, fwiw. I'm personally not excited
> how deeply we're hacking things, and how little we're able to use that for
> webapps on the web. I don't have a constructive proposal, though.

I think it's okay if it's just an optimization for built-in Gaia apps, so long as it doesn't make things worse for web apps.

(In reply to Zbigniew Braniecki [:gandalf] from comment #3)

> Also, I filed bug 811593 - maybe if we can fix that then we will not have to
> do this dirty hacking around it. (or at least we just do it for 1.0 and
> clean up later once we can ensure reasonable resource load times)

The conversation in that bug looks promising. However, I don't think anything will be as fast as including a pre-processed l10n dictionary object directly in the page.

Also, I should mention that in the pre-processed JS data, we'll include the locale, so when we call loadLocale(lang, callback) in l10n.js, we'll verify that we're loading the locale we intend to load.

Comment 5

[[:fabrice] Fabrice Desré]

(In reply to Margaret Leibovic [:margaret] from comment #4)
> (In reply to Axel Hecht [:Pike] from comment #2)
> 
> > I'd not post-fix the url with the locale, just because that changes the
> > extension, that might result in new weirdness.
> 
> Hm, what happens to window.location? Ideally, this magical file swap should
> be totally hidden from the app.

It is totally hidden, the channel URI is still the foo.html one (we add to do that anyway to not expose app:// urls as jar:// ones).

> > I'd ask for super-review on this change, fwiw. I'm personally not excited
> > how deeply we're hacking things, and how little we're able to use that for
> > webapps on the web. I don't have a constructive proposal, though.
> 
> I think it's okay if it's just an optimization for built-in Gaia apps, so
> long as it doesn't make things worse for web apps.

I agree.

Comment 6

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

(In reply to Margaret Leibovic [:margaret] from comment #4)
> > Also, I filed bug 811593 - maybe if we can fix that then we will not have to
> > do this dirty hacking around it. (or at least we just do it for 1.0 and
> > clean up later once we can ensure reasonable resource load times)
> 
> The conversation in that bug looks promising. However, I don't think
> anything will be as fast as including a pre-processed l10n dictionary object
> directly in the page.

Yeah. But if you deal with performance then l10n is no different from CSS and JS here. We're calling to at least 24 files. I believe that if the issue is in file access, then such pre-processed HTML should contain all of that content inline for performance reasons.
(I'm not arguing against this approach, just pointing out that it may be expanded to cover more ground until we fix the file access)

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 681321 [details] [diff] [review]
patch

Seems like this code could stand to use nsIURI/nsIURL APIs rather than string munging in a bunch of places?

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Thanks Gavin for pointing to nsIURL. I didn't know this interface, and it's providing all I need!

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 681698 [details] [diff] [review]
patch v2

I'm probably not the right person to review this, but I guess I have a couple of comments...

>diff --git a/netwerk/protocol/app/AppProtocolHandler.js b/netwerk/protocol/app/AppProtocolHandler.js

>   newChannel: function app_phNewChannel(aURI) {

>+    let url = aURI.QueryInterface(Ci.nsIURL);

This QI can fail (e.g. if passed a data: URI). I imagine that's probably fine for this purpose, but worth considering explicitly!

It would probably be clearer to call this "inputURL".

>+      zipReader.open(zipFile);
>+      if (zipReader.hasEntry(fileSpec.substring(1) + "." + locale)) {
>+        uri += "." + locale;
>+      }
>+      zipReader.close();

I don't know much about zipReader - is this going to result in main-thread disk IO on every newChannel call for an html file? If this is guaranteed to be cached/cheap somehow then this may not be a problem, but it looks a bit fishy.

Comment 10

[Fabien Cazenave [:kaze]]

Nominating. Now that bug 821691 has landed, this would allow a very significant improvement on loading times for all apps.

Comment 11

[JP Rosevear [:jpr]]

Any idea how significant?  I think gandalf is about to do some test runs.

Comment 12

[[:fabrice] Fabrice Desré]

Note that the wip I had here will not work anymore once we have the app:// protocol remoted.

Comment 13

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

@JP: I need to retest now that bug 820196 landed. Majority of the win would come from not having to load resources from remote resource, but since CSP cost has been cut that may be of a smaller win now.

Comment 14

[Fabien Cazenave [:kaze]]

JPR, I’ve just proposed another possible fix in bug 815852 (= inlining all l10n resources), see this PR:
https://github.com/mozilla-b2g/gaia/pull/7166

If adding l10n support to the app:// protocol is too difficult/risky at this stage, inlining l10n resources in webapps should be an acceptable solution for v1.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

If I've ready comment 14 correctly, it looks like one of bug 815852 or this bug need to be basecamp+. Bug 815852 is currently basecamp-. We should select the simplest approach with the least amount of risk at this point. I take it that is Fabien's Gaia fix but would like to hear from someone more familiar with this code.

Margaret/Fabrice - Can you weigh in?

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Based on feedback from Jonas via e-mail, marking at basecamp-. We can't take the risk of introducing this type of change in Gecko at this point in the schedule. We should pursue Fabien's Gaia fix in bug 815852. Of note, Gaia triage has marked bug 815852 as a soft blocker for v1.

Comment 17

[Dietrich Ayala (:dietrich)]

Still worth making this change?

Comment 18

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258