deCOMtaminate nsINameSpaceManager

RESOLVED FIXED in Firefox 63

Status

()

defect
RESOLVED FIXED
11 years ago
3 months ago

People

(Reporter: taras.mozilla, Assigned: qdot)

Tracking

(Blocks 1 bug)

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

11 years ago
Posted patch cleanup (obsolete) — Splinter Review
This is one of the tasks listed on https://wiki.mozilla.org/Gecko:DeCOMtamination

I moved nsNameSpaceManagerImpl into nsINameSpaceManager, got rid of virtual methods. Not sure what else is one is to do. It's pretty trivial and none of these methods are outparamdel-able.

Should change the class to not inherit from nsISupports and s/nsCOMPtr/nsRefPtr/ for pointers containing it?
Reporter

Comment 1

11 years ago
Posted patch cleanup (obsolete) — Splinter Review
accidentally sent a stale ver
Attachment #338954 - Attachment is obsolete: true
Attachment #338961 - Flags: review?(jst)
Comment on attachment 338961 [details] [diff] [review]
cleanup

class nsINameSpaceManager : public nsISupports

This could totally be a followup bug, but there's no real need to inherit nsISupports here any longer I don't think. And this class should be nsNameSpaceManager now, no need for ns_I_ here any more IMO.

+  nsresult RegisterNameSpace(const nsAString& aURI,
+                                     PRInt32& aNameSpaceID);

Got some auto-emacs-indentation magic pixie dust that would help here too? If not, fix this manually before landing :)
 
+nsINameSpaceManager::RegisterNameSpace(const nsAString& aURI, 
                                         PRInt32& aNameSpaceID)

Here,

+nsresult nsINameSpaceManager::AddNameSpace(const nsAString& aURI,
                                             const PRInt32 aNameSpaceID)

and here too.

r+sr=jst with that.
Attachment #338961 - Flags: superreview+
Attachment #338961 - Flags: review?(jst)
Attachment #338961 - Flags: review+
Assignee: nobody → tglek
QA Contact: content
Hardware: PC → All
QA Contact: content
QA Contact: content
Reporter

Updated

11 years ago
Blocks: 458300
Reporter

Comment 3

11 years ago
pushed http://hg.mozilla.org/mozilla-central/rev/92e24ee719b8
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
This broke up the windows and linux leak test builds, so backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
To give you more of a hint than that, it fried everything building both accessibility and -enable-shared: SeaMonkey builds everything shared, so all their Linux and Windows builds, Thunderbird only builds unittests shared, so Windows there (and would have been Linux, except it was already busted). I wondered if it just needed a clobber, so depending on when it started, we may or may not have gotten a clobbered Tb Windows check build before the backout.
Nope, backout made it in before my clobber started.
Don't forget debug builds are also shared by default.
Yeah, so with this patch I get:

Undefined symbols:
  "nsINameSpaceManager::GetNameSpaceURI(int, nsAString_internal&)", referenced from:
      nsDocAccessible::GetNameSpaceURIForID(short, nsAString_internal&)  in nsDocAccessible.o

because that function became non-virtual..
Reporter

Comment 9

11 years ago
jst, is this still a candidate for decomtamination? Once I rename and remove nsISupports, are the downstream projects expected to adapt?

Updated

10 years ago
Component: Content → DOM
QA Contact: content → general
Blocks: deCOM
Is it still possible to deCOM this or should we just close the bug?
Assignee: tglek → Ms2ger
Posted patch WIP (obsolete) — Splinter Review
I don't remember why I stopped working on this bug; this seems to be where I got.
Assignee: Ms2ger → nobody
Assignee: nobody → kyle
Attachment #338961 - Attachment is obsolete: true
Attachment #606884 - Attachment is obsolete: true
Most of this was taken care of in bug 458300. However, nsNameSpaceManager still inherited from nsISupports, only for the refcounting bits. We should be able to use NS_INLINE_DECL_REFCOUNTING and finish this off.
Most of the deCOM work was handled in bug 458300, so we just need to
use the inline refcounting macro to remove nsISupports.

MozReview-Commit-ID: 7npo7RF68Vo
Comment on attachment 8994372 [details]
Bug 455595 - Finish deCOMtaminating nsNameSpaceManager

Andrea Marchesini [:baku] has approved the revision.

https://phabricator.services.mozilla.com/D2309
Attachment #8994372 - Flags: review+

Comment 16

11 months ago
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf462cc971f8
Finish deCOMtaminating nsNameSpaceManager r=baku

Comment 17

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cf462cc971f8
Status: REOPENED → RESOLVED
Closed: 11 years ago11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.