[webkit-reviews] review denied: [Bug 7582] c_utility.cpp contains platform dependant code : [Attachment 6832] proposed patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 7582: c_utility.cpp contains platform dependant code
http://bugzilla.opendarwin.org/show_bug.cgi?id=7582

Attachment 6832: proposed patch
http://bugzilla.opendarwin.org/attachment.cgi?id=6832&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Excellent thing to do -- no reason for this to use CoreFoundation just to
convert to UTF-8.

Ideally we would svn copy uenum.h, ucnv.h, and ucnv_err.h from WebCore instead
of checking new copies in. But that's pretty unimportant.

+    *UTF16Length = UTF8Length+1; 

Our normal coding style would have spaces around the + operator here.

+    *UTF16Chars = NULL;

Our coding style is to use 0 for this sort of thing instead of NULL.

+    UConverter* conv = ucnv_open( "utf8", &status);
+	 conv = ucnv_open( "latin1", &status);

There's a space before "utf8" and "latin1" there.

+	 ucnv_toUChars(conv, *UTF16Chars, *UTF16Length, UTF8Chars,
+				  UTF8Length, &status); 

Seems short enough to fit on one line.

+	 if (U_SUCCESS(status)) {
+	     for (unsigned int i=0; i<*UTF16Length && U_SUCCESS(status); i++) {

+		 if ((*UTF16Chars)[i] == 0xFFFD) {
+		     status = U_INVALID_CHAR_FOUND;
+		 }
+	     }
+	 }

Our coding style doesn't use spaces around single-character things, so these
braces should be removed. Also, we use "unsigned" instead of "unsigned int" and
put spaces around things like the = in "i = 0" and the <  in "i <
*UTF16Length". Also seems unnecessary to have the if (U_SUCCESS) around the
thing -- the loop already takes care of that.

For conversion from UTF-8 to UTF-16 and from Latin-1 to UTF-16 there are much
more efficient ways than calling ucnv -- on the other hand ucnv is at least as
good as calling CF, so that seems fine. UString::UTF8String has UTF-16 -> UTF-8
code and decode() in function.cpp has a function that converts from UTF-8 to
UTF-16, but I think it's better to not make more of these. The conversion from
Latin-1 to UTF-16 is literally just turning unsigned chars into unsigned
shorts, so it's a shame to call the library for that case. But I'm probably
"optimizing for the wrong thing"; seems unnecessary to write custom code for
something that's unlikely to be performance-critical.

Overall, the change looks great. How did you test it? Is there a way we can
test this in one of the layout tests, perhaps with our test plug-in?

Marking review-, not for the minor style issues above, but because we need a
change log and if possible a test to land along with this.