Bug 7487 – platformize KWQTextCodec

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Maciej Stachowiak]

KWQTextCodec needs to be converted to follow WebCore/platform conventions, and to use portable APIs as much as possible.

[Maciej Stachowiak]

Created attachment 6752 [details]
the patch

[Darin Adler]

Comment on attachment 6752 [details]
the patch

Saw QTextencoding in a comment. Please remove.

+#include <TextEncoding.h>

Should use "" not <>.

+    if (enc.isNull() || enc.isEmpty())

Above code is silly, since isEmpty includes isNull.

+    StreamingTextDecoder *m_decoder;

* should be next to type name.

To answer your question:

+        // FIXME: we have an alias table already, do we need to consult icu's as well?
+        // do they know extra aliases?

Yes, ICU does have some extra aliases that our table does not. Alexy Prosukaryov recently added that instead of adding the aliases to our table (not sure the bug number but it should be in change log). I'm not entirely comfortable with a mix of our own aliases and ICU's.

There's a mention of TextEncoding::heuristicContentMatch in a comment. I think that's a Qt thing and should not be mentioned in a comment any more.

ExtraCFEncodings.h should just have the macros; I don't see any reason it needs to include CoreFoundation.h.

+// Since they are macros, they won't cause a compile failure even a the CFString constant is added.

The above comment should say "if the".

Why all the WebCore:: in WebCoreFrameBridge.mm? I think that entire file has "using namespace WebCore" and the explicit namespace prefixes are not needed.

character-sets.txt is missing from the patch -- it's being deleted but not added.

Looks fine, r=me.

[Darin Adler]

Maciej landed this last night.