[webkit-reviews] review granted: [Bug 7487] platformize KWQTextCodec : [Attachment 6752] the patch

bugzilla-request-daemon at opendarwin.org bugzilla-request-daemon at opendarwin.org
Sun Feb 26 22:16:22 PST 2006

Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 7487: platformize KWQTextCodec

Attachment 6752: the patch

------- Additional Comments from Darin Adler <darin at apple.com>
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

Looks fine, r=me.

More information about the webkit-reviews mailing list