[webkit-reviews] review requested: [Bug 14626] Make bidiReorderCharacters independent of RenderBlock : [Attachment 15578] 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 15578: Take bidiReorderCharacters out of RenderBlock
http://bugs.webkit.org/attachment.cgi?id=15578&action=edit

------- Additional Comments from mitz at webkit.org
(In reply to comment #12)
> +static bool operator==(const BidiContext& c1, const BidiContext& c2)
> 
> This should have external linkage. If this function isn't going to be
inlined,
> then it should be in a .cpp file instead of a .h file. Or if it should be
> inlined, mark it inline and leave it in the header. But remove "static" no
> matter which way you go.

Moved it back to BidiContext.cpp. (Inlining it had a negative impact on my
performance numbers, perhaps because doing so negates the inlining of the
BidiStatus == operator. There may be room for further optimization here in the
future).

I reverted the change to make BidiContext inherit from Shared<BidiContext> as
it resulted in a significant performance regression. Again, there may be a
well-performing way to do it in the future, but for now I would rather just
leave existing code as it was.

> +    unsigned offset() { return m_offset; }
> 
> Mark this const?

Done.

> +#ifndef BidiResolver_h
> +#define BidiResolver_h
> +
> +#include "BidiContext.h"
> +#include <wtf/Assertions.h>
> +#include <wtf/PassRefPtr.h>
> +#include <wtf/RefPtr.h>
> +#include <wtf/unicode/Unicode.h>
> 
> There are some rendundant includes here. No need to include something that's
> already included by BidiContext.h.

Removed 2 of the 5 #includes.

> +class BidiIterator;
> +class BidiRun;
> +template <class Iterator, class Run> class BidiResolver;
> +typedef BidiResolver<BidiIterator, BidiRun> BidiState;
>  class Position;
> +class RootInlineBox;
> 
> I'd suggest putting the template declaration and the typedef in a separate
> section, after the plain old class declarations. I've done the same for
struct
> declarations in the past.

Done.

> 
> -    void bidiReorderLine(const BidiIterator& start, const BidiIterator& end,

> BidiState& bidi);
> +    void bidiReorderLine(const BidiIterator& start, const BidiIterator& end,

> BidiState&);
> 
> You removed the name "bidi" from this, but not from the next declaration.

Fixed.

> +struct BidiRun : public BidiCharacterRun {
> 
> For struct, inheritance is public by default. So I suggest omitting "public"
> when a struct inherits.

Done.

> I suppose we should be renaming "bidi.h" to "BidiRun.h" in a future pass
given
> that's the only declaration in there now. Not sure what to do about
"bidi.cpp".

I think the RenderBlock methods currently in bidi.cpp should end up in
RenderBlock.cpp. Then bidi.cpp and bidi.h will be about what's now known as
BidiState (which should be renamed; I like the name RenderBidiResolver but I
think names beginning with "Render" are reserved for classes that inherit from
RenderObject, so maybe BlockBidiResolver).

> I'm not sure this is a convenient time for this huge improvement, but the
> possibility of fixing some bidi bugs on Windows seems one great reason to do
> it.

I should mention that there are two other ways to address (if not completely
resolve) the bidi bugs on Windows that are possibly less risky:

1) Since ICU is available on Windows, use its implementation of the bidi
algorithm. Alternatively, use some other implementation available on the
platform or bring in some other open-source implementation.

2) Do something similar to what -[NSString _web_drawAtPoint:font:textColor:]
does, namely implement canUseFastRenderer() and as the "slow renderer" still
use WebCore, but temporarily force it onto the complex string drawing path.
While you are not really supposed to use the WebCore renderer for
mixed-directionality text, in all but some contrived cases (which possibly
cannot ever occur in the intended use), the "complex" code path will do the
right thing.