[Webkit-unassigned] [Bug 7582] c_utility.cpp contains platform dependant code

[bugzilla-daemon at opendarwin.org]

http://bugzilla.opendarwin.org/show_bug.cgi?id=7582


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #6832|review?                     |review-
               Flag|                            |




------- Comment #3 from darin at apple.com  2006-03-04 12:08 PDT -------
(From update of attachment 6832)
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.


-- 
Configure bugmail: http://bugzilla.opendarwin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.