[Webkit-unassigned] [Bug 34402] Implement numeric CSS3 list-style-types

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=34402





--- Comment #3 from Daniel Bates <dbates at webkit.org>  2010-01-31 19:19:34 PST ---
(In reply to comment #2)
> (From update of attachment 47801 [details])
> > +        Implements all of the alphabetic CSS3 list-style-types as per
> 
> numeric

Will fix.

> > -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.

Will fix.

> 
> > +    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.

Will look into.

> 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.

Will fix.

> > +            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.

Will fix style.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.