Remove the obsolete 2nd parameter from |nsIStringBundleService::createBundle(...)| "JS" callers

RESOLVED FIXED in mozilla1.9.1a1

Status

()

defect
--
trivial
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: torisugari, Assigned: sgautherie)

Tracking

Trunk
mozilla1.9.1a1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

Since bug 76332 was fixed, nsIStringBundleService::createBundle(...)
needs only one argument, |aURLSpec|.
> 87   nsIStringBundle createBundle(in string aURLSpec);

However, many scripts have the second argument in that function.
And contributors do not stop using that wrong style, for more than 4 years.
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsIStringBundle.idl&branch=&root=/cvsroot&subdir=mozilla/intl/strres/public&command=DIFF_FRAMESET&rev1=1.13&rev2=1.14

lazy scripts list:
http://lxr.mozilla.org/mozilla/source/browser/base/content/search.xml
http://lxr.mozilla.org/mozilla/source/browser/components/bookmarks/content/bookmarks.js
http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/browser.xml
http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/dialog.xml
http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/stringbundle.xml
http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/wizard.xml
http://lxr.mozilla.org/mozilla/source/toolkit/content/contentAreaUtils.js
http://lxr.mozilla.org/mozilla/source/xpfe/communicator/resources/content/contentAreaUtils.js
http://lxr.mozilla.org/mozilla/source/xpfe/components/bookmarks/resources/bookmarks.js
http://lxr.mozilla.org/mozilla/source/xpfe/global/resources/content/bindings/browser.xml
http://lxr.mozilla.org/mozilla/source/xpfe/global/resources/content/bindings/dialog.xml
http://lxr.mozilla.org/mozilla/source/xpfe/global/resources/content/bindings/stringbundle.xml
http://lxr.mozilla.org/mozilla/source/xpfe/global/resources/content/bindings/wizard.xml
Sounds valid, no duplicates found - confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
Posted patch patch for xpfe (obsolete) — Splinter Review
Assignee: nobody → torisugari
Status: NEW → ASSIGNED
I'm not too sure I should delete |appLocale| property in <xul:stringbundle>

http://xulplanet.com/references/elemref/ref_stringbundle.html#prop_appLocale

I guess nobody (among any extension authors) are using that property,
but changing XUL spec is something awful...
Attachment #204112 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #204112 - Flags: review?(cbiesinger)
Comment on attachment 204112 [details] [diff] [review]
patch for xpfe

>       this._brandShortName = BUNDLESVC.createBundle(brandBundle,     LOCALESVC.getApplicationLocale())
Nit: missed one :-P
Attachment #204112 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #204112 - Flags: superreview+
Attachment #204112 - Flags: review?(cbiesinger)
Attachment #204112 - Flags: review+
Torisugari, were you still planning on working on this?
Assignee: torisugari → smontagu
Severity: minor → trivial
Status: ASSIGNED → NEW
Component: General → Internationalization
Depends on: 76333
Product: Firefox → Core
QA Contact: general → i18n
Depends on: 76332
No longer depends on: 76333
</xpfe/> patch, with comment 4 suggestion(s),
and ported to </Suite/>,
with further rewrite.
Assignee: smontagu → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #328182 - Flags: superreview?(neil)
Attachment #328182 - Flags: review?(neil)
Attachment #328182 - Flags: superreview?(neil)
Attachment #328182 - Flags: superreview+
Attachment #328182 - Flags: review?(neil)
Attachment #328182 - Flags: review+
Keywords: checkin-needed
Whiteboard: [c-n: Cv1 // Leave opened]
Posted patch (Dv1) </browser/*/*> (obsolete) — Splinter Review
[Unified diff; no(t) Hg.]

Too bad this function is duplicated...

NB: <bookmarks.js>, from the older patch, doesn't exist anymore.
Attachment #328238 - Flags: review?(zeniko)
Attachment #328238 - Attachment description: (Cv1) </browser/*/*> → (Dv1) </browser/*/*>
Attachment #204112 - Attachment is obsolete: true
Attachment #204114 - Attachment is obsolete: true
Comment on attachment 328238 [details] [diff] [review]
(Dv1) </browser/*/*>

Thanks, Serge. BTW: You could also just completely remove _getStringBundle from nsSessionStore.js, as it's never been used at all (since the code needing it was moved to nsSessionStartup.js).
Attachment #328238 - Flags: review?(zeniko) → review+
Dv1, with comment 8 suggestion(s),
and related nits.
Attachment #328238 - Attachment is obsolete: true
Attachment #328253 - Flags: review?(zeniko)
(In reply to comment #5)
> Torisugari, were you still planning on working on this?

He emailed me that he is not.
Attachment #328253 - Flags: review?(zeniko) → review+
Whiteboard: [c-n: Cv1 // Leave opened] → [c-n: Cv1, Dv1a // Leave opened]
Depends on: 142502, 323492
Flags: in-testsuite-
Posted patch (Ev1) </toolkit/*/*> (obsolete) — Splinter Review
Older </toolkit/> patch,
with further rewrite.

2 untested/uncompiled points to double check:
*<nsNavHistoryResult.cpp>: bug 323492 should have removed these lines too, shouldn't it ?
*<stringbundle.xml> removal of |<property name="appLocale">|: see comment 3.

NB: <wizard.xml> use of its |var localeService| was removed by bug 142502.
Attachment #328289 - Flags: review?
Attachment #328289 - Flags: review? → review?(mano)
Comment on attachment 328289 [details] [diff] [review]
(Ev1) </toolkit/*/*>

r=mano
Attachment #328289 - Flags: review?(mano) → review+
Summary: Remove the 2nd param from nsIStringBundleService::createBundle(...) → Remove the obsolete 2nd parameter from |nsIStringBundleService::createBundle(...)| "JS" callers
Whiteboard: [c-n: Cv1, Dv1a // Leave opened] → [c-n: Cv1, Dv1a, Ev1 // Leave opened]
Target Milestone: --- → mozilla1.9.1a1
No other cases to fix (in mozilla & mozilla-central) :-)
Whiteboard: [c-n: Cv1, Dv1a, Ev1 // Leave opened] → [c-n: Cv1, Dv1a, Ev1]
Blocks: 444021
(In reply to comment #11)
> Created an attachment (id=328289) [details]
> (Ev1) </toolkit/*/*>
> 
> Older </toolkit/> patch,
> with further rewrite.
> 
> 2 untested/uncompiled points to double check:
> *<nsNavHistoryResult.cpp>: bug 323492 should have removed these lines too,
> shouldn't it ?
> *<stringbundle.xml> removal of |<property name="appLocale">|: see comment 3.
> 
> NB: <wizard.xml> use of its |var localeService| was removed by bug 142502.
> 

>diff -r b1e8d5739778 -r 0c65c6af8987 toolkit/content/widgets/wizard.xml
>--- a/toolkit/content/widgets/wizard.xml	Mon Jul 07 02:52:56 2008 +0200
>+++ b/toolkit/content/widgets/wizard.xml	Mon Jul 07 04:41:08 2008 +0200
>@@ -175,12 +175,9 @@
>         try {
>           // need to create string bundle manually instead of using <xul:stringbundle/>
>           // see bug 63370 for details
>-          var localeService = Components.classes["@mozilla.org/intl/nslocaleservice;1"]
>-                                .getService(Components.interfaces.nsILocaleService);
>-          var stringBundleService = Components.classes["@mozilla.org/intl/stringbundle;1"]
>-                                      .getService(Components.interfaces.nsIStringBundleService);
>-          var bundleURL = "chrome://global/locale/wizard.properties";
>-          this._bundle = stringBundleService.createBundle(bundleURL);
>+          this._mStrBundle = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                                       .getService(Components.interfaces.nsIStringBundleService)
>+                                       .createBundle("chrome://global/locale/wizard.properties");
>         } catch (e) {
>           // This fails in remote XUL, which has to provide titles for all pages
>           // see bug 142502
>

Are you sure to change from |this._bundle| to |this._mStrBundle| ?
Ev1, with comment 14 suggestion(s).

(Good catch ! I double checked again...)
Attachment #328289 - Attachment is obsolete: true
Whiteboard: [c-n: Cv1, Dv1a, Ev1] → [c-n: Cv1, Dv1a, Ev1a]
Comment on attachment 328182 [details] [diff] [review]
(Cv1) </suite/*/*>
[Checkin: Comment 16]

Checking in bookmarks/bookmarks.js;
/cvsroot/mozilla/suite/common/bookmarks/bookmarks.js,v  <--  bookmarks.js
new revision: 1.145; previous revision: 1.144
done
Checking in contentAreaUtils.js;
/cvsroot/mozilla/suite/common/contentAreaUtils.js,v  <--  contentAreaUtils.js
new revision: 1.154; previous revision: 1.153
done
Attachment #328182 - Attachment description: (Cv1) </suite/*/*> → (Cv1) </suite/*/*> (checked in)
Attachment #328182 - Attachment description: (Cv1) </suite/*/*> (checked in) → (Cv1) </suite/*/*> [Checkin: Comment 16]
Whiteboard: [c-n: Cv1, Dv1a, Ev1a] → [c-n: Dv1a, Ev1a]
http://hg.mozilla.org/index.cgi/mozilla-central/rev/d4c293d33170
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: Dv1a, Ev1a]
Attachment #328253 - Attachment description: (Dv1a) </browser/*/*> → (Dv1a) </browser/*/*> [Checkin: Comment 17]
Attachment #328535 - Attachment description: (Ev1a) </toolkit/*/*> → (Ev1a) </toolkit/*/*> [Checkin: Comment 17]
Blocks: 444955
You need to log in before you can comment on or make changes to this bug.