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

Status: RESOLVED FIXED

(Firefox Graveyard :: Webapp Runtime, defect)

Description

[Justin Wood (:Callek)]

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.

Comment 1

[Dave Townsend [:mossop]]

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?

Comment 2

[Edmund Wong (:ewong)]

Attachment: Moved Webapp RT l10n code from browser/ to toolkit/

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

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 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 6

[Edmund Wong (:ewong)]

Attachment: Move Webapp RT l10n from browser/ to toolkit. (v2)

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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?

Comment 8

[Edmund Wong (:ewong)]

Attachment: Move Webapps RT l10n from browser/ to toolkit/ (v3)

Comment 9

[Myk Melez [:myk] [@mykmelez]]

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 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 11

[Edmund Wong (:ewong)]

Attachment: Moved Webapp l10n strings from browser/ to toolkit/ (v4)

fixed nits.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e21e4ace8b7

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7e21e4ace8b7