Closed Bug 1335877 Opened 3 years ago Closed 3 years ago

Remove resource://services-common/stringbundle.js from gecko

Categories

(Core :: Internationalization, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

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: nobody → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8832755 [details]
Bug 1335877 - Remove resource://services-common/stringbundle.js.

Does it look reasonable?
Attachment #8832755 - Flags: feedback?(l10n)
Attachment #8832755 - Flags: feedback?(l10n)
Comment on attachment 8832755 [details]
Bug 1335877 - Remove resource://services-common/stringbundle.js.

lgtm.
Attachment #8832755 - Flags: feedback?(l10n) → feedback+
Priority: -- → P3
Attachment #8832755 - Attachment is obsolete: true
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)
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)
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)
ok, I think I cleared the errors related to this bug. Ready for 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)
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 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+
(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)
Thank you for the quick review! :)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0bd17b868a31
Remove resource://services-common/stringbundle.js. r=rnewman
(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.
https://hg.mozilla.org/mozilla-central/rev/0bd17b868a31
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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 → ---
Attachment #8843115 - Attachment is obsolete: true
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&regexp=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+
Attachment #8844053 - Flags: review+ → review?(gandalf)
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+
Attachment #8844053 - Flags: review?(rnewman)
Attachment #8844053 - Flags: review?(rnewman) → review+
Attachment #8844053 - Flags: review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18b7e4434612
Remove resource://services-common/stringbundle.js from gecko. r=gandalf
https://hg.mozilla.org/mozilla-central/rev/18b7e4434612
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.