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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied mitz at webkit.org's request 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 Darin Adler <darin at apple.com>
+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.

+    unsigned offset() { return m_offset; }

Mark this const?

+#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.

+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.

-    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.

+struct BidiRun : public BidiCharacterRun {

For struct, inheritance is public by default. So I suggest omitting "public"
when a struct inherits.

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'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. Setting review- because I know you well enough to know you'll want to take
another pass on this and write a new ChangeLog.

(For selfish Apple reasons it would be good to get this checked in by Friday
morning. After that time we may have to branch if there are any major changes
on the trunk.)