[webkit-reviews] review requested: [Bug 14626] Make bidiReorderCharacters independent of RenderBlock : [Attachment 15566] Take bidiReorderCharacters out of RenderBlock

[bugzilla-daemon at webkit.org]

mitz at webkit.org has asked  for review:
Bug 14626: Make bidiReorderCharacters independent of RenderBlock
http://bugs.webkit.org/show_bug.cgi?id=14626

Attachment 15566: Take bidiReorderCharacters out of RenderBlock
http://bugs.webkit.org/attachment.cgi?id=15566&action=edit

------- Additional Comments from mitz at webkit.org
(In reply to comment #9)
> Please don't do this in the header.
> +using namespace WTF::Unicode;

OK.

> Since this is ref-counted, can this inherit from Shared instead of doing its
> own?
> +class BidiContext {

Thanks, now it does.

> in BidiContext.cpp:
> 
> It seems arbitrary to put this in the .cpp file when so much implementation
is
> BidiResolver.h

Moved to BidiContext.h.

> in BidiResolver.h:
> 
> Why not put each of the classes in this file into it's own header?

Because they are small and those headers will not be included directly by any
file besides BidiResolver.h.

> Please don't do this in the header.
> +using namespace WTF::Unicode;

OK.

> Put a space after the if.
> +	       if(dir == LeftToRight || dir == ArabicNumber || dir ==
> EuropeanNumber)

Done for all "if(", but otherwise style cleanup of existing code is outside the
scope of this patch.

> Put spaces around the %'s in few places including
> +	   if (runDir == RightToLeft) {
> +	       if (level%2) // we have an odd level
> 

Ditto.

Renamed BidiRunBase to BidiCharacterRun. Note that in this patch I am not
renaming existing bidi.cpp types such as BidiIterator, BidiState and BidiRun,
although eventually I would like to (at which time BidiStatus could be renamed
BidiState or BidiResolverState).