734008 - deCOM nsILanguageAtomService

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Makoto Kato [:m_kato]]

This interface isn't scriptable and only on Gecko (not used into comm-central).

Comment 1

[Bastiaan Jacques]

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.)

Comment 2

[Bastiaan Jacques]

Attachment: part 1: outparamdel nsresult (wip)

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.

Comment 3

[Bastiaan Jacques]

Attachment: rm nsILanguageAtomService, outparamdel, devirtualise and minor clean-up

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.

Comment 4

[Makoto Kato [: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).

Comment 5

[Bastiaan Jacques]

Attachment: deCOMtaminate nsILangAtomService

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. :)

Comment 6

[Bastiaan Jacques]

smontagu: ping?

Comment 7

[Guilherme Lima]

Review ping?

Comment 8

[Masatoshi Kimura [:emk]]

Comment on attachment 627343 [details] [diff] [review]
deCOMtaminate nsILangAtomService

This patch no longer applies to the latest trunk. Please unbitrot.

Comment 9

[Dirkjan Ochtman (:djc)]

Attachment: MozReview Request: bz://734008/djc

/r/6615 - Bug 734008 - DeCOMtaminate nsILanguageAtomService

Pull down this commit:

hg pull -r b377be4bb271e8816b6d95abe23b5105f122f5d5 https://reviewboard-hg.mozilla.org/gecko/

Comment 10

[Dirkjan Ochtman (:djc)]

Who wants to review?

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8588119 [details]
MozReview Request: bz://734008/djc

I think that :smontagu is owner.

Comment 12

[Simon Montagu :smontagu]

I am on vacation all this week so will not get to this soon.

Comment 14

[Dirkjan Ochtman (:djc)]

Attachment: MozReview Request: Bug 734008 - DeCOMtaminate nsILanguageAtomService

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: DeCOMtaminate nsILanguageAtomService, make it a non-refcounted singleton and clean up various call sites

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...

Comment 16

[Jonathan Kew [:jfkthame]]

Try run, to check if I broke anything.... https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc829f90cd16f33c3275fe6b6d332c41fbad2448.

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8617985 [details]
MozReview Request: Bug 734008 - DeCOMtaminate nsILanguageAtomService

cancel due to this is old.

Comment 19

[Jonathan Kew [:jfkthame]]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eaaf2913c680