Closed Bug 734008 Opened 9 years ago Closed 3 years ago

deCOM nsILanguageAtomService

Categories

(Core :: Internationalization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: m_kato, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

This interface isn't scriptable and only on Gecko (not used into comm-central).
Makoto, could you describe specifically what should be done to deCOM nsILanguageAtomService? 

(The only thing I can think of is to make the interface functions return nsresult, but that seems to me only a marginal improvement, if any.)
The interface functions all return an nsresult outparam, mostly forwarded from nsresult return values from other function calls. I went through the implementations and discovered they all return either NS_OK or NS_ERROR_FAILURE. So just checking the returned pointer for null would do the trick.
Assignee: nobody → bastiaan
I took a cue from DMO on deCOM and attempted to remove the interface in favour  of using the concrete class directly. I used NS_DEFINE_STATIC_CID_ACCESSOR for completeness sake, but I don't understand its purpose.
Attachment #625154 - Attachment is obsolete: true
Attachment #625511 - Flags: feedback?(m_kato)
Comment on attachment 625511 [details] [diff] [review]
rm nsILanguageAtomService, outparamdel, devirtualise and minor clean-up

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

You don't understand "decom"  decom means that object doesn't inheritance from nsISupport.  (ex. bug 635170 for sample)  And we need modify nsLocaleConstructors.h for decom.

Also, if possible, this object can be singleton class. or all member variant and method can be static.

::: intl/locale/public/nsLanguageAtomService.h
@@ +46,5 @@
> +#define NS_LANGUAGEATOMSERVICE_CID \
> +{ 0x81abb273, 0xa015, 0x4d79, \
> +  { 0x83, 0xb9, 0x38, 0xdb, 0x82, 0x02, 0x44, 0x15 } }
> +
> +class nsLanguageAtomService : public nsISupports

don't inheritance from nsISupport.

@@ +50,5 @@
> +class nsLanguageAtomService : public nsISupports
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +

Remove this due to decom

@@ +52,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +
> +  NS_DEFINE_STATIC_CID_ACCESSOR(NS_LANGUAGEATOMSERVICE_CID)
> +

Remove this due to decom

::: intl/locale/src/nsLanguageAtomService.cpp
@@ +49,1 @@
>  

remove this

@@ +130,2 @@
>  {
> +  nsIAtom *rv = mLangToGroup.GetWeak(aLanguage);

don't use rv for nsIAtom.  "rv" uses as nsresult.  (See https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide).
Attachment #625511 - Flags: feedback?(m_kato) → feedback-
This patch employs reference counting for nsLanguageAtomService to avoid dangling pointer issues during XPCOM shutdown. If it is possible to do without reference counting, I'm all ears. :)
Attachment #625511 - Attachment is obsolete: true
Attachment #627343 - Flags: review?(smontagu)
Assignee: bastiaan → nobody
smontagu: ping?
Review ping?
Attachment #627343 - Flags: review?(smontagu) → review?(VYV03354)
Comment on attachment 627343 [details] [diff] [review]
deCOMtaminate nsILangAtomService

This patch no longer applies to the latest trunk. Please unbitrot.
Attachment #627343 - Flags: review?(VYV03354)
Assignee: nobody → dirkjan
Attached file MozReview Request: bz://734008/djc (obsolete) —
/r/6615 - Bug 734008 - DeCOMtaminate nsILanguageAtomService

Pull down this commit:

hg pull -r b377be4bb271e8816b6d95abe23b5105f122f5d5 https://reviewboard-hg.mozilla.org/gecko/
Who wants to review?
Comment on attachment 8588119 [details]
MozReview Request: bz://734008/djc

I think that :smontagu is owner.
Attachment #8588119 - Flags: review?(smontagu)
I am on vacation all this week so will not get to this soon.
Attachment #8588119 - Attachment is obsolete: true
Attachment #8588119 - Flags: review?(smontagu)
Attachment #8617985 - Flags: review?(smontagu)
I happened to run across nsILanguageAtomService recently, and was going to do some cleanup; then found this bug already on file. Sorry it has languished without review for so long. Of course, in the meantime the patch has bit-rotted and no longer applies to trunk code. So I have rebased/updated the patch, and simplified a bit further (I don't think we need to refcount the nsLanguageAtomService at all, it's just a singleton that will live until shutdown). Let's see if we can actually get it landed this time...
Attachment #8870056 - Flags: review?(m_kato)
Assignee: dirkjan → jfkthame
Status: NEW → ASSIGNED
Duplicate of this bug: 1366886
Attachment #8870056 - Flags: review?(m_kato) → review+
Comment on attachment 8617985 [details]
MozReview Request: Bug 734008 - DeCOMtaminate nsILanguageAtomService

cancel due to this is old.
Attachment #8617985 - Flags: review?(smontagu)
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaaf2913c680334b0426a93b7c27b0280f2a7f67
Bug 734008 - DeCOMtaminate nsILanguageAtomService, make it a non-refcounted singleton and clean up various call sites. r=m_kato
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaaf2913c680
DeCOMtaminate nsILanguageAtomService, make it a non-refcounted singleton and clean up various call sites. r=m_kato
https://hg.mozilla.org/mozilla-central/rev/eaaf2913c680
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.