Closed Bug 762864 Opened 12 years ago Closed 12 years ago

Webapp runtime locale files are part of browser, not webapprt

Categories

(Firefox Graveyard :: Webapp Runtime, defect, P3)

Product:
Firefox Graveyard
Component:
Webapp Runtime
Version:
14 Branch
Platform:
x86_64
Linux
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
Firefox 16

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Whiteboard: [qa!])

Attachments

(1 file, 2 obsolete files)

Ship webapprt locale files in webapprt chrome
3.58 KB, patch
benjamin
: review+
Details | Diff | Splinter Review 
This effectively prevents webapprt from working in FF-on-XR, but also will make things not work very smoothly with bug 755724.
Relatedly, webapprt probably doesn't work with langpacks.
This sounds like this is related to the result of the implementation of bug 747645.
(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.
Actually, we could "cheat" and keep the locale files under browser in the source but install them in webapprt.
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)
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/.
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.
(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.
(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.
(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
(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?
(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.
(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?
Priority: -- → P3
(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)
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?
Attachment #631621 - Flags: review?(l10n)
Attachment #631621 - Flags: review?(benjamin)
Assignee: nobody → mh+mozilla
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/
Attachment #631621 - Flags: review?(l10n)
Attachment #631621 - Flags: review?(benjamin)
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.
Attachment #631621 - Attachment is obsolete: true
Attachment #631626 - Flags: review?(l10n)
Attachment #631626 - Flags: review?(benjamin)
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
Attachment #631626 - Flags: review?(benjamin) → review-
Attachment #631626 - Attachment is obsolete: true
Attachment #631626 - Flags: review?(l10n)
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.
Attachment #632155 - Flags: review?(l10n)
Attachment #632155 - Flags: review?(benjamin)
(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 :(
(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 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.
Attachment #632155 - Flags: review?(l10n)
(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)
QA Contact: jsmith
Attachment #632155 - Flags: review?(benjamin) → review+
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 ?
(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.
I tested l10n repackaging locally and added a missing bit for clobber-zip.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4761bf12898b
Target Milestone: --- → Firefox 16
Thanks. Maybe trigger a new nightly when this merges to central and watch http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mozilla-l10n for trouble?
https://hg.mozilla.org/mozilla-central/rev/4761bf12898b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Is there anything to verify here from an end-user perspective?
Whiteboard: [qa?]
(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.
Whiteboard: [qa?] → [qa+]
(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?
(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?
Depends on: 774772
Whiteboard: [qa+] → [qa verification failed]
It might be worth checking with nightlies older than the landing.
(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.
(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?
(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.
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"
                         --^
Whiteboard: [qa verification failed] → [qa+]
Verified on Nightly.
Status: RESOLVED → VERIFIED
status-firefox15: affected → ---
Whiteboard: [qa+] → [qa!]
Blocks: 844016
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: