108603 - NS_IMPL_QUERY_INTERFACE_INHERITED0 is silly

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P5)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[Benjamin Smedberg]

Remove them at http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#1124,1256

Comment 2

[Andrew McCreight [:mccr8]]

This isn't totally useless.  The inherited ADDREF and RELEASE give more precise information to nsTraceRefcnt logging.

Comment 3

[Andrew McCreight [:mccr8]]

Oops, I thought this was talking about a different macro!  This particular one doesn't seem to exist any more.

Comment 4

[Andrew McCreight [:mccr8]]

Oh, I see the macro has a slightly different name.

Comment 5

[Aidin Gharibnavaz]

Can I take it? Seems a good starting point (:

Comment 6

[Josh Matthews [:jdm]]

Go ahead!

Comment 7

[Aidin Gharibnavaz]

Attachment: 108603.patch

I removed the Macro completely.

P.S: Can you assign the bug to me? Thanks.

Comment 8

[Andrew McCreight [:mccr8]]

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.

Comment 9

[Andrew McCreight [:mccr8]]

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.

Comment 10

[Benjamin Smedberg]

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.

Comment 11

[Aidin Gharibnavaz]

Attachment: 108603-2.patch

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.

Comment 12

[Andrew McCreight [:mccr8]]

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.

Comment 13

[Nathan Froyd [:froydnj]]

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"

Comment 14

[Aidin Gharibnavaz]

Attachment: 108603-2.patch

Thank you (:

I updated the message.
Is there any documented convension about commit messages?

Comment 15

[Nathan Froyd [:froydnj]]

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.

Comment 16

[Aidin Gharibnavaz]

Thanks for the explanation (:

Comment 17

[Anup Kumar [:reznord]]

(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*

Comment 18

[Aidin Gharibnavaz]

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?

Comment 19

[Josh Matthews [:jdm]]

Agreed. It looks like an intermittent problem on a build machine not caused by your changes.

Comment 20

[Aidin Gharibnavaz]

Thanks Josh (-:

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9c7975723da

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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