[webkit-reviews] review granted: [Bug 20953] Rework the Font handling in QtWebKit : [Attachment 23591] Split WidthIterator out of Font.cpp

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Sep 21 11:32:59 PDT 2008


Darin Adler <darin at apple.com> has granted Holger Freyther
<freyther at handhelds.org>'s request for review:
Bug 20953: Rework the Font handling in QtWebKit
https://bugs.webkit.org/show_bug.cgi?id=20953

Attachment 23591: Split WidthIterator out of Font.cpp
https://bugs.webkit.org/attachment.cgi?id=23591&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This code in WidthIterator was all Apple-written in 2003, and didn't come from
KHTML, so you don't need to include the pre-2003 copyrights on the new file.

+/**
+ * This file is part of the html renderer for KDE.

You shouldn't copy those lines either.

+	 private:
+	 UChar32 normalizeVoicingMarks(int currentCharacter);

Indentation here is wrong. the private should be outdented.

+#include "Font.h"

I don't think we really need to include the entire Font.h header. This file
just declares things, it doesn't actually need class definitions for classes
like Font and TextRun. All we need is a forward delcaration of Font,
GlyphBuffer, and TextRun, and then an appropriate include to get the UChar32
type.

WidthIterator should inherit from Noncopyable.

+	 WidthIterator(const Font* font, const TextRun& run);
+
+	 void advance(int to, GlyphBuffer* glyphBuffer = 0);
+	 bool advanceOneCharacter(float& width, GlyphBuffer* glyphBuffer = 0);

Style says we shouldn't have named arguments for font, run, and glyphBuffer.

r=me, but some of those changes should be done, i think.


More information about the webkit-reviews mailing list