Bug 34402 – Implement numeric CSS3 list-style-types

• Component: CSS
• Status: RESOLVED FIXED

[Daniel Bates]

Following up after the passing of bug #33089,  we should implement all of the numeric CSS3 list-style-types as per section 4.3 of the CSS3 Lists module <http://www.w3.org/TR/css3-lists/#numeric>.

[Daniel Bates]

Created attachment 47801 [details]
Patch with test case

[Darin Adler]

Comment on attachment 47801 [details]
Patch with test case

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

[Daniel Bates]

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

[Daniel Bates]

(In reply to comment #2)
> 
> > +    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.

Will fix.

[Daniel Bates]

Created attachment 48291 [details]
Patch with test case

Updated patch based on Darin's comments.

[WebKit Review Bot]

Attachment 48291 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/240765

[Daniel Bates]

I have confirmed with Dimitri Glazkov that this is an issue with the Chromium EWS bot. Notice, the linked output states:
...
g++: Internal error: Killed (program cc1plus)
Please submit a full bug report.
...

CC'ing Adam Barth

(In reply to comment #6)
> Attachment 48291 [details] did not build on chromium:
> Build output: http://webkit-commit-queue.appspot.com/results/240765

[Darin Adler]

Comment on attachment 48291 [details]
Patch with test case

> +static inline String toAlphabeticOrNumeric(int number, const UChar* sequence, int sequenceSize, bool isAlphabeticSequence)

Elsewhere in WebKit code we've found that enums are easier to read than bools for arguments like this where the value passed is a constant.

    enum SequenceType { NumericSequence, AlphabeticSequence };

You can still name the argument isAlphabeticSequence and use boolean-style logic inside the function.

> +    ASSERT(sizeof(number) <= 4);

This could instead be a compile-time assert.

> +    const int lettersSize = 32 + 1; // big enough to hold the ASCII representation of the largest negative 32-bit int in binary plus a minus sign.

Since this is the only place we are depending on the size of the number argument, then I think you we just use sizeof in this expression instead of using an assertion.

    const int lettersSize = sizeof(number) * 8 + 1; // Binary is the worst case; requires one character per bit plus a minus sign.

> +        numberShadow = -number;

I'm glad this works for MIN_INT, but I'm not sure how it works.

r=me

[Daniel Bates]

(In reply to comment #8)
> (From update of attachment 48291 [details])
> > +static inline String toAlphabeticOrNumeric(int number, const UChar* sequence, int sequenceSize, bool isAlphabeticSequence)
> 
> Elsewhere in WebKit code we've found that enums are easier to read than bools
> for arguments like this where the value passed is a constant.
> 
>     enum SequenceType { NumericSequence, AlphabeticSequence };

Added enum and modified code accordingly.

> 
> You can still name the argument isAlphabeticSequence and use boolean-style
> logic inside the function.
> 
> > +    ASSERT(sizeof(number) <= 4);
> 
> This could instead be a compile-time assert.

Removed assert.

> 
> > +    const int lettersSize = 32 + 1; // big enough to hold the ASCII representation of the largest negative 32-bit int in binary plus a minus sign.
> 
> Since this is the only place we are depending on the size of the number
> argument, then I think you we just use sizeof in this expression instead of
> using an assertion.
> 
>     const int lettersSize = sizeof(number) * 8 + 1; // Binary is the worst
> case; requires one character per bit plus a minus sign.
> 

Changed code to reflect this.

> > +        numberShadow = -number;
> 
> I'm glad this works for MIN_INT, but I'm not sure how it works.

By the definition of two's complement and unsigned int.

> r=me

Thank you for reviewing this patch.

[Daniel Bates]

Committed r54472: <http://trac.webkit.org/changeset/54472>

[Adam Barth]

> g++: Internal error: Killed (program cc1plus)

Strange!