PeerConnectionImpl.h misuses NS_IMETHODIMP all over the place

NEW
Unassigned

Status

()

Core
WebRTC
P5
normal
Rank:
45
3 years ago
9 months ago

People

(Reporter: dholbert, Unassigned)

Tracking

Trunk
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox40 affected)

Details

(Reporter)

Description

3 years ago
PeerConnectionImpl.h has a whole bunch of NS_IMETHODIMP usages, which I assert are incorrect.

I'm guessing that most of these actually want to be NS_IMETHOD, though if they're not actually overriding a (virtual) XPCOM interface method, then they really probably want to just be 'nsresult'.

(Brief NS_IMETHODIMP overview, to the best of my knowledge: The NS_IMETHODIMP macro is simply a variant of NS_IMETHOD, and it's only intended to be used on out-of-line method definitions. It only differs from "NS_IMETHOD" in that it lacks "virtual", and the only reason for that difference is that C++ disallows you from re-specifying "virtual" on an out-of-line method definition.  So, you should never see NS_IMETHODIMP inside of a "class Foo { ... };" scope -- only on "Foo::Blah()" implementations, outside of the class definition.  Also, these macros should generally only be used on functions that are declared in XPCOM interfaces; not on just arbitrary virtual methods.)
(Reporter)

Comment 1

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #0)
> I'm guessing that most of these actually want to be NS_IMETHOD, though if
> they're not actually overriding a (virtual) XPCOM interface method, then
> they really probably want to just be 'nsresult'.

Looks like most of these are implementing functions defined in:
  http://mxr.mozilla.org/mozilla-central/source/dom/webidl/PeerConnectionImpl.webidl
so "NS_IMETHOD" is indeed appropriate.

One exception: "EnsureDataConnection" seems to just be a normal method (not an inherited or interface method), so that one should just be declared/defined as "nsresult EnsureDataConnection(...)", with no NS_* macros
(Reporter)

Comment 2

3 years ago
(er, actually, I'm not as familiar with webidl, so I'm not sure even NS_IMETHOD would be appropriate for PeerConnectionImpl.webidl methods, according to:
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#C.2B.2B_reflections_of_WebIDL_constructs
)
Yeah I was going to say. These aren't XPCOM interfaces, so just nsresult I think.
backlog: --- → webRTC+
Rank: 45
Priority: -- → P4
Mass change P4->P5 to align with new Mozilla triage process.
Priority: P4 → P5
You need to log in before you can comment on or make changes to this bug.