Bug 7487 - platformize KWQTextCodec
Summary: platformize KWQTextCodec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Maciej Stachowiak
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-02-26 21:38 PST by Maciej Stachowiak
Modified: 2006-02-27 08:28 PST (History)
0 users

See Also:


Attachments
the patch (253.81 KB, patch)
2006-02-26 21:48 PST, Maciej Stachowiak
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.