Last Comment Bug 762864 - Webapp runtime locale files are part of browser, not webapprt
: Webapp runtime locale files are part of browser, not webapprt
Status: VERIFIED FIXED
[qa!]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Webapp Runtime (show other bugs)
: 14 Branch
: x86_64 Linux
: P3 normal
: Firefox 16
Assigned To: Mike Hommey [:glandium]
: Jason Smith [:jsmith]
:
Mentors:
Depends on: 774772
Blocks: metro-build 844016
  Show dependency treegraph
 
Reported: 2012-06-08 05:18 PDT by Mike Hommey [:glandium]
Modified: 2016-03-21 12:39 PDT (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Ship webapprt locale files in webapprt chrome (2.08 KB, patch)
2012-06-09 00:01 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Ship webapprt locale files in webapprt chrome (2.93 KB, patch)
2012-06-09 00:35 PDT, Mike Hommey [:glandium]
benjamin: review-
Details | Diff | Splinter Review
Ship webapprt locale files in webapprt chrome (3.58 KB, patch)
2012-06-12 00:27 PDT, Mike Hommey [:glandium]
benjamin: review+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2012-06-08 05:18:50 PDT
This effectively prevents webapprt from working in FF-on-XR, but also will make things not work very smoothly with bug 755724.
Comment 1 Mike Hommey [:glandium] 2012-06-08 05:24:06 PDT
Relatedly, webapprt probably doesn't work with langpacks.
Comment 2 Jason Smith [:jsmith] 2012-06-08 08:16:07 PDT
This sounds like this is related to the result of the implementation of bug 747645.
Comment 3 Mike Hommey [:glandium] 2012-06-08 08:21:08 PDT
(In reply to Jason Smith [:jsmith] from comment #2)
> This sounds like this is related to the result of the implementation of bug
> 747645.

Indeed. Since I don't have these problems directly, I can revert that bug locally, but at some point we'll have to address this.
Comment 4 Mike Hommey [:glandium] 2012-06-08 08:31:43 PDT
Actually, we could "cheat" and keep the locale files under browser in the source but install them in webapprt.
Comment 5 Mike Hommey [:glandium] 2012-06-08 08:35:50 PDT
For instance, this is kind of awful, but does the job (it probably needs some more tweaks, but you get the idea):

diff --git a/browser/locales/jar.mn b/browser/locales/jar.mn
--- a/browser/locales/jar.mn
+++ b/browser/locales/jar.mn
@@ -120,6 +120,7 @@
     locale/pdfviewer/viewer.properties             (%pdfviewer/viewer.properties)
     locale/pdfviewer/chrome.properties             (%pdfviewer/chrome.properties)
 #ifdef MOZ_WEBAPP_RUNTIME
+../webapprt/chrome/@AB_CD@.jar:
 % locale webapprt @AB_CD@ %locale/webapprt/
     locale/webapprt/webapp.dtd                     (%webapprt/webapp.dtd)
     locale/webapprt/webapp.properties              (%webapprt/webapp.properties)
Comment 6 Myk Melez [:myk] [@mykmelez] 2012-06-08 09:22:39 PDT
We put the files under browser/ intentionally, because the l10n toolchain would need to be modified to find them under webapprt/.

cc:ing Pike for input into the l10n retooling that would be required to move these files to webapprt/.

cc:ing bsmedberg for his feedback on the proposed workaround of leaving the source files under browser/ but installing them into [objdir]/dist/bin/webapprt/.
Comment 7 Axel Hecht [:Pike] 2012-06-08 09:46:25 PDT
I don't understand why this blocks XR? Also, moving the files for fun isn't really helping our localization story, as we have 90 other places where you'll need to move them, at different times for different teams, and all that.

If you'd want a tld webapprt, you'd have to do either of:

* add webapprt to the directories in browser/locales/l10n.ini
* add an webbapprt/locales/l10n.ini, and include that in browser/locales/l10n.ini

The difference is when other apps would like to ship with webapprt, that'd make it easier.

The files would need to go to webapprt/locales/en-US/...

fx-langpacks would work if they'd have compatibility info for whatever appid webapprt uses, and they'd be at a discoverable location.
Comment 8 Mike Hommey [:glandium] 2012-06-08 10:51:30 PDT
(In reply to Axel Hecht [:Pike] from comment #7)
> I don't understand why this blocks XR?

The webapp runtime, when running on top of XR doesn't have access to browser chrome. This issue will become visible on mozilla builds when browser chrome moves under a browser/ subdirectory in bug 755724.

The lack of access to these l10n files makes webapps just display XML errors because of missing entities.
Comment 9 Mike Hommey [:glandium] 2012-06-08 11:21:25 PDT
(In reply to Axel Hecht [:Pike] from comment #7)
> If you'd want a tld webapprt, you'd have to do either of:
> 
> * add webapprt to the directories in browser/locales/l10n.ini
> * add an webbapprt/locales/l10n.ini, and include that in
> browser/locales/l10n.ini
> 
> The difference is when other apps would like to ship with webapprt, that'd
> make it easier.
> 
> The files would need to go to webapprt/locales/en-US/...

Bug 747645 comment 0 suggests this is not enough. It's sad that the files were moved if that's really all that was needed...

> fx-langpacks would work if they'd have compatibility info for whatever appid
> webapprt uses, and they'd be at a discoverable location.

The problem is the latter. AIUI webapprt doesn't have an appid and doesn't use the addon manager, quite rightfully, imho. We may need a special mode to the addon manager, enabling langpacks only.
Comment 10 Myk Melez [:myk] [@mykmelez] 2012-06-08 12:32:56 PDT
(In reply to Mike Hommey [:glandium] from comment #9)
> Bug 747645 comment 0 suggests this is not enough. It's sad that the files
> were moved if that's really all that was needed...

Only one file was moved; the other two were already there (in browser/locales/en-US/webapprt/).


> AIUI webapprt doesn't have an appid

The desktop webapp runtime does actually have an app ID; it's webapprt@mozilla.org:

http://mxr.mozilla.org/mozilla-central/source/webapprt/application.ini.in#10
Comment 11 Mike Hommey [:glandium] 2012-06-08 12:46:56 PDT
(In reply to Myk Melez [:myk] [@mykmelez] from comment #10)
> > AIUI webapprt doesn't have an appid
> 
> The desktop webapp runtime does actually have an app ID; it's
> webapprt@mozilla.org:
> 
> http://mxr.mozilla.org/mozilla-central/source/webapprt/application.ini.in#10

Erf, how could I miss that? But it doesn't enable the addons manager, does it?
Comment 12 Myk Melez [:myk] [@mykmelez] 2012-06-08 12:55:49 PDT
(In reply to Mike Hommey [:glandium] from comment #11)
> But it doesn't enable the addons manager, does it?

The runtime doesn't load addons, but I think the addon manager is enabled in it, since it used to load addons before we explicitly disabled their loading via preferences that are presumably being read by the addon manager.
Comment 13 Axel Hecht [:Pike] 2012-06-08 14:59:24 PDT
(In reply to Mike Hommey [:glandium] from comment #8)
> (In reply to Axel Hecht [:Pike] from comment #7)
> > I don't understand why this blocks XR?
> 
> The webapp runtime, when running on top of XR doesn't have access to browser
> chrome. This issue will become visible on mozilla builds when browser chrome
> moves under a browser/ subdirectory in bug 755724.
> 
> The lack of access to these l10n files makes webapps just display XML errors
> because of missing entities.

We've moved a bunch of stuff under browser, also pdf.js, for example. Does that pose similar problems?
Comment 14 Mike Hommey [:glandium] 2012-06-08 23:27:31 PDT
(In reply to Axel Hecht [:Pike] from comment #13)
> We've moved a bunch of stuff under browser, also pdf.js, for example. Does
> that pose similar problems?

No, these have access to browser chrome. Webapprt is peculiar in that it's the only piece of Firefox that doesn't access browser chrome (until the upcoming Metro UI code, which will have the same property, AIUI)
Comment 15 Mike Hommey [:glandium] 2012-06-09 00:01:11 PDT
Created attachment 631621 [details] [diff] [review]
Ship webapprt locale files in webapprt chrome

This is a (hackish and kind of ugly) way to fix this that doesn't need to change l10n. Would that work for you or should we just move l10n?
Comment 16 Mike Hommey [:glandium] 2012-06-09 00:22:10 PDT
Comment on attachment 631621 [details] [diff] [review]
Ship webapprt locale files in webapprt chrome

In fact, that doesn't work because it puts this in the manifest:
locale webapprt en-US jar:../webapprt/chrome/en-US.jar!/locale/webapprt/
Comment 17 Mike Hommey [:glandium] 2012-06-09 00:35:21 PDT
Created attachment 631626 [details] [diff] [review]
Ship webapprt locale files in webapprt chrome

With an additional change to JarMaker, it works. AFAIK, there are no jar.mn entries in m-c or c-c expecting a different behaviour.
Comment 18 Benjamin Smedberg [:bsmedberg] 2012-06-11 15:05:47 PDT
Comment on attachment 631626 [details] [diff] [review]
Ship webapprt locale files in webapprt chrome

I'm pretty sure that this needs to be in the @AB_CD@ section of package-manifest.in 

This file may need to be added to the clobber-zip target of browser/locales/Makefile.in
Comment 19 Mike Hommey [:glandium] 2012-06-12 00:27:52 PDT
Created attachment 632155 [details] [diff] [review]
Ship webapprt locale files in webapprt chrome
Comment 20 Mike Hommey [:glandium] 2012-06-12 00:28:57 PDT
Comment on attachment 632155 [details] [diff] [review]
Ship webapprt locale files in webapprt chrome

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> Comment on attachment 631626 [details] [diff] [review]
> Ship webapprt locale files in webapprt chrome
> 
> I'm pretty sure that this needs to be in the @AB_CD@ section of
> package-manifest.in 

Doing so doesn't install the locale chrome manifest in webapprt. It makes it install in browser.
Comment 21 Mike Hommey [:glandium] 2012-06-12 00:31:24 PDT
(In reply to Mike Hommey [:glandium] from comment #20)
> Comment on attachment 632155 [details] [diff] [review]
> Ship webapprt locale files in webapprt chrome
> 
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> > Comment on attachment 631626 [details] [diff] [review]
> > Ship webapprt locale files in webapprt chrome
> > 
> > I'm pretty sure that this needs to be in the @AB_CD@ section of
> > package-manifest.in 
> 
> Doing so doesn't install the locale chrome manifest in webapprt. It makes it
> install in browser.

Mmmm I guess not doing so breaks langpacks. Both ways, something is broken :(
Comment 22 Mike Hommey [:glandium] 2012-06-12 00:33:41 PDT
(In reply to Mike Hommey [:glandium] from comment #21)
> Mmmm I guess not doing so breaks langpacks. Both ways, something is broken :(

Actually, it looks like langpacks are just okay with the webapprt locale not being under the @AB_CD@ section. And I don't think it's a problem for repacks.
Comment 23 Axel Hecht [:Pike] 2012-06-12 04:07:31 PDT
Comment on attachment 632155 [details] [diff] [review]
Ship webapprt locale files in webapprt chrome

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

I don't think I can page the relevant pieces into my head, and/or test to make a relevant review any time soon.

Scenarios that confuse me, at least AFAICT:

- single locale
-- omnijar
-- regular jar
-- xulrunner
-- langpack
- multi-locale
-- probably all of the above in some way.
Comment 24 Mike Hommey [:glandium] 2012-06-12 04:27:41 PDT
(In reply to Axel Hecht [:Pike] from comment #23)
> - single locale
> -- omnijar
> -- regular jar
> -- langpack

AFAICT, each of these work with the patch.

> -- xulrunner

xulrunner doesn't enable webapprt (and doesn't have a package manifest)
Comment 25 Benjamin Smedberg [:bsmedberg] 2012-07-10 12:18:59 PDT
Will you need a package-manifest change to go with this? And l10n repackaging? At least somewhere around http://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in#180 ?
Comment 26 Mike Hommey [:glandium] 2012-07-10 14:40:53 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #25)
> Will you need a package-manifest change to go with this? And l10n
> repackaging? At least somewhere around
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.
> in#180 ?

huh? package-manifest and clobber-zip changes are in the patch you r+ed. Not sure about l10n repackaging.
Comment 27 Mike Hommey [:glandium] 2012-07-11 23:32:54 PDT
I tested l10n repackaging locally and added a missing bit for clobber-zip.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4761bf12898b
Comment 28 Axel Hecht [:Pike] 2012-07-11 23:54:24 PDT
Thanks. Maybe trigger a new nightly when this merges to central and watch http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mozilla-l10n for trouble?
Comment 29 Ed Morley [:emorley] 2012-07-12 09:36:18 PDT
https://hg.mozilla.org/mozilla-central/rev/4761bf12898b
Comment 30 Jason Smith [:jsmith] 2012-07-12 11:11:07 PDT
Is there anything to verify here from an end-user perspective?
Comment 31 Mike Hommey [:glandium] 2012-07-13 14:49:40 PDT
(In reply to Jason Smith [:jsmith] from comment #30)
> Is there anything to verify here from an end-user perspective?

That webapps menus work and are localized.
Comment 32 Jason Smith [:jsmith] 2012-07-16 19:50:43 PDT
(In reply to Mike Hommey [:glandium] from comment #31)
> (In reply to Jason Smith [:jsmith] from comment #30)
> > Is there anything to verify here from an end-user perspective?
> 
> That webapps menus work and are localized.

How do I test non-US nightly builds then? On the ftp server, I only see ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central/ which produces an english build for me.

I'd like to test this using a non-US build to see what happens (e.g. Spanish, Japanese). How would I go about that?
Comment 33 Mike Hommey [:glandium] 2012-07-16 23:34:11 PDT
Check ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central-l10n/
Comment 34 Jason Smith [:jsmith] 2012-07-17 11:06:14 PDT
(In reply to Mike Hommey [:glandium] from comment #33)
> Check ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central-l10n/

Yikes. Lots of problems discovered:

- Doorhanger isn't localized
- App Notifications isn't localized on install
- Launching of a web app results in:

XML パースエラー: 定義されていない実体が使用されています。
URL: chrome://webapprt/content/webapp.xul
行番号: 32, 列番号: 3:  <key id="key_undo"
--^

Tested using the Japanese locale build.

Any ideas?
Comment 35 Mike Hommey [:glandium] 2012-07-17 11:38:31 PDT
It might be worth checking with nightlies older than the landing.
Comment 36 Jason Smith [:jsmith] 2012-07-17 11:46:44 PDT
(In reply to Mike Hommey [:glandium] from comment #35)
> It might be worth checking with nightlies older than the landing.

Okay. Tested this with the 7/14/2012 AF build as well and saw the same error occur filed in bug 774772.
Comment 37 Jason Smith [:jsmith] 2012-07-17 11:51:32 PDT
(In reply to Jason Smith [:jsmith] from comment #36)
> (In reply to Mike Hommey [:glandium] from comment #35)
> > It might be worth checking with nightlies older than the landing.
> 
> Okay. Tested this with the 7/14/2012 AF build as well and saw the same error
> occur filed in bug 774772.

Also tested with 7/11/2012 build. The app successfully launches on the 7/11 nightly build. Did this patch break something?
Comment 38 Jason Smith [:jsmith] 2012-07-17 11:51:50 PDT
(In reply to Jason Smith [:jsmith] from comment #37)
> (In reply to Jason Smith [:jsmith] from comment #36)
> > (In reply to Mike Hommey [:glandium] from comment #35)
> > > It might be worth checking with nightlies older than the landing.
> > 
> > Okay. Tested this with the 7/14/2012 AF build as well and saw the same error
> > occur filed in bug 774772.
> 
> Also tested with 7/11/2012 build. The app successfully launches on the 7/11
> nightly build. Did this patch break something?

7/11/2012 AF Build to be exact.
Comment 39 Anant Narayanan [:anant] 2012-07-17 15:26:38 PDT
I can confirm that this patch broke the ability to launch apps installed from a localized version of Firefox. The error is:

XML Parsing Error: undefined entity
Location: chrome://webapprt/content/webapp.xul
Line Number 32, Column 3:  <key id="key_undo"
                         --^
Comment 40 Jason Smith [:jsmith] 2012-07-22 08:51:01 PDT
Verified on Nightly.

Note You need to log in before you can comment on or make changes to this bug.