8929 - Can we get rid of NS_COMFALSE?

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Brendan Eich [:brendan]]

As waterson and warren demonstrated by fisticuffs, NS_COMFALSE overloading of
the return value makes a hazard:

  nsresult rv = ifp->someMethodReturningNS_COMFALSE();
  if (rv == NS_COMFALSE)
    ok, false was returned;
  else
    true? could be error!

Why not axe NS_COMFALSE now and never have this hazard darken our door?

/be

Comment 1

[brendan-obsolete]

Ok, I'm making this the meta-bug (bug about bugs?  hmm, it's sort of that, but
it is also about NS_COMFALSE -- maybe uber- rather than meta-).

/be

Comment 2

[Suresh Duddi (gone)]

Hey I wont have time to track/do this one. Assigning back to brendan. Anyway
this is the tracking bug.

Comment 3

[Brad Roberts]

I have the time and desire, so I'm both tracking and implementing this one, in
stages.

Comment 4

[John Bandhauer]

Is NS_COMFALSE ever going to really go away?

I just got burned because fur changed nsHashtableEnumerator::IsDone to return
NS_COMFALSE to 'be consistent with other enumerators' without fixing the code
that uses it. This is a step backwards.

I see that we now have NS_ENUMERATOR_FALSE (which equals 1 like NS_COMFALSE) all
over the place.
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsIEnumerator.idl#26
http://lxr.mozilla.org/seamonkey/ident?i=NS_ENUMERATOR_FALSE

Comment 5

[Brad Roberts]

NS_ENUMERATOR_FALSE is one step along the way of eliminating NS_COMFALSE from
the rest of the code.  The bad usage among nsIEnumerators is the largest
consumer and one of the more complicated to get rid of.  With that one out of
the way for now, the rest of the usages are easier to remove from the code and
that's what I've been doing, if incredibly slowly.  The right answer for
enumerators is to NOT use NS_ENUMERATOR_FALSE or NS_COMFALSE, its to have a
function like ::isDone(bool myResult) -- or something along those lines, NOT
overloading the nsresult with both error codes and the true/false result its
trying to convey.  Make sense/

Comment 6

[John Bandhauer]

I'm glad you're dealing with this. I don't really see how transitioning to
NS_ENUMERATOR_FALSE really helps much. Whacking a tree to switch all
declarations of these methods to use the boolean out param is the only way you
are ever going to uncover and fix all the insidious little uses of this pattern.

Comment 7

[moshev]

How come this is still M14? M14 is already out!

Comment 8

[David Krause]

Mass moving old milestone bugs to M16

Comment 9

[John Dobbins]

This is pretty old so I'm doing some Cleaning up

Up to date lists submitted to open bugs
bug 11592 , bug 11593 , bug 11595 .

bug 11597 can be resolved fixed.

In addition to open bugs NS_COMFALSE was found in
/js/src/xpconnect/xpccomponents.cpp
/mailnews/db/msgdb/public/public/nsIDBChangeAnnouncer.idl
/mailnews/db/msgdb/src/msMsgDatabase.cpp
so bug 11591 and 11598 need to be reopened as regressions.

Comment 10

[Mike]

M16 has been out for a while now, these bugs target milestones need to be 
updated.

Comment 11

[Brad Roberts]

moving this out a few.  I don't have the time for this, if someone else does,
grab it, run with it, maybe even finish it.

Comment 12

[Dawn Endico]

dp is no longer @netscape.com. changing qa contact to default for this product

Comment 13

[Oliver Klee]

Milestone 0.8 has been released. We should either resolve this bug or update its
milestone.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

*** Bug 11592 has been marked as a duplicate of this bug. ***

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

*** Bug 11593 has been marked as a duplicate of this bug. ***

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

*** Bug 11595 has been marked as a duplicate of this bug. ***

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

*** Bug 11597 has been marked as a duplicate of this bug. ***

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

*** Bug 169088 has been marked as a duplicate of this bug. ***

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I have a patch for this in bug 169088 that i'll sync to tip in a little while

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch v1

Fixes comments from peterv and dbaron. The patch still does what is described
in bug 169088 comment 6. The only difference is that i've split up
nsContentErrors.h in to nsContentErrors.h and nsLayoutErrors.h and placed those
files in {content|layout}/base/public. I've also given a proper name to the
error-constant.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 107233 [details] [diff] [review]
patch v1

Peterv: This patch hasn't changed codewise since you reviewed it in bug 169088.
The only real change is that nsContentErrors.h has been split up in two files

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 107233 [details] [diff] [review]
patch v1

> -  return NS_COMFALSE;
> +  return NS_OK;

This is in nsSelection.cpp.  Does no one check the return value or something?

nsGenericHTMLElement.cpp:

>   if (nsnull != mAttributes) {

fix this while you're here?  ;)

I assume the comments on the Handle*Event functions were just out of touch with
reality?

sr=bzbarsky pending explanation on the nsSelection issue.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > -  return NS_COMFALSE;
> > +  return NS_OK;

> This is in nsSelection.cpp.  Does no one check the return value or something?

No C++ caller of this function checks the returnvalue. The function is internal
to the class so there are no JS-callers (and if there were they wouldn't see a
difference since both are success-values)

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

yes, all the changed/removed comments were lying

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

checked in! thanks for reviews