I've got some comments/suggestions/notes. If I'm wrong on something please point it
out.
General:
1. NS_INTERFACE_MAP_* usage- There are other ways to prefill your nsISupports
implementation in XPCOM. The most frequently used is the following macro:
NS_IMPL_ISUPPORTSx(className, interfaceA, interfaceB, etc);
x = the number of interfaces (not including the default nsISupports) the said class
implements.
2. Unless you *know* that an object impls a particular interface, it's best to always
call your QI methods (whether they're macros or do_*) w/ the nsresult param.
nsresult rv = NS_OK;
nsCOMPtr<nsIFoo> foo = do_QueryInterface(someObject, rv);
if (NS_FAILED(rv) error out;
Note that I checked the return value *not* the nullness of the pointer it returned. I
think it's a matter of style though. Anyone else have opinions?
Specific:
1. CBrowserImpl. In BrowserView.h you have a raw pointer to CBrowserImpl which is an
XPCOM object (meaning that it implements (at least) nsISupports). Raw pointers to
XPCOM objects are generally bad for your health, because you have to deal w/ all the
ref count balancing by hand which is error prone. When you need to create an XPCOM
object that isn't part of the component manager (and subsequently not registered), you
can do that by using the NS_NEWXPCOM() macro. So, instead of the following code
fragment in BrowserView.cpp:160 -
mpBrowserImpl = new CBrowserImpl(this);
if(mpBrowserImpl == nsnull)
return NS_ERROR_OUT_OF_MEMORY;
mpBrowserImpl->AddRef();
you should have:
CBrowserImpl* browserImpl;
NS_XPCOMNEW(browserImpl, CBrowserImpl);
/* NS_XPCOMNEW() allows tracing of object creation */
if (browserImpl == nsnull) return NS_ERROR_OUT_OF_MEMORY;
nsresult rv = NS_OK;
mChrome = do_QueryInterface(browserImpl, rv);
if (NS_FAILED(rv)) return rv;
/* as you can see above, we transfer ownership of the new XPCOM object (CBrowserImpl)
to a new member varialbe mChrome (nsCOMPtr<nsIWebBrowserChrome> mChrome). This gets
rid of your static cast need on line 166, *and* more importantly, you don't have to
monitor ownership anymore. you ditch the manual addref, and you can ditch the release
in your destructor. */
It probably sounds like I'm beating a dead horse w/ the ownership stuff, but it's a
major problem and it's best to let nsCOMPtrs do all the heavy lifting for you.
2. You're passing a raw pointer to a constructor, CBrowserImpl(this), which raises a
few points in the XPCOM world.
a. XPCOM object constructors should *never* take arguments. Arguments should always be
passed into an ::Init() method. This allows macroization of XPCOM object creation and
abstracts the "init" step which permits generic construction mechanisms and ultimately
allows us to package and build objects into different modules, *without* modifying
source code :-).
b. Constructors for XPCOM objects should basically do nothing (except setup the ref
count) and all work should be done in an Init() method.
3. For the interface methods you don't implement, you should return
"NS_ERROR_NOT_IMPLEMENTED" rather than "NS_ERROR_FAILURE". _NOT_IMPLEMENTED is part of
the error range so error checking macros (NS_FAILED, NS_SUCCEEDED) will capture it
properly.
Chak Nanga wrote:
> * I've taken some shortcuts such as to access some member vars
> directly etc. in places mainly to reduce additional getters/new
> interfaces to keep the code simple - which the OO purist in you
> may frown upon.
The only rules that are stricktly enforced are those of XPCOM. If you want to play w/
member variables *internal* to your implementation, that's your business :-). It's
very important that your *external* boundary conform to XPCOM rules.
It's sooooo nice to see meaningful comments in code :-); thanks!
Jud
