280949 - XPCOM utility NS_IS_SUPPORTED(object, interface)

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Aaron Leventhal]

It's very awkward to check to see if an object supports an interface.
I see a lot of places in our code that do this:
nsCOMPtr<nsIFoo> foo;
foo = do_QueryInterface(bar);
if (foo) {
  blah;
}

Wouldn't it be nice to just say something like:
if (NS_IS_SUPPORTED(bar, nsIFoo)) {
}

This is about code clarity, not optimization.

Code could be cut into a third the size, e.g.:
  PRBool isHTML;  // If it is HTML we will check extra DHTML accesibility logic
  nsCOMPtr<nsIContent> content(do_QueryInterface(aNode));
  if (content) {
    isHTML = content->IsContentOfType(nsIContent::eHTML);
  }
  else {
    nsCOMPtr<nsIHTMLDocument> htmlDoc(do_QueryInterface(aNode));
    isHTML = (htmlDoc != nsnull);
  }
  if (isHTML) {
    FireDHTMLFocusRelatedEvents(aAccessible, role);
  }

could be shorted to:
if (content? content->IsContentOfType(nsIContent::eHTML) : 
             NS_IS_SUPPORTED(content, nsIHTMLDocument)) {
  FireDHTMLFocusRelatedEvents(aAccessible, role);
}

Comment 1

[Aaron Leventhal]

Sorry, I mean
nsCOMPtr<nsIContent> content(do_QueryInterface(aNode));
if (content? content->IsContentOfType(nsIContent::eHTML) : 
             NS_IS_SUPPORTED(aNode, nsIHTMLDocument)) {
  FireDHTMLFocusRelatedEvents(aAccessible, role);
}

Comment 2

[Doug Turner (:dougt)]

most of the time, you will want to use the resulting interface pointer.

if you want to add this as a #define or something, write up a patch and I will
review it.

Comment 3

[Benjamin Smedberg]

eww, I don't like this even as a #define. It hides what's going on much more
than necessary, and would make debugging a real pain.

Comment 4

[Doug Turner (:dougt)]

a) the point of what we do is hide the details from the developer as much as we
can.  There are plenty of examples of this.  

b) how does something as simple as this make debugging difficult.  I don't buy it.

Comment 5

[Benjamin Smedberg]

The point of the COM macros/templates/etc is not to hide the details, but to
make programmer error visible at compile-time instead of runtime (i.e. making QI
type-safe, and reducing the likelihood of unreleased pointers through nsCOMPtr).
This macro does not prevent programmer error, but it does hide the fact that we
are doing a QI-release.

A macro like this makes debugging difficult because you cannot step through the
QI one line at a time.

Comment 6

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

I also don't like this.  NS_WITH_SERVICE was a disaster, and this seems similar
(although maybe worse, since it gives the impression of being more efficient
while not being more efficient).

Comment 7

[Doug Turner (:dougt)]

okay, aaron:  2 against 1.  

we can debate this till we are all blue in the face.  But I think there are
bigger problems to have us worried about ;-)

Comment 8

[Aaron Leventhal]

Alrighty. Just trying to make our code less ugly. 

For example, it shouldn't be so hard to figure out if a DOM node is HTML or not.
Sure you could probably use a QI of some kind for all cases, but most of the
time you want to use nsIContent::IsContentOfType because it's faster.  It would
be a lot easier if the document node supported nsIContent, but Hyatt once told
me that makes no sense brecause it's not content.

If it was up to me, I could use nsIContent for everything I can do with a dom
node so that I never needed all this QI'ing back and forth. It would reduce a
lot of code in the accessibility module.