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 Akkana:
============================================
Most of my comments relate to general coding style; I'm not familiar
enough with bidi text that I can say whether the logic of the bidi
code itself is correct or not.
Most of the changes to nsPresShell.cpp are whitespace formatting
changes, which in nearly every case take lines which previously lined
up with the lines around them, and make them not line up. What's up
with that? I don't own nsPresShell.cpp, so it's someone else's decision,
but if it were a file I owned I'd veto all those whitespace changes.
The code I was reviewing in DoCopy was hard to read because of
incorrect indentation (I ended up reformatting my local copy).
I'd at least like to see that part formatted, and the other
reformatting changes inside DoCopy removed.
Why do we call
NS_WITH_SERVICE(nsIUBidiUtils, BidiEngine, kUBidiUtilCID, &rv);
nsBidiOptions mBidioptions;
mPresContext->GetBidi(&mBidioptions);
and
mPresContext->IsVisualMode(mIsvisual);
and
aBidiSystem=mPresContext->aIsBidiSystem;
when we're not going to use any of these unless
mPresContext->IsArabicEncoding returns true? Seems like those
should be inside the if (ArabicCharset==PR_TRUE) clause (BTW,
I'd slightly prefer just "if (ArabicCharset)" and omit the
"==PR_TRUE", but I won't be pedantic about that).
else if (mIsvisual) {
if ( (mBidioptions.mclipboardtextmode == IBMBIDI_CLIPBOARDTEXTMODE_VISUAL)||
(mBidioptions.mclipboardtextmode == IBMBIDI_CLIPBOARDTEXTMODE_SOURCE) ){
#ifdef _WIN32
if ( (BidiEngine->IsWin95())||(BidiEngine->IsWin98()) ){
nsString NewBuffer;
BidiEngine->Conv_FE_06 (buffer, NewBuffer);
buffer = NewBuffer;
}
#endif//_win32
}
What happens if these conditions are true but we're on a platform
other than win32? Does this mean we're only supporting bidi copy on
Windows and not on other platforms? Shouldn't platform-specific
code in any case live in platform libraries (like widget or gfx)?
If there's a good answer for that question, then why not ifdef out
the whole clause, rather than just the inner one? Especially
since the redundant next test
else if (mIsvisual) {
}
else if (!mIsvisual) {
ensures that we won't go into the next clause if !visual.
Shouldn't "nsString NewBuffer" be an nsAutoString (here and in other
places where NewBuffer is used)? "buffer" is an nsAutoString, and
we eventually say
buffer = NewBuffer;
and then do a GetUnicode() on buffer and pass the result into the
clipboard, which presumably copies the result.
His local variables are named things like mIsvisual, aBidiSystem,
ArabicCharset and NewBuffer, none of which follow our naming conventions.
else if (!mIsvisual) {
if ( (mBidioptions.mclipboardtextmode ==
IBMBIDI_CLIPBOARDTEXTMODE_VISUAL) || (!aBidiSystem) ){
nsString NewBuffer;
BidiEngine->Conv_06_FE_WithReverse(buffer, NewBuffer);
buffer = NewBuffer;
}
}
Doesn't this assume that it's always going in the reverse direction,
without checking? I don't really know how bidi works, but it
doesn't seem consistent with what he does earlier in visual mode.
>
