Simon Montagu wrote:
> I've appointed myself as clean-up and documentation policeman for the
> Bidi code. I'm working on the code and I will be posting some
> documentation here.
>
> Steve, if you, Chris Waterson and Kevin McCluskey could send more
> detailed comments directly to me at [EMAIL PROTECTED] it would be
> very helpful.
>
> Best regards,
> Simon
From Simon Fraser:
========================================================
-------- Original Message --------
Subject: Re: [Fwd: Re: Bug 62777 (BiDi feature added by IBM) needs
extensive review]
Date: Thu, 04 Jan 2001 11:44:41 -0800
From: [EMAIL PROTECTED] (Simon Fraser)
Organization: Netscape Communications Corp.
To: Steve Clark <[EMAIL PROTECTED]>
CC: [EMAIL PROTECTED]
References: <[EMAIL PROTECTED]>
Steve Clark wrote:
>
> I had already included Simon on the reviewers list for BiDi support,
> but I also need someone to review the selection and caret portions of
> the layout changes, along with related classes (like
> nsFrameIterator). Simon, I'm guessing you're the man for the caret
> code. How about selection? Is that Mike? Anyone else.
>
> Since this request is late, please don't feel pressure to do this by
> the 1:00 mtg today.
Comments here (since I'm too lazy to go to the meeting):
nsICaret.h:
They added non-virual methods and member variables to an XPCOM
interface -- a complete no-no.
In addition, would prefer use of PRPackedBool rather than PRBool
for member variables (esp. when > 1).
nsCaret.h/cpp
Is frame->GetStyleData() guaranteed to return a non-null style
info pointer? Thye never check it.
nsHTMLEditRules.cpp
Would prefer to see
if (NS_SUCCEEDED(res))
instead of
if NS_SUCCEEDED(res)
Also, need to be more careful about methods that can return NS_OK
but a null frame/content pointer. So after the call to
primaryFrame->GetChildFrameContainingOffset
should check frameBefore before dereferencing it. Same with frameAfter
lower down.
Should use a nsCOMPtr here and for baseLevel, because of an early return:
nsIAtom* embeddingLevel = NS_NewAtom("EmbeddingLevel");
nsTextEditRules.cpp
Similar comments apply as to nsHTMLEditRules.cpp.
Otherwise, things look fine.
Simon
