Remove ExtensibleStringBundle support

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

Last year
These aren't used by any in-tree code anymore.
Comment hidden (mozreview-request)
Assignee

Updated

Last year
Assignee: nobody → kmaglione+bmo

Comment 2

Last year
mozreview-review
Comment on attachment 8987325 [details]
Bug 1470716: Remove unused ExtensibleStringBundle support.

https://reviewboard.mozilla.org/r/252582/#review259438

Saying our goodbyes to 1999 code feels good. lgtm! :)
Attachment #8987325 - Flags: review?(gandalf) → review+
Assignee

Comment 4

Last year
Jorg, there's one no-op use of this in comm-central that you'll probably want to just remove.
Flags: needinfo?(jorgk)

Comment 5

Last year
Hmm, you're referring to:
https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mailnews/intl/nsCharsetConverterManager.cpp#54
return sbServ->CreateExtensibleBundle(aCategory, aResult);

Why is that a no-op? That's not so obvious to me, what am I missing?
Flags: needinfo?(jorgk) → needinfo?(kmaglione+bmo)
Assignee

Comment 6

Last year
(In reply to Jorg K (GMT+2) from comment #5)
> Hmm, you're referring to:
> https://dxr.mozilla.org/comm-central/rev/
> 51678c5b0b3edd827c57a70f1c5baf02d44acead/mailnews/intl/
> nsCharsetConverterManager.cpp#54
> return sbServ->CreateExtensibleBundle(aCategory, aResult);
> 
> Why is that a no-op? That's not so obvious to me, what am I missing?

Extensible string bundles only work if there are category entries registered to add string bundles to them. comm-central doesn't have any.

It's possible that some extensions may provide entries for that extensible bundle. If that's the case and you want to continue supporting it, you could probably move the old extensible string bundle code to a separate service, or reimplement it in JS. I don't really have an easy way of finding whether any add-ons do use it, but I couldn't find any by googling the category name, and it seems rather unlikely.
Flags: needinfo?(kmaglione+bmo)

Comment 7

Last year
Posted patch 1470716-c-c-part.patch (obsolete) — Splinter Review
Hmm, I'm not really sure what the replacement should be, maybe just CreateBundle(). This compiles and the charset menu appears to work. Sorry about the ignorance, I can't really know everything.
Attachment #8989023 - Flags: review?(kmaglione+bmo)
Assignee

Comment 8

Last year
Comment on attachment 8989023 [details] [diff] [review]
1470716-c-c-part.patch

Review of attachment 8989023 [details] [diff] [review]:
-----------------------------------------------------------------

I believe you'll need to replace the *_CATEGORY constants with these URLs:

https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mailnews/build/nsMailModule.cpp#1321-1322

and then remove the category registrations/original category constants.
Attachment #8989023 - Flags: review?(kmaglione+bmo) → review-

Comment 9

Last year
Posted patch 1470716-c-c-part.patch (obsolete) — Splinter Review
How about this one then. I wasn't sure about that category stuff.
Attachment #8989023 - Attachment is obsolete: true
Attachment #8989025 - Flags: review?(kmaglione+bmo)
Assignee

Comment 10

Last year
Comment on attachment 8989025 [details] [diff] [review]
1470716-c-c-part.patch

Review of attachment 8989025 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

::: mailnews/intl/nsCharsetConverterManager.cpp
@@ +42,5 @@
>    NS_IF_RELEASE(sTitleBundle);
>  }
>  
>  static
> +nsresult LoadBundle(const char* aCategory,

Should rename this to something like aURL. Also, please fix indentation of the next line.
Attachment #8989025 - Flags: review?(kmaglione+bmo) → review+
Thanks, Kris!
Attachment #8989026 - Flags: review+

Updated

Last year
Attachment #8989025 - Attachment is obsolete: true

Updated

Last year
Keywords: leave-open

Comment 12

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/67e3d766d515
C-C part: Replace call to LoadExtensibleBundle(). r=kmag DONTBUILD

Updated

Last year
Keywords: leave-open

Comment 13

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/f6b1c959442d
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.