[webkit-reviews] review denied: [Bug 34402] Implement numeric CSS3 list-style-types : [Attachment 47801] Patch with test case

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 34402: Implement numeric CSS3 list-style-types
https://bugs.webkit.org/show_bug.cgi?id=34402

Attachment 47801: Patch with test case
https://bugs.webkit.org/attachment.cgi?id=47801&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   Implements all of the alphabetic CSS3 list-style-types as per

numeric

> -static String toAlphabetic(int number, const UChar* alphabet, int
alphabetSize)
> +static inline String toAlphabeticOrNumeric(int number, const UChar*
sequence, int sequenceSize, bool isAlphabeticSequence)
>  {
> -    ASSERT(alphabetSize >= 10);
> -
> -    if (number < 1)
> -	   return String::number(number);
> +    ASSERT(sequenceSize >= 2);
>  
>      const int lettersSize = 10; // big enough for a 32-bit int, with a
10-letter alphabet

The constant and comment made sense before when there was a minimum alphabet
size of 10 enforced by the assertion above. But clearly 10 characters is not
enough for binary.

> +    if (isAlphabeticSequence)
> +	   while ((number /= sequenceSize) > 0)
> +	       letters[lettersSize - ++length] = sequence[number % sequenceSize
- 1];
> +    else
> +	   while ((number /= sequenceSize) > 0)
> +	       letters[lettersSize - ++length] = sequence[number %
sequenceSize];

We use braces around multi-line blocks in WebKit code, so the two while loops
should be enclosed in braces.

> +    String numberStr = toAlphabeticOrNumeric(number, numerals, numeralsSize,
false);
> +    return (number < 0) ? "-" + numberStr : numberStr;

Does this really work for negatives? I don't see any test cases that cover it,
and I think this will fail because we pass the negative number in to the
toAlphabeticOrNumeric function, not the absolute value. It's also challenging
to handle the maximum negative integer if you do it like this.

String concatenation requires extra memory allocations with WebCore::String, so
it would be better if the sign was handled inside the function and put in the
string when it's constructed instead of being concatenated later.

> +	       static const UChar arabicIndicNumerals[10] = {
> +		   0x0660, 0x0661, 0x0662, 0x0663, 0x0664, 0x0665, 0x0666,
0x0667, 0x0668,
> +		   0x0669
> +	       };

Would look nicer if all these 10-element arrays were on a single line rather
than two lines. There's no 80-column limit or anything to worry about.

review- because the negative number part is wrong

Otherwise, things look great to me.