Closed
Bug 1335877
Opened 7 years ago
Closed 7 years ago
Remove resource://services-common/stringbundle.js from gecko
Categories
(Core :: Internationalization, defect, P3)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: Pike, Assigned: zbraniecki)
References
Details
Attachments
(1 file, 2 obsolete files)
http://searchfox.org/mozilla-central/source/services/common/stringbundle.js should be removed in favor of Services.strings. The js file is full of DEPRECATED notices, but really, the whole thing shouldn't exist. Wrapper APIs are evil. I found this while trying to make a call on call sites to getApplicationLocale, which this file calls, and passes to the string bundle service as an argument that doesn't exist. Best fix is to remove this. https://dxr.mozilla.org/mozilla-central/search?q=stringbundle.js&redirect=true for call sites.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
Comment on attachment 8832755 [details] Bug 1335877 - Remove resource://services-common/stringbundle.js. Does it look reasonable?
Attachment #8832755 -
Flags: feedback?(l10n)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8832755 -
Flags: feedback?(l10n)
Reporter | ||
Comment 4•7 years ago
|
||
Comment on attachment 8832755 [details] Bug 1335877 - Remove resource://services-common/stringbundle.js. lgtm.
Attachment #8832755 -
Flags: feedback?(l10n) → feedback+
Updated•7 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8832755 -
Attachment is obsolete: true
Assignee | ||
Comment 6•7 years ago
|
||
Comment on attachment 8843115 [details] Bug 1335877 - Remove resource://services-common/stringbundle.js. Flagging :rnewman for feedback since he shows up in blame as either an author or reviewer for a few patches that added dependency on stringbundle.js that I'm trying to remove.
Attachment #8843115 -
Flags: feedback?(rnewman)
Assignee | ||
Comment 7•7 years ago
|
||
Comment on attachment 8843115 [details] Bug 1335877 - Remove resource://services-common/stringbundle.js. triggered an error, need to fix it.
Attachment #8843115 -
Flags: review?(jfkthame)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Ugh, so apparently MR removes f requests, so settings a NI on :rnewman instead to let him know we're changing the code.
Flags: needinfo?(rnewman)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
ok, I think I cleared the errors related to this bug. Ready for review.
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8843115 [details] Bug 1335877 - Remove resource://services-common/stringbundle.js. https://reviewboard.mozilla.org/r/116878/#review118634 I'm really not the right reviewer for this....I don't have any connection to this services code, and JS is largely a foreign language to me. ;)
Attachment #8843115 -
Flags: review?(jfkthame)
Assignee | ||
Comment 13•7 years ago
|
||
Comment on attachment 8843115 [details] Bug 1335877 - Remove resource://services-common/stringbundle.js. Hmm, then maybe Richard will be able to review that? :)
Attachment #8843115 -
Flags: review?(rnewman)
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8843115 [details] Bug 1335877 - Remove resource://services-common/stringbundle.js. https://reviewboard.mozilla.org/r/116878/#review118712 ::: browser/base/content/test/general/browser_misused_characters_in_strings.js:195 (Diff revision 3) > // our zipreader APIs are all sync) > let uris = yield getAllTheFiles(".properties"); > ok(uris.length, `Found ${uris.length} .properties files to scan for misused characters`); > > for (let uri of uris) { > - let bundle = new StringBundle(uri.spec); > + let bundle = Services.strings.createBundle(uri.spec); This change doesn't seem like it should be in this patch -- I presume you're fixing the whole tree in a series?
Attachment #8843115 -
Flags: review?(rnewman) → review+
Comment 15•7 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #14) > This change doesn't seem like it should be in this patch -- I presume you're > fixing the whole tree in a series? Disregard this. (I dropped the issue in RB.) (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6) > Flagging :rnewman for feedback since he shows up in blame as either an > author or reviewer for a few patches that added dependency on > stringbundle.js that I'm trying to remove. This code was added in changeset: 45658:4f4088a4b2ed user: Edward Lee <edilee@mozilla.com> date: Thu Aug 20 01:09:28 2009 -0700 summary: Add Myk's StringBundle to modules/ext -- it looks like the goal at the time, eight years ago, was to eliminate the super-verbose XBL version and replace it with a nice JSey API. It looks like that will no longer happen! Everything in the meantime was just following module conventions.
Flags: needinfo?(rnewman)
Assignee | ||
Comment 16•7 years ago
|
||
Thank you for the quick review! :)
Comment 17•7 years ago
|
||
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0bd17b868a31 Remove resource://services-common/stringbundle.js. r=rnewman
Comment 18•7 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #0) > http://searchfox.org/mozilla-central/source/services/common/stringbundle.js > should be removed in favor of Services.strings. > > The js file is full of DEPRECATED notices, Those methods were intentionally deprecated but not removed when the module was originally created, because I didn't want consumers to use the old API, but I did want to ease the transition for consumers who were already using it. > but really, the whole thing > shouldn't exist. Wrapper APIs are evil. If wrapper APIs didn't exist, they'd have to be invented (cf. the web). > I found this while trying to make a call on call sites to > getApplicationLocale, which this file calls, and passes to the string bundle > service as an argument that doesn't exist. Anymore. It did exist when the module was originally created (for use by extensions). It had been removed by the time the module landed in Central, however, so this API call should have been updated then.
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0bd17b868a31
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 20•7 years ago
|
||
backed out in https://hg.mozilla.org/mozilla-central/rev/517c553ad64746c479456653ce11b04ab8e4977f for causing the regression / Bug 1344760
Status: RESOLVED → REOPENED
Flags: needinfo?(gandalf)
Resolution: FIXED → ---
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8843115 -
Attachment is obsolete: true
Assignee | ||
Comment 22•7 years ago
|
||
Comment on attachment 8844053 [details] Bug 1335877 - Remove resource://services-common/stringbundle.js from gecko. Updated the patch. The bug is in one call to the old API in utils.js. I did follow up STR from bug 1344760 to verify that login into sync, switching to sync pane works. I also grepped for all strings of the same nature: http://searchfox.org/mozilla-central/search?q=Str%5C..%2B%5C.get&case=true®exp=true&path=.js And this was the only one that I did not switch in my original patch, so I do have high hopes that there are no more regressions of that nature in this patch. And since it's not covered in tests, I don't expect a try run to be necessary. Will reland.
Flags: needinfo?(gandalf)
Attachment #8844053 -
Flags: review?(rnewman) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8844053 -
Flags: review+ → review?(gandalf)
Assignee | ||
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8844053 [details] Bug 1335877 - Remove resource://services-common/stringbundle.js from gecko. https://reviewboard.mozilla.org/r/117608/#review119292 carrying over r+
Attachment #8844053 -
Flags: review?(gandalf) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8844053 -
Flags: review?(rnewman)
Assignee | ||
Updated•7 years ago
|
Attachment #8844053 -
Flags: review?(rnewman) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8844053 -
Flags: review+
Comment 24•7 years ago
|
||
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/18b7e4434612 Remove resource://services-common/stringbundle.js from gecko. r=gandalf
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/18b7e4434612
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•