Bug 27952

Created attachment 34033 [details] proposed patch. Invoke the cast to String operator by hand.

Comment on attachment 34033 [details] proposed patch. This is going to copy the string. that's an extra malloc during parsing, and likely a slowdown. :(

(In reply to comment #2) > (From update of attachment 34033 [details]) > This is going to copy the string. that's an extra malloc during parsing, and > likely a slowdown. :( The old code already copies the string. This is bad. The best fix that fixes both compiling and improves performance is to create a charactersEqualIgnoringCase function that works with a character pointer and length. This would be like the other "characters" functions in PlatformString.h. We could also overload equalIgnoringCase for CSSParserString; that would just be a convenience that would make it slightly easier to read. Code to do this is already present in StringImpl.cpp; it should be relatively straightforward to refactor it. In the mean time I actually see nothing wrong with Laszlo's initial patch, except that there is a stray space after "selector". But I'd love it if we could get rid of the stray memory allocation here.

Created attachment 34201 [details] second try. Darin, thanks for the detailed recommendation. As you suggested I get rid of the stray allocation by working directly with characters.

Comment on attachment 34201 [details] second try. 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.

Fix landed in r46846 /me agrees with Darins comments

This patch claims to have been landed.