Bug 20953 – Rework the Font handling in QtWebKit

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Holger Freyther]

QtWebKit should use more of the common font codepaths. One constraint QtWebKit has is to always use the "complex" text paths to match the output of the other qt widgets when it comes to text.

[Holger Freyther]

Created attachment 23591 [details]
Split WidthIterator out of Font.cpp

Move WidthIterator to a separate file. svn cp should be used, build systems still need adjustments.

[Holger Freyther]

Created attachment 23592 [details]
Move the "fast" WebCore Font path to a separate file

Move the implementation of the fast webcore font path to a separate file. svn cp should be used, buildsystems need adjustment.

[Holger Freyther]

Created attachment 23593 [details]
Add compiletime option to not use the fast path

For QtWebKit we don't want to go through the simple path and don't want to have the runtime overhead.

[Holger Freyther]

Created attachment 23594 [details]
Bring the QtWebKit font handling more inline with the rest of WebCore

This is mostly done and helps us to pass acid3. The Qt4.3 support needs to be fixed though.

[Dave Hyatt]

Comment on attachment 23592 [details]
Move the "fast" WebCore Font path to a separate file

I don't really like the idea of splitting the fast font path into its own file.  Why is that necessary?

[Eric Seidel (no email)]

Comment on attachment 23593 [details]
Add compiletime option to not use the fast path

Looks sane enough to me.  Normally we don't use "return foo()" in functions returning void.

The WTF_USE definition should really go in WebCore/config.h (unless you all don't use that).

[Eric Seidel (no email)]

Comment on attachment 23592 [details]
Move the "fast" WebCore Font path to a separate file

I expect this would need to be performance tested by the Apple folks for them to accept it.  I'm not sure if the font stuff depends on this all getting inlined or not.  Looks good to me though.

Hyatt should really review this.

[Holger Freyther]

(In reply to comment #5)
> (From update of attachment 23592 [details] [edit])
> I don't really like the idea of splitting the fast font path into its own file.
>  Why is that necessary?

The other option would be to add a #ifdef around the code inside Font.cpp. It needs to be #ifdef'ed or moved out though. QtWebKit does not want to use the WebCore fast font path, in order to compile the WebCore fast font path we would need to have a very complex FontCache, FontFallBackList, GlyphPageTree.. implementation that would be a duplication of Qt's internals and mostly unused.

[Dave Hyatt]

Right, I get that.  I think it's better to just ifdef in Font.cpp though.  We get so much confusion over RenderBlock being split into two files... I think something similar would happen if Font got split up... people would get confused over where functions were... or where to put new functions.

[Dave Hyatt]

Splitting WidthIterator out is good though.

[Darin Adler]

Comment on attachment 23591 [details]
Split WidthIterator out of Font.cpp

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.

[Eric Seidel (no email)]

Comment on attachment 23594 [details]
Bring the QtWebKit font handling more inline with the rest of WebCore

This seems sane enough to me.  This is great to see the Qt port's font code aligning more with the other ports.

There are a few style violations in this patch (mostly * and & spacing inconsistent with the WebKit style guidelines).  There are also a bunch of // qtDebug() comments.  We generally don't commit commented out code, so I would encourage you to remove those.

This patch of course depends on the other two, which I feel hyatt or mitz are much more capable of reviewing than I.

[Maciej Stachowiak]

Has anyone done perf testing to determine wether the fast path makes a performance difference for Qt? It might be good to know if this causes a perf regression (though perhaps the Qt folks are willing to take the speed hit for other reasons).

[Simon Hausmann]

(In reply to comment #13)
> Has anyone done perf testing to determine wether the fast path makes a
> performance difference for Qt? It might be good to know if this causes a perf
> regression (though perhaps the Qt folks are willing to take the speed hit for
> other reasons).

I don't have hard numbers, but the main problem with the "fast" path is that it's slower with Qt because we can not provide a proper mapping from characters to glyphs. There is currently no public API in Qt for that, and as a result of that the implementation would have to map a character to a "virtual" glyph that is the unicode value again. When drawing the glyphs we treat them as characters and pass them as string to Qt to draw the text. That is much more work and uses more memory than just always going the "complex" path of passing the string down to the toolkit.

I recall very well how the qt port become significantly faster after Lars removed the "fast" path for the Qt port.

[Darin Adler]

Comment on attachment 23591 [details]
Split WidthIterator out of Font.cpp

Cleared review flag since I landed this.

[Eric Seidel (no email)]

I think the only way to get this patch reviewed is to catch hyatt on IRC and talk it out with him.  I don't feel like enough of an expert in our font system to give these patches a good review.  I'm going to mark the last one as r?hyatt too.

[Eric Seidel (no email)]

Comment on attachment 23594 [details]
Bring the QtWebKit font handling more inline with the rest of WebCore

Marking this is r?hyatt as hyatt is really our font expert (or was at least last time I checked).  I don't feel enough of an expert in this code to review it, but certainly if someone else does they should feel free to do so!

[Darin Adler]

Comment on attachment 23592 [details]
Move the "fast" WebCore Font path to a separate file

Moving to a separate file seems fine.

> +/**

We don't use that double-star format.

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

You can leave this out of the new file.

> + * Copyright (C) 1999 Lars Knoll ([e-mail])
> + *           (C) 1999 Antti Koivisto ([e-mail])
> + *           (C) 2000 Dirk Mueller ([e-mail])
> + * Copyright (C) 2003, 2006 Apple Computer, Inc.
> + * Copyright (C) 2008 Holger Hans Peter Freyther

None of the code you're moving is covered by the older copyrights (older than 2003); it's all new stuff. So you can omit those older copyright notices from this new file.
> +    return data;
> +}
> +
> +
> +
> +}

We just use one blank line in places like this, not 3.

r=me on moving this code

[Darin Adler]

Comment on attachment 23592 [details]
Move the "fast" WebCore Font path to a separate file

Wait a second, this only includes fixes to GNUmakefile.am, not the makefiles for other platforms. So it will break all builds that don't use GNUmakefile.am if landed as-is.

[Holger Freyther]

Comment on attachment 23592 [details]
Move the "fast" WebCore Font path to a separate file

Clear review.

[Simon Hausmann]

Landed in r39207 and the three commits before.