The default bug view has changed. See this FAQ.

Webapp runtime locale files are part of browser, not webapprt

VERIFIED FIXED in Firefox 16

Status

Firefox Graveyard
Webapp Runtime
P3
normal
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

14 Branch
Firefox 16
x86_64
Linux
Dependency tree / graph

Details

(Whiteboard: [qa!])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
This effectively prevents webapprt from working in FF-on-XR, but also will make things not work very smoothly with bug 755724.
(Assignee)

Updated

5 years ago
status-firefox14: --- → affected
status-firefox15: --- → affected
(Assignee)

Comment 1

5 years ago
Relatedly, webapprt probably doesn't work with langpacks.

Updated

5 years ago
status-firefox14: affected → ---
This sounds like this is related to the result of the implementation of bug 747645.
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Actually, we could "cheat" and keep the locale files under browser in the source but install them in webapprt.
(Assignee)

Comment 5

5 years ago
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/.

Comment 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
(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
(Assignee)

Comment 11

5 years ago
(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.

Comment 13

5 years ago
(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
(Assignee)

Comment 14

5 years ago
(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)
(Assignee)

Comment 15

5 years ago
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?
(Assignee)

Updated

5 years ago
Attachment #631621 - Flags: review?(l10n)
Attachment #631621 - Flags: review?(benjamin)
(Assignee)

Updated

5 years ago
Assignee: nobody → mh+mozilla
(Assignee)

Comment 16

5 years ago
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)
(Assignee)

Comment 17

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #631621 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 19

5 years ago
Created attachment 632155 [details] [diff] [review]
Ship webapprt locale files in webapprt chrome
(Assignee)

Updated

5 years ago
Attachment #631626 - Attachment is obsolete: true
Attachment #631626 - Flags: review?(l10n)
(Assignee)

Comment 20

5 years ago
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)
(Assignee)

Comment 21

5 years ago
(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 :(
(Assignee)

Comment 22

5 years ago
(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

5 years ago
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)
(Assignee)

Comment 24

5 years ago
(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)

Updated

5 years ago
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 ?
(Assignee)

Comment 26

5 years ago
(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.
(Assignee)

Comment 27

5 years ago
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

Comment 28

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Is there anything to verify here from an end-user perspective?
Whiteboard: [qa?]
(Assignee)

Comment 31

5 years ago
(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.

Updated

5 years ago
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?
(Assignee)

Comment 33

5 years ago
Check ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central-l10n/
(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?

Updated

5 years ago
Depends on: 774772
Whiteboard: [qa+] → [qa verification failed]
(Assignee)

Comment 35

5 years ago
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"
                         --^

Updated

5 years ago
Whiteboard: [qa verification failed] → [qa+]
Verified on Nightly.
Status: RESOLVED → VERIFIED
status-firefox15: affected → ---
Whiteboard: [qa+] → [qa!]
(Assignee)

Updated

4 years ago
Blocks: 844016
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.