11598 - eliminate NS_COMFALSE

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Warren Harris]

We should eliminate the NS_COMFALSE madness once and for all. Here's a list of
offending uses in your module -- please pass the bug along if there's someone
else who should deal with it.

      mailnews/addrbook/src/nsAddrDatabase.cpp:
            View change log or Blame annotations
            line 255
            line 2193
      mailnews/base/search/src/nsMsgLocalSearch.cpp:
            View change log or Blame annotations
            line 486
            line 520
            line 593
            line 600
      mailnews/base/search/src/nsMsgSearchTerm.cpp:
            View change log or Blame annotations
            line 579
            line 589
            line 648
            line 680
            line 741
            line 853
            line 898
            line 948
            line 988
            line 1008
            line 1034
      mailnews/base/src/nsMsgFolderCache.cpp:
            View change log or Blame annotations
            line 316
      mailnews/db/msgdb/src/nsMsgDatabase.cpp:
            View change log or Blame annotations
            line 105
            line 1529
            line 1654
            line 1835
            line 1895
      mailnews/db/msgdb/src/nsMsgThread.cpp:
            View change log or Blame annotations
            line 717

Comment 1

[scottputterman]

reassigning to bienvenu who owns most of these file and cc'ing chuang who owns
one of these files.

Comment 2

[David :Bienvenu]

woa, I need a little background. What should I replace it with? Did RDF stop
caring about NS_COMFALSE? I don't want to blindly replace it with NS_OK or
NS_FAILURE and break everything.

Comment 3

[Warren Harris]

We should replace it with NS_OK/NS_ERROR_FAILURE. You'll need to coordinate
with Waterson to change it.

This is one of Brendan's architecture cleanup items.

Comment 4

[David :Bienvenu]

Chris, any thoughts about this?

Comment 5

[John Bandhauer]

Without looking at specifics...

At least some of our uses of NS_COMFALSE are functions that really just want to
return PRBool. The 'right' way to fix them is to do:

(in xpidl)
  PRBool Foo();

which becomes in C++

NS_IMETHOD Foo(PRBool *_result);

So in calling it you do:

PRBool ok;
if(SUCCEEDED(bar->Foo(&ok)) && ok)
 // yada yada

This is, of course, more work than using NS_COMFALSE.

But with NS_COMFALSE we have the "NS_OK == PR_FALSE and NS_COMFALSE == PR_TRUE"
problem. And also the problem that both of those return values pass the
NS_SUCCEEDED(rv) test that we all love so much.

Comment 6

[David :Bienvenu]

That's fine for my internal routines which I can fix right now, but when dealing
with rdf enumerators, I don't have that kind of flexibility

Comment 7

[brendan-obsolete]

jband, if you want to avoid warren's fate (losing in fisticuffs to waterson),
you might rather write the exemplar this way:

  if (NS_FAILED(rv))
    deal with failure, probably by early return;
  if (ok)
    boolean result was true;
  else
    boolean result was false;

Conjoining NS_SUCCEEDED(rv) && ok may hide errors along with false-no-op cases.

bienvenu, when I asked waterson about how his hoped-for conversion from
nsIEnumerator to nsISimpleEnumerator might proceed, he came back with this
thought: add boolean HasMoreElements(); and nsISupports GetNext(); methods to
nsIEnumerator and its impl, incrementally convert from the First/Next/IsDone
methods to these, then rename or equate typenames so nsIEnumerator becomes
nsISimpleEnumerator.  Does this sound like a plan?

I'm cc'ing rjc in addition to waterson for RDF help.  I'll seek comments in
other fora from editor, widget, and other folks who currently use nsIEnumerator
and not nsISimpleEnumerator.

/be

Comment 8

[David :Bienvenu]

Sounds like a good plan. I'm not real familiar with the db code that uses
NS_COMFALSE, since Warren wrote it :-) but I'll work with Chris to clean it up.

Comment 9

[David :Bienvenu]

fixed in almost every file in mailnews.

Comment 10

[David :Bienvenu]

I noticed there are still uses of NS_COMFALSE in the address book backend, so
I'm reopening and reassigning

Comment 11

[leger]

Clearing Fixed resolution due to reopen.

Comment 12

[chuang]

Done on address book, will check in if tree open.

Comment 13

[chuang]

Done

Comment 14

[Chris Petersen]

Based on chuang's comment's, I mark this fixed in the Sept 2nd build.