Closed Bug 1470716 Opened 2 years ago Closed 2 years ago

Remove ExtensibleStringBundle support

Categories

(Core :: Internationalization, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

These aren't used by any in-tree code anymore.
Assignee: nobody → kmaglione+bmo
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+
Jorg, there's one no-op use of this in comm-central that you'll probably want to just remove.
Flags: needinfo?(jorgk)
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)
(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)
Attached 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)
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-
Attached 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)
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+
Attachment #8989025 - Attachment is obsolete: true
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/67e3d766d515
C-C part: Replace call to LoadExtensibleBundle(). r=kmag DONTBUILD
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/f6b1c959442d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.