Remove ExtensibleStringBundle support

RESOLVED FIXED in Firefox 63

Status

()

RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks: 1 bug)

unspecified
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Updated

7 months ago
Assignee: nobody → kmaglione+bmo

Comment 2

7 months ago
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

7 months ago
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

7 months ago
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

7 months ago
(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

7 months ago
Created attachment 8989023 [details] [diff] [review]
1470716-c-c-part.patch

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

7 months ago
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

7 months ago
Created attachment 8989025 [details] [diff] [review]
1470716-c-c-part.patch

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

7 months ago
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+

Comment 11

7 months ago
Created attachment 8989026 [details] [diff] [review]
1470716-c-c-part.patch

Thanks, Kris!
Attachment #8989026 - Flags: review+

Updated

7 months ago
Attachment #8989025 - Attachment is obsolete: true

Updated

7 months ago
Keywords: leave-open

Comment 12

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

Updated

7 months ago
Keywords: leave-open

Comment 13

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