Closed Bug 108603 Opened 20 years ago Closed 6 years ago



(Core :: XPCOM, defect, P5)




Tracking Status
firefox42 --- fixed


(Reporter: dbaron, Assigned: aidin)




(Whiteboard: [good first bug][lang=C++])


(1 file, 2 obsolete files)

NS_IMPL_QUERYINTERFACE_INHERITED0 is silly -- there's no need to 
forward if you're not implementing new interfaces.  The only possible 
reason one might want it is if you're inheriting the same interfaces from 
two concrete classes, but I really don't see why one would want to do that.

Perhaps we should remove it and |#define| it to be something that doesn't 
compile, with a comment explaining why you can just inherit without using 
any macros at all if you're not adding any new interfaces.
Priority: -- → P2
Target Milestone: --- → Future
QA Contact: scc → xpcom
Assignee: dbaron → nobody
Priority: P2 → P5
Remove them at,1256
Whiteboard: [good first bug][lang=C++]
This isn't totally useless.  The inherited ADDREF and RELEASE give more precise information to nsTraceRefcnt logging.
Oops, I thought this was talking about a different macro!  This particular one doesn't seem to exist any more.
Oh, I see the macro has a slightly different name.
Can I take it? Seems a good starting point (:
Go ahead!
Attached patch 108603.patch (obsolete) — Splinter Review
I removed the Macro completely.

P.S: Can you assign the bug to me? Thanks.
Attachment #8632508 - Flags: review?(continuation)
Assignee: nobody → aidin
Comment on attachment 8632508 [details] [diff] [review]

Review of attachment 8632508 [details] [diff] [review]:

Sorry for the delay.

Does this compile locally for you?  For me, with clang, it is failing to compile in dom/asmjscache/AsmJSCache.cpp, for the class SingleProcessRunnable. I think the problem is that that class uses NS_DECL_ISUPPORTS_INHERITED, which declares a QueryInterface method, but because of your changes there is no QueryInterface defined.

Maybe you could define a new macro in nsISupportsImpl.h called something like NS_DECL_ISUPPORTS_ADDREF_RELEASE that just defines AddRef and Release? I think that would be useful in other places, too. The big drawback of this is it would require fixing every class that uses NS_IMPL_ISUPPORTS_INHERITED0 right now, which is a lot of them.
Attachment #8632508 - Flags: review?(continuation) → review-
Benjamin, what do you think? (I'd ask Nathan, but it looks like he's away at the moment.) It seems like this would require a lot of change for not a lot of benefit. I guess NS_IMPL_QUERY_INTERFACE_INHERITED0 could be inlined into NS_IMPL_ISUPPORTS_INHERITED0 which would be a little better.
Flags: needinfo?(benjamin)
Huh, I thought we had the separate addref/release declarations. I fully support that.

But I'm also a little surprised/concerned that we have so many NS_IMPL_ISUPPORTS_INHERITED0 users. Why do those classes need to implement isupports at all?

As an example there's nsFileUploadContentStream,
I can't see any reason for this class to have NS_DECL_ISUPPORTS_INHERITED or NS_IMPL_ISUPPORTS_INHERITED0. Unless perhaps it's so that refcount logging will log the correct type? That seems like a silly reason to me.

A small random sample of didn't show others which were necessary either. SimpleProcessRunnable seems to be in the same situation.

The only real thing to check is that the destructor of the base class is virtual. If it's not, that will definitely change behavior.
Flags: needinfo?(benjamin)
Attached patch 108603-2.patch (obsolete) — Splinter Review
Sorry for the compile error. And sorry for the delay.

I couldn't remove NS_IMPL_ISUPPORTS_INHERITED0 completely. There were some points that I couldn't decide what to do. For example, in the dom/asmjscache/AsmJSCache.cpp, class ParentProcessRunnable should implement QueryInterface along with AddRef and Release, because its parent declare them as abstract methods. So, I can't replace NS_IMPL_ISUPPORTS_INHERITED0 with new NS_DECL_ISUPPORTS_ADDREF_RELEASE.
Or, maybe I didn't understand it well.

Thus, I gone with the other solution: To inline NS_IMPL_QUERY_INTERFACE_INHERITED0 into NS_IMPL_ISUPPORTS_INHERITED0.
Attachment #8632508 - Attachment is obsolete: true
Attachment #8638484 - Flags: review?(continuation)
Comment on attachment 8638484 [details] [diff] [review]

Review of attachment 8638484 [details] [diff] [review]:

This looks good to me. I'll forward the review to nfroyd because he is an XPCOM peer.
Attachment #8638484 - Flags: review?(nfroyd)
Attachment #8638484 - Flags: review?(continuation)
Attachment #8638484 - Flags: feedback+
Comment on attachment 8638484 [details] [diff] [review]

Review of attachment 8638484 [details] [diff] [review]:

Thanks for the patch!  Please modify your commit message to say:

"Bug 108603 - Remove NS_IMPL_QUERY_INTERFACE_INHERITED0; r=mccr8"
Attachment #8638484 - Flags: review?(nfroyd) → review+
Attached patch 108603-2.patchSplinter Review
Thank you (:

I updated the message.
Is there any documented convension about commit messages?
Attachment #8638484 - Attachment is obsolete: true
Attachment #8638699 - Flags: review?(nfroyd)
Comment on attachment 8638699 [details] [diff] [review]

Review of attachment 8638699 [details] [diff] [review]:


If we did have documentation about commit message formatting, I can't find it currently. :(  The first line should typically be a short description of what the patch accomplishes, written as though you were giving a command to someone about what to do.  Hence:


Write: Avoid implicit unicode <-> str conversion
Don't write: Implicit unicode <-> str conversion avoided

Write: Disable browser_trackingProtection.js on Linux due to ...
Don't write: browser_trackingProtection.js on Linux disabled due to...

The rest of the commit message should be a longer description of what motivates the patch and perhaps a bit about how the patch accomplishes that.  For a short patch like this, you probably don't need to write a longer description.

Don't worry too much about the exact mechanics.  Keep writing code and the people who review your patches will help you out.
Attachment #8638699 - Flags: review?(nfroyd) → review+
Thanks for the explanation (:
(In reply to Aidin Gharibnavaz from comment #16)
> Thanks for the explanation (:

here is the try server PR:

once all the builds are successful here, you can go ahead and add checkin-needed in the *Keywords*
There's one fail in one of the jobs: One of the tests of "B2G Device Image opt" is failed. I couldn't understand it clearly, but it seems that it have nothing to do with my changes.

Can you check it too please?
Agreed. It looks like an intermittent problem on a build machine not caused by your changes.
Thanks Josh (-:
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.