Webapp RT code living in toolkit should not load l10n from browser/

RESOLVED FIXED in Firefox 18

Status

defect
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: Callek, Assigned: ewong)

Tracking

unspecified
Firefox 18
x86_64
Windows 7
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

7 years ago
So, in practice and by way of being an html5-based app system, WebbappRT should be able to work on other applications (Such as SeaMonkey) that base themselves off Gecko+Toolkit.

The fact that toolkit/webapps/WebappsInstaller.jsm happens to live in toolkit even suggests this "should" be possible.

However, it appears that many strings for it are being taken from browser/ which prevents Mobile, SeaMonkey, or any other seperately managed project from reusing this code, since it is trying to load the properties from a non-existing location.

I noticed this with Bug 780530 landing, however it appears its a problem in more than just that one bug/patch.

This is *keeping* some tests orange on SeaMonkey due to the JS errors it throws, and using up a lot of machine time, due to the exceptions+timeouts this causes.
Toolkit code shouldn't depend on browser wherever possible, is this just as simple as moving the strings to somewhere in toolkit or is there something else going on here?
Assignee

Comment 2

7 years ago
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #654134 - Flags: review?(bmcbride)
Comment on attachment 654134 [details] [diff] [review]
Moved Webapp RT l10n code from browser/ to toolkit/

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

(Copy&pasting from IRC, so it's not forgotten.)

Will have a deeper look later tonight, but I think browser.properties is the wrong string bundle for that (it should probably be named browsewithcaret.properties). If there's not a bundle in toolkit for webapprt, may as well make one now.
Providing context: the WebappsInstaller.jsm is basically Firefox code that was written in browser/ at first, but it got in a weird situation that it needs to be used by both Firefox and the webapp runtime, so that's why it got moved to a place in toolkit where it's accessible by both.

Now, these strings used by the (un)installer can certainly be moved to toolkit. But other strings used in the webapprt itself were originally created in browser/ on purpose to make it easier to handle them on the l10n tools. But I believe that they have since been moved as well. Bug 747645 and bug 762864 have all the details.
Comment on attachment 654134 [details] [diff] [review]
Moved Webapp RT l10n code from browser/ to toolkit/

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

This seems to be sound, just needs a couple of fixes for code polish :)

::: toolkit/webapps/WebappsInstaller.jsm
@@ +801,4 @@
>      let factory = Cc["@mozilla.org/xpcom/ini-processor-factory;1"]
>                      .getService(Ci.nsIINIParserFactory);
>  
> +    let browserBundle = Services.strings.createBundle("chrome://global/locale/browser.properties");

Should rename this variable to something like webappsBundle/strBundle/something that doesn't have "browser" in it.
Attachment #654134 - Flags: review?(bmcbride) → review-
Assignee

Comment 6

7 years ago
Attachment #654134 - Attachment is obsolete: true
Attachment #654193 - Flags: review?(bmcbride)
Comment on attachment 654193 [details] [diff] [review]
Move Webapp RT l10n from browser/ to toolkit. (v2)

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

Forgot comment 3?
Attachment #654193 - Flags: review?(bmcbride) → review-
Assignee

Comment 8

7 years ago
Attachment #654193 - Attachment is obsolete: true
Attachment #654511 - Flags: review?(bmcbride)
Comment on attachment 654511 [details] [diff] [review]
Move Webapps RT l10n from browser/ to toolkit/ (v3)

>diff --git a/toolkit/locales/en-US/chrome/global/webapps.properties b/toolkit/locales/en-US/chrome/global/webapps.properties
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/locales/en-US/chrome/global/webapps.properties
>@@ -0,0 +1,7 @@
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+# LOCALIZATION NOTE (webapps.uninstall.notification): %S will be replaced with the name of the uninstalled web app
>+webapps.uninstall.notification = %S has been uninstalled from your computer.
>+webapps.uninstall.label = Uninstall App

Drive-by nit: now that these strings are in a webapps-specific file, their names should drop the "webapps." prefix.
Comment on attachment 654511 [details] [diff] [review]
Move Webapps RT l10n from browser/ to toolkit/ (v3)

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

r+ with comment 9 addressed.
Attachment #654511 - Flags: review?(bmcbride) → review+
Assignee

Comment 11

7 years ago
fixed nits.
Attachment #654511 - Attachment is obsolete: true
Attachment #654899 - Flags: review+
Assignee

Updated

7 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7e21e4ace8b7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.