Jud,
Thanks for your feedback. I'll make the necessary changes...
Chak
Judson Valeski wrote:
> 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
>
