Bug 7487

Summary: platformize KWQTextCodec
Product: WebKit Reporter: Maciej Stachowiak <mjs>
Component: WebKit Misc.Assignee: Maciej Stachowiak <mjs>
Severity: Normal    
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Description Flags
the patch darin: review+

Description Maciej Stachowiak 2006-02-26 21:38:21 PST
KWQTextCodec needs to be converted to follow WebCore/platform conventions, and to use portable APIs as much as possible.
Comment 1 Maciej Stachowiak 2006-02-26 21:48:10 PST
Created attachment 6752 [details]
the patch
Comment 2 Darin Adler 2006-02-26 22:16:22 PST
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.
Comment 3 Darin Adler 2006-02-27 08:28:39 PST
Maciej landed this last night.