[webkit-reviews] review granted: [Bug 27952] cssgrammar.cpp fails to compile with RVCT compiler : [Attachment 34201] second try.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Laszlo Gombos
<laszlo.1.gombos at nokia.com>'s request for review:
Bug 27952: cssgrammar.cpp fails to compile with RVCT compiler
https://bugs.webkit.org/show_bug.cgi?id=27952

Attachment 34201: second try.
https://bugs.webkit.org/attachment.cgi?id=34201&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
I think this change is OK.

I'd prefer that function declarations that don't need argument names don't have
them. And the one for equalIgnoringCase uses "a" and "b" even though those
don't add any information.

I'd prefer it if all the string-like functions that operate on runs of
characters were grouped together. At this point, the others are in
PlatformString.h, but these new ones are in StringImpl.h.

I'd prefer it if the functions that work with characters were as consistent as
possible. The ones in PlatformString.h use size_t for length, but this one uses
unsigned. The ones in PlatformString.h use "characters" as a prefix on the
function name, but this one uses overloading instead. The ones in
PlatformString.h put the UChar* and length first, and other arguments
afterward, but these new ones do not.

Finally, I think tis function should be more clear on how it handles the const
char*. If that's intended to be a null-terminated C string, then we need to
check for the null character, because otherwise if the UChar contains a 0 in
just the right place we could walk off the end of the passed in const char*.
Alternatively, if we want to pass in the length then I think we'd need a second
length argument for the const char*. I don't think the function definition
makes it clear how it's intended to be used.

But I don't think any of these concerns should prevent you from landing this. I
just think they would be things to improve in the future.