Closed
Bug 475283
Opened 16 years ago
Closed 15 years ago
contentAreaUtils.js is polluting global namespace with getStringBundle(), replace it with ContentAreaUtils.stringBundle
Categories
(Toolkit :: XUL Widgets, defect)
Toolkit
XUL Widgets
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a4
People
(Reporter: BenB, Assigned: Paolo)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
Reproduction:
function getStringBundle(bundleURI)
{
try {
Components.classes["@mozilla.org/intl/stringbundle;1"]
.getService(Components.interfaces.nsIStringBundleService)
.createBundle(bundleURI);
return gStringBundleServiceCache.createBundle(bundleURI);
} catch (e) {
throw new Exception("Failed to get stringbundle URI <" + bundleURI + ">. Error: " + e);
}
}
1. alert(getStringBundle("chrome://messenger/locale/accountCreationUtil.properties").GetStringFromName("cannot_contact_server.error"));
2. alert(getStringBundle("chrome://messenger/locale/accountCreationUtil.properties").GetStringFromName("DefaultSaveFileName"));
Actual result:
1: throws [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]" nsresult: "0x80004005 (NS_ERROR_FAILURE)"
2: returns "index" from toolkit/locales/en-US/chrome/global/contentAreaCommands.properties
Obviously, toolkit/content/contentAreaUtils.js line 681 is defining getStringBundle(), and polluting the global namespace (of anybody who includes it) with its private little string bundle.
contentAreaUtil.js was included in our window by some other developer.
If I rename my getStringBundle() to getStringBundle2(), it works (2 works, 1 fails, as expected).
Expected result:
Toolkit scripts don't use generically sounding names for things which are not generic and not for public use nor defined.
Severity: I just spent 2 hours trying to figure out why the string can't be found.
Fix:
Rename getStringBundle() in contentAreaUtil.js to getContentAreaUtilsStringBundle() (3 usages only).
Comment 1•16 years ago
|
||
What are you suggesting we do to fix this bug?
Any function name we choose is going to "pollute the global namespace", and changing to some more obscure name is likely to break more extensions than it fixes, given that this function is generally useful, and has had that name for at least 4 years and multiple Firefox releases.
I think this is WONTFIX.
Comment 2•16 years ago
|
||
Sorry, I missed your suggested fix.
I just did a search on http://mxr-test.konigsberg.mozilla.org/addons/ (index of the source of all AMO extensions) and it appears to not be a commonly called as I expected it to be (I couldn't find a caller that depended on the toolkit version).
I'm still wary of making changes like these (that index is old and represents only a subset of Firefox extensions), but perhaps we could try it if we do it early in a cycle (e.g. now, on trunk).
Reporter | ||
Comment 3•16 years ago
|
||
Yes. I don't think it's very reasonable for an extension to rely on this function. Maybe they use the properties files, but most likely not the function, with this name. If they do, it's probably an accident like in my case.
Reporter | ||
Comment 4•16 years ago
|
||
Patch, v1
Untested so far. Need to update FF. Will report on test results.
Assignee: nobody → ben.bucksch
Reporter | ||
Comment 5•16 years ago
|
||
Comment on attachment 358880 [details] [diff] [review]
Patch, v1
I'm running with the patch since a while and haven't noticed any problems.
Attachment #358880 -
Flags: review?(gavin.sharp)
Updated•15 years ago
|
Comment 6•15 years ago
|
||
Please, rather port bug 445831 ;-)
Assignee | ||
Comment 7•15 years ago
|
||
Ben, reducing global namespace pollution seems like a good idea. The patch for
bug 506116 already took a step in this direction, introducing this namespace:
+var ContentAreaUtils = {
+ get ioService() {
+ delete this.ioService;
+ return this.ioService =
+ Components.classes["@mozilla.org/network/io-service;1"]
+ .getService(Components.interfaces.nsIIOService);
+ }
+}
I think that reimplementing getStringBundle as a getter on the
ContentAreaUtils object would be great, maybe using the same caching
technique as above.
Since it is meant for internal use, I'd also prefix the getter name
with an underscore (something like "get _stringBundle()" or
"get _commandsStringBundle()").
Let's also hear Gavin's thoughts on this.
Assignee | ||
Comment 8•15 years ago
|
||
(In reply to comment #6)
> Please, rather port bug 445831 ;-)
While this may be appropriate for SeaMonkey, I advise against using this
technique in Toolkit, since it introduces a dependency on the XUL document
where the script is loaded, which would then need to define a specific
<stringbundle> element.
Comment 9•15 years ago
|
||
(In reply to comment #7)
> I think that reimplementing getStringBundle as a getter on the
> ContentAreaUtils object would be great, maybe using the same caching
> technique as above.
>
> Since it is meant for internal use, I'd also prefix the getter name
> with an underscore (something like "get _stringBundle()" or
> "get _commandsStringBundle()").
Makes sense to me.
Updated•15 years ago
|
Attachment #358880 -
Flags: review?(gavin.sharp)
Comment 10•15 years ago
|
||
Comment on attachment 358880 [details] [diff] [review]
Patch, v1
Going to wait for a patch that addresses the comments in comment 7.
Assignee | ||
Comment 11•15 years ago
|
||
Using a "smart getter" like the one for ioService might also improve
performance slightly when the string bundle is needed.
Reporter | ||
Comment 12•15 years ago
|
||
Paolo, feel free to take it over.
Assignee | ||
Comment 13•15 years ago
|
||
(In reply to comment #12)
> Paolo, feel free to take it over.
Thanks, I'm about to do other changes in the same file so it makes sense
that I do this small piece too.
Assignee: ben.bucksch → paolo.02.prg
Assignee | ||
Comment 14•15 years ago
|
||
Attachment #358880 -
Attachment is obsolete: true
Attachment #430734 -
Flags: review?(gavin.sharp)
Reporter | ||
Comment 15•15 years ago
|
||
Thanks, Paolo, for taking over!
Looking at the patch, it looks good, with 2 exceptions:
+ get _stringBundle() {
+ delete this._stringBundle;
+ return this._stringBundle = ....createBundle(...);
}
1. don't delete it.
Rather, do:
get _stringBundle() {
if (!this.__stringBundle)
this.__stringBundle = ....createBundle(...);
return this.__stringBundle;
}
2. You're using the same name for the getter and the internal properly variable. I'm not sure, but I think they should have separate names. That's why I above used 2x _ for the variable above.
+ delete ContentAreaUtils._stringBundle;
+ ContentAreaUtils._stringBundle = {
+ GetStringFromName: function() ""
+ };
You should implement formatStringFromName() as well.
Comment 16•15 years ago
|
||
(In reply to comment #15)
> 1. don't delete it.
> Rather, do:
> get _stringBundle() {
> if (!this.__stringBundle)
> this.__stringBundle = ....createBundle(...);
> return this.__stringBundle;
> }
Couldn't disagree more! :) The way the patch is now is the canonical way to implement memoizing getters - it avoids the function call overhead for subsequent accesses. It's fine as is.
(You could also use XPCOMUtil's defineLazyGetter, but no need to pull in that entire module for a single getter if it isn't already imported.)
Assignee | ||
Comment 17•15 years ago
|
||
(In reply to comment #15)
> + delete ContentAreaUtils._stringBundle;
> + ContentAreaUtils._stringBundle = {
> + GetStringFromName: function() ""
> + };
>
> You should implement formatStringFromName() as well.
Well, I thought the same thing, but I saw that the approach in the rest of
this test file is to create mock objects that define just the functions that
are called in the tested code paths. nsIStringBundle contains four other
methods including formatStringFromName, adding stubs for each of them
doesn't hurt but I preferred to preserve the style of this part of the code.
Comment 18•15 years ago
|
||
A few extensions seem to use this to access the filesFolder, WebPageCompleteFilter and DefaultSaveFileName strings. Maybe this should remain public, just with a better name.
Assignee | ||
Comment 19•15 years ago
|
||
(In reply to comment #18)
> A few extensions seem to use this to access the filesFolder,
> WebPageCompleteFilter and DefaultSaveFileName strings.
The one I'm maintaining is one of them :-)
> Maybe this should remain public, just with a better name.
I'm not so sure this function was intended as a public API to begin with,
nor the contents of the string bundle itself. Extensions use them because
they have access to virtually anything, but authors know that they must
be prepared to update their code in response to changes like this, here
or in any other part of the code. As comment 2 suggests, if this change
is done on trunk we're probably safe enough, and to be safer we can
mention it in the release notes for extension developers.
I think the name "_stringBundle" for the getter on the ContentAreaUtils
object is as good as any other name, since in practice there's nothing
that stops extensions from accessing it; as I see it, the underscore in
the name is a hint that tells extension authors that they must be prepared
to see this function disappear without this being mentioned in the release
notes, and Toolkit developers that they can safely operate on the function
without wondering at each step if someone depends on its behavior or not.
Comment 20•15 years ago
|
||
Comment on attachment 430734 [details] [diff] [review]
The change, with test case using file picker title keys
I don't really see any benefit to the underscore - we're unlikely to be changing this property again, and I don't think we really need to try and discourage its use (and the underscore probably isn't really an effective way to do that anyways). So r=me without it.
Attachment #430734 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 21•15 years ago
|
||
Ok, I'll generate a new patch where getStringBundle is a public getter (it's
true that it makes little difference). It's just that when I imagined an MDC
page documenting the public API of the ContentAreaUtils object and the rest
of the file (like the pages that exist for Places), getStringBundle seemed
out of place.
For other functions like internalPersist, are you fine with using the
underscore to mark them as private in case we move them inside the new
namespace in the future, even if extensions happen to use them?
Assignee | ||
Comment 22•15 years ago
|
||
Attachment #430734 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 23•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Summary: contentAreaUtils.js is polluting global namespace with getStringBundle() → contentAreaUtils.js is polluting global namespace with getStringBundle(), replace it with ContentAreaUtils.stringBundle
Target Milestone: --- → mozilla1.9.3a4
Reporter | ||
Comment 24•15 years ago
|
||
We gave birth!
(Thanks all :) )
You need to log in
before you can comment on or make changes to this bug.
Description
•