deCOM nsILanguageAtomService

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: m_kato, Assigned: jfkthame)

Tracking

(Blocks 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

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

Comment 3

7 years ago
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)
(Reporter)

Comment 4

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

Comment 5

7 years ago
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)

Updated

7 years ago
Assignee: bastiaan → nobody

Comment 6

7 years ago
smontagu: ping?

Comment 7

6 years ago
Review ping?

Updated

6 years ago
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
Posted 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?
(Reporter)

Comment 11

4 years ago
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)
(Assignee)

Comment 15

2 years ago
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)

Updated

2 years ago
Assignee: dirkjan → jfkthame
Status: NEW → ASSIGNED
Duplicate of this bug: 1366886
(Reporter)

Updated

2 years ago
Attachment #8870056 - Flags: review?(m_kato) → review+
(Reporter)

Comment 18

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

cancel due to this is old.
Attachment #8617985 - Flags: review?(smontagu)
(Assignee)

Comment 19

2 years ago
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

2 years ago
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

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eaaf2913c680
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.