NS_IMPL_QUERY_INTERFACE_INHERITED0 is silly

RESOLVED FIXED in Firefox 42

Status

()

defect
P5
normal
RESOLVED FIXED
18 years ago
4 years ago

People

(Reporter: dbaron, Assigned: aidin)

Tracking

Trunk
Future
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

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.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → Future
QA Contact: scc → xpcom

Updated

6 years ago
Assignee: dbaron → nobody
Priority: P2 → P5

Comment 1

5 years ago
Remove them at http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#1124,1256
Status: ASSIGNED → NEW
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.
Summary: NS_IMPL_QUERYINTERFACE_INHERITED0 is silly → NS_IMPL_QUERY_INTERFACE_INHERITED0 is silly
Assignee

Comment 5

4 years ago
Can I take it? Seems a good starting point (:
Go ahead!
Assignee

Comment 7

4 years ago
Posted 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]
108603.patch

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, http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/file/nsFileChannel.cpp#200
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 http://mxr.mozilla.org/mozilla-central/search?string=NS_IMPL_ISUPPORTS_INHERITED0 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)
Assignee

Comment 11

4 years ago
Posted 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]
108603-2.patch

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]
108603-2.patch

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+
Assignee

Comment 14

4 years ago
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]
108603-2.patch

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

Thanks!

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: Remove NS_IMPL_QUERY_INTERFACE_INHERITED0
Don't write: NS_IMPL_QUERY_INTERFACE_INHERITED0 removed

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+
Assignee

Comment 16

4 years ago
Thanks for the explanation (:
(In reply to Aidin Gharibnavaz from comment #16)
> Thanks for the explanation (:

here is the try server PR: 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6777dcbd8710

once all the builds are successful here, you can go ahead and add checkin-needed in the *Keywords*
Assignee

Comment 18

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

Comment 20

4 years ago
Thanks Josh (-:
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f9c7975723da
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.