Bug 33089 – Implement alphabetic CSS3 list-style-types

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

Instead of just implementing the upper-greek list-style-type (bug #32138). We should implement all of the alphabetic CSS3 list-style-types as per section 4.4 of the CSS3 Lists module <http://www.w3.org/TR/css3-lists/#alphabetic>.

[Daniel Bates]

Created attachment 45725 [details]
Patch with test case

[WebKit Review Bot]

Attachment 45725 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/style/RenderStyle.h:182:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1

[Daniel Bates]

Created attachment 45726 [details]
Patch with test case

Fixed style issue to make style-bot happy.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45726 [details] without any errors.

[Daniel Bates]

Comment on attachment 45726 [details]
Patch with test case

Need to update patch with suffixes. For instance, Afar uses Unicode code point U+1366 for the suffix, instead of '.'.

[Daniel Bates]

Created attachment 46017 [details]
Patch with test case

Implements list-style-type suffixes.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46017 [details] without any errors.

[Daniel Bates]

Created attachment 46019 [details]
Patch with test case

Updated comment in file WebCore/css/CSSParser.cpp to include added list-style-types.

Also, updated the entry in the WebCore/ChangeLog.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46019 [details] without any errors.

[Darin Adler]

Comment on attachment 46019 [details]
Patch with test case

>          // disc | circle | square | decimal | decimal-leading-zero | lower-roman |
>          // upper-roman | lower-greek | lower-alpha | lower-latin | upper-alpha |
> -        // upper-latin | hebrew | armenian | georgian | cjk-ideographic | hiragana |
> -        // katakana | hiragana-iroha | katakana-iroha | none | inherit
> +        // upper-latin | afar | ethiopic-halehame-aa-et | ethiopic-halehame-aa-er |
> +        // amharic | ethiopic-halehame-am-et | amharic-abegede | ethiopic-abegede-am-et |
> +        // cjk-earthly-branch | cjk-heavenly-stem | ethiopic | ethiopic-halehame-gez |
> +        // ethiopic-abegede | ethiopic-abegede-gez | hangul-consonant | hangul |
> +        // lower-norwegian | oromo | ethiopic-halehame-om-et | sidama |
> +        // ethiopic-halehame-sid-et | somali | ethiopic-halehame-so-et | tigre |
> +        // ethiopic-halehame-tig | tigrinya-er | ethiopic-halehame-ti-er |
> +        // tigrinya-er-abegede | ethiopic-abegede-ti-er | tigrinya-et |
> +        // ethiopic-halehame-ti-et | tigrinya-et-abegede | ethiopic-abegede-ti-et |
> +        // upper-greek | upper-norwegian | hebrew | armenian | georgian |
> +        // cjk-ideographic | hiragana | katakana | hiragana-iroha | katakana-iroha |
> +        // none | inherit

So lame that you have to do this. This comments can be useful, but it's always a pain to have one more place you have to edit! My apologies.

> +afar
> +ethiopic-halehame-aa-et
> +ethiopic-halehame-aa-er
> +amharic
> +ethiopic-halehame-am-et
> +amharic-abegede
> +ethiopic-abegede-am-et
> +cjk-earthly-branch
> +cjk-heavenly-stem
> +ethiopic
> +ethiopic-halehame-gez
> +ethiopic-abegede
> +ethiopic-abegede-gez
> +hangul-consonant
> +hangul
> +lower-norwegian
> +oromo
> +ethiopic-halehame-om-et
> +sidama
> +ethiopic-halehame-sid-et
> +somali
> +ethiopic-halehame-so-et
> +tigre
> +ethiopic-halehame-tig
> +tigrinya-er
> +ethiopic-halehame-ti-er
> +tigrinya-er-abegede
> +ethiopic-abegede-ti-er
> +tigrinya-et
> +ethiopic-halehame-ti-et
> +tigrinya-et-abegede
> +ethiopic-abegede-ti-et
> +upper-greek
> +upper-norwegian

Is there an order here? Could we sort these alphabetically so the order isn't seemingly-random?

> -        "upper-alpha", "upper-latin", "hebrew", "armenian", "georgian", "cjk-ideographic", "hiragana", "katakana",
> -        "hiragana-iroha", "katakana-iroha", "inline", "block", "list-item", "run-in", "compact", "inline-block",
> -        "table", "inline-table", "table-row-group", "table-header-group", "table-footer-group", "table-row",
> +        "upper-alpha", "upper-latin", "afar", "ethiopic-halehame-aa-et", "ethiopic-halehame-aa-er", "amharic",
> +        "ethiopic-halehame-am-et", "amharic-abegede", "ethiopic-abegede-am-et", "cjk-earthly-branch", "cjk-heavenly-stem",
> +        "ethiopic", "ethiopic-halehame-gez", "ethiopic-abegede", "ethiopic-abegede-gez", "hangul-consonant", "hangul",
> +        "lower-norwegian", "oromo", "ethiopic-halehame-om-et", "sidama", "ethiopic-halehame-sid-et", "somali",
> +        "ethiopic-halehame-so-et", "tigre", "ethiopic-halehame-tig", "tigrinya-er", "ethiopic-halehame-ti-er",
> +        "tigrinya-er-abegede", "ethiopic-abegede-ti-er", "tigrinya-et", "ethiopic-halehame-ti-et", "tigrinya-et-abegede",
> +        "ethiopic-abegede-ti-et", "upper-greek", "upper-norwegian", "hebrew", "armenian", "georgian", "cjk-ideographic",
> +        "hiragana", "katakana", "hiragana-iroha", "katakana-iroha", "inline", "block", "list-item", "run-in", "compact",
> +        "inline-block", "table", "inline-table", "table-row-group", "table-header-group", "table-footer-group", "table-row",

Same question about sort order.

> +        return 0x1366;

In most cases, we use names for characters rather than just putting the number in. That's typically done by adding named constants to CharacterNames.h that exactly match the names from the Unicode specification. The exception would be in a table where there are so many characters it is not helpful. This seems like a case where the name would be better.

> +            static const UChar ethiopicHalehameAaErAlphabet[18] = {
> +                0x1200, 0x1208, 0x1210, 0x1218, 0x1228, 0x1230, 0x1260, 0x1270, 0x1290,
> +                0x12A0, 0x12A8, 0x12C8, 0x12D0, 0x12E8, 0x12F0, 0x1308, 0x1338, 0x1348
> +            };

How confident are we that these tables are correct? I was concerned when I considered doing this, because the CSS draft had a lot of cautions about not knowing if the tables were correct.

> +            return toAlphabetic(value, ethiopicHalehameAaErAlphabet, 18);

It's unfortunate to repeat the constants like "18" in the call sites like this. I'd rather see us use a sizeof approach so the constant isn't repeated twice. I believe someone had a patch to add a "number of array elements" macro or function template to WTF. And I think there's a way to do this with a function template so we don't even have to type the sizeof each time, although I'm not sure. If all else fails I suggest using a macro.

> -    switch (style()->listStyleType()) {
> +    const EListStyleType type = style()->listStyleType();

For brevity, WebKit code doesn't use const for the values local variables, even though we could for many; almost all. Please don't do it here.

> +    const UChar suffix = listMarkerSuffix(type);

Same issue here.

> -        const UChar periodSpace[2] = { '.', ' ' };
> -        context->drawText(style()->font(), TextRun(periodSpace, 2), marker.location() + IntSize(width, 0));
> +        UChar suffixSpace[2];
> +        suffixSpace[0] = suffix;
> +        suffixSpace[1] = ' ';

You could use array initialization syntax here as the old code did. It's good that you removed the const, by the way.

> -        const UChar spacePeriod[2] = { ' ', '.' };
> -        TextRun spacePeriodRun(spacePeriod, 2);
> -        int width = font.width(spacePeriodRun);
> -        context->drawText(style()->font(), spacePeriodRun, marker.location());
> +        UChar spaceSuffix[2];
> +        spaceSuffix[0] = ' ';
> +        spaceSuffix[1] = suffix;
> +        TextRun spaceSuffixRun(spaceSuffix, 2);
> +        int width = font.width(spaceSuffixRun);
> +        context->drawText(style()->font(), spaceSuffixRun, marker.location());

Same here.

> -                const UChar periodSpace[2] = { '.', ' ' };
> -                int periodSpaceWidth = font.width(TextRun(periodSpace, 2));
> -                width = itemWidth + periodSpaceWidth;
> +                UChar suffixSpace[2];
> +                suffixSpace[0] = listMarkerSuffix(type);
> +                suffixSpace[1] = ' ';

And here.

> +    const EListStyleType type = style()->listStyleType();

Const here.

> -            const UChar periodSpace[2] = { '.', ' ' };
> -            int periodSpaceWidth = font.width(TextRun(periodSpace, 2));
> -            return IntRect(x(), y() + font.ascent(), itemWidth + periodSpaceWidth, font.height());
> +            UChar suffixSpace[2];
> +            suffixSpace[0] = listMarkerSuffix(type);
> +            suffixSpace[1] = ' ';
> +            int suffixSpaceWidth = font.width(TextRun(suffixSpace, 2));
> +            return IntRect(x(), y() + font.ascent(), itemWidth + suffixSpaceWidth, font.height());

Same array issue here again.

>          unsigned _box_direction : 1; // EBoxDirection (CSS3 box_direction property, flexible box layout module)
> -        // 32 bits
> +        // 33 bits

There is another comment a few lines later than also needs to be updated. The one that says "39 bits". And then one further down, the one that says "48 bits".

> +     Afar, EthiopicHalehameAaEt, EthiopicHalehameAaEr, Amharic,
> +     EthiopicHalehameAmEt, AmharicAbegede, EthiopicAbegedeAmEt,
> +     CjkEarthlyBranch, CjkHeavenlyStem, Ethiopic, EthiopicHalehameGez,
> +     EthiopicAbegede, EthiopicAbegedeGez, HangulConsonant, Hangul,
> +     LowerNorwegian, Oromo, EthiopicHalehameOmEt, Sidama,
> +     EthiopicHalehameSidEt, Somali, EthiopicHalehameSoEt, Tigre,
> +     EthiopicHalehameTig, TigrinyaEr, EthiopicHalehameTiEr,
> +     TigrinyaErAbegede, EthiopicAbegedeTiEr, TigrinyaEt,
> +     EthiopicHalehameTiEt, TigrinyaEtAbegede, EthiopicAbegedeTiEt,
> +     UpperGreek, UpperNorwegian,
>       Hebrew, Armenian, Georgian, CJKIdeographic,
>       Hiragana, Katakana, HiraganaIroha, KatakanaIroha, NoneListStyle

At a certain point these lists are far better if sorted, and one item per line.

> +        /* 
> +        The following styles are ordered as they appear in section 4.4. of the 
> +        Draft 7 November 2002 draft of the CSS3 Lists module <http://www.w3.org/TR/css3-lists/#alphabetic>.
> +        */

Ah, the answer to my question about ordering. I'm not sure what's best.

By the way, we almost always prefer "//" comments everywhere, even in CSS and test cases.

A test case is much more valuable if it can be a dumpAsText test rather than a render-tree-based test, since those usually need different results per-platform. That having been said, I can't think of a way to do this at the moment. We should try to think of a way, though, because I think it makes things easier to maintain and test later. It's also much better to have a test where we can see if we're passing or failing even if we don't have fonts that cover all the characters. Worth thinking on this too.

review- because I am sure that at least one of the comments above will lead to a code change, if only the number of bits comments, for example

[Daniel Bates]

(In reply to comment #10)
> (From update of attachment 46019 [details])
> >          // disc | circle | square | decimal | decimal-leading-zero | lower-roman |
> >          // upper-roman | lower-greek | lower-alpha | lower-latin | upper-alpha |
> > -        // upper-latin | hebrew | armenian | georgian | cjk-ideographic | hiragana |
> > -        // katakana | hiragana-iroha | katakana-iroha | none | inherit
> > +        // upper-latin | afar | ethiopic-halehame-aa-et | ethiopic-halehame-aa-er |
>> [...]
> > +        // upper-greek | upper-norwegian | hebrew | armenian | georgian |
> > +        // cjk-ideographic | hiragana | katakana | hiragana-iroha | katakana-iroha |
> > +        // none | inherit
> 
> So lame that you have to do this. This comments can be useful, but it's always
> a pain to have one more place you have to edit! My apologies.

Maybe we should remove this comment? or better yet, either refer to section CSS_PROP_LIST_STYLE_TYPE in file CSSValueKeywords.in or to the enum EListStyleType in RenderStyleConstants.h.

I have another patch (coming shortly - depends on this one) to implement all the numeric CSS3 list-style-types. So, this comment will get even longer.

> > +afar
> > +ethiopic-halehame-aa-et
> > +ethiopic-halehame-aa-er
> > +amharic
> > [...]
> > +ethiopic-abegede-ti-et
> > +upper-greek
> > +upper-norwegian
> 
> Is there an order here? Could we sort these alphabetically so the order isn't
> seemingly-random?

I'll re-order  the whole section CSS_PROP_LIST_STYLE_TYPE in file CSSValueKeywords.in alphabetically.

For your reference, they are ordered according to how they appear in the spec. <http://www.w3.org/TR/css3-lists/#alphabetic>, which is grouped by alias and sorted alphabetically by the group representative.  For instance:

For example, the first two groups in the spec are:
[afar], ethiopic-halehame-aa-et, ethiopic-halehame-aa-er

And

[amharic], ethiopic-halehame-am-et

where [X] is the representative of the group.

> 
> > -        "upper-alpha", "upper-latin", "hebrew", "armenian", "georgian", "cjk-ideographic", "hiragana", "katakana",
> > -        "hiragana-iroha", "katakana-iroha", "inline", "block", "list-item", "run-in", "compact", "inline-block",
> > [...]
> > +        "inline-block", "table", "inline-table", "table-row-group", "table-header-group", "table-footer-group", "table-row",
> 
> Same question about sort order.

Will re-order alphabetically.

> 
> > +        return 0x1366;
> 
> In most cases, we use names for characters rather than just putting the number
> in. That's typically done by adding named constants to CharacterNames.h that
> exactly match the names from the Unicode specification. The exception would be
> in a table where there are so many characters it is not helpful. This seems
> like a case where the name would be better.
> 
> > +            static const UChar ethiopicHalehameAaErAlphabet[18] = {
> > +                0x1200, 0x1208, 0x1210, 0x1218, 0x1228, 0x1230, 0x1260, 0x1270, 0x1290,
> > +                0x12A0, 0x12A8, 0x12C8, 0x12D0, 0x12E8, 0x12F0, 0x1308, 0x1338, 0x1348
> > +            };
> 
> How confident are we that these tables are correct? I was concerned when I
> considered doing this, because the CSS draft had a lot of cautions about not
> knowing if the tables were correct.

I am confident that I created the tables with the same Unicode code points as in the spec., with the exception for upper-greek (which I added the missing characters and notified Ian Hickson) (*).

The only ones in the spec. (at this time) that are missing characters are for hiragana-iroha, hiragana, katakana-iroha, katakana, and upper-greek (not listed, see (*)).

For your reference, the tables for hiragana-iroha, hiragana, katakana-iroha, katakana are currently in in the tree.

> 
> > +            return toAlphabetic(value, ethiopicHalehameAaErAlphabet, 18);
> 
> It's unfortunate to repeat the constants like "18" in the call sites like this.
> I'd rather see us use a sizeof approach so the constant isn't repeated twice. I
> believe someone had a patch to add a "number of array elements" macro or
> function template to WTF. And I think there's a way to do this with a function
> template so we don't even have to type the sizeof each time, although I'm not
> sure. If all else fails I suggest using a macro.

I'll look into this.

> > -    switch (style()->listStyleType()) {
> > +    const EListStyleType type = style()->listStyleType();
> 
> For brevity, WebKit code doesn't use const for the values local variables, even
> though we could for many; almost all. Please don't do it here.

Will change.

> 
> > +    const UChar suffix = listMarkerSuffix(type);
> 

Will change.

> 
> > -        const UChar periodSpace[2] = { '.', ' ' };
> > -        context->drawText(style()->font(), TextRun(periodSpace, 2), marker.location() + IntSize(width, 0));
> > +        UChar suffixSpace[2];
> > +        suffixSpace[0] = suffix;
> > +        suffixSpace[1] = ' ';
> 
> You could use array initialization syntax here as the old code did. It's good
> that you removed the const, by the way.

Will change.

> 
> > -        const UChar spacePeriod[2] = { ' ', '.' };
> > -        TextRun spacePeriodRun(spacePeriod, 2);
> > -        int width = font.width(spacePeriodRun);
> > -        context->drawText(style()->font(), spacePeriodRun, marker.location());
> > +        UChar spaceSuffix[2];
> > +        spaceSuffix[0] = ' ';
> > +        spaceSuffix[1] = suffix;
> > +        TextRun spaceSuffixRun(spaceSuffix, 2);
> > +        int width = font.width(spaceSuffixRun);
> > +        context->drawText(style()->font(), spaceSuffixRun, marker.location());
> 

Will change.

> 
> > -                const UChar periodSpace[2] = { '.', ' ' };
> > -                int periodSpaceWidth = font.width(TextRun(periodSpace, 2));
> > -                width = itemWidth + periodSpaceWidth;
> > +                UChar suffixSpace[2];
> > +                suffixSpace[0] = listMarkerSuffix(type);
> > +                suffixSpace[1] = ' ';
> 

Will change.

> 
> > +    const EListStyleType type = style()->listStyleType();
> 

Will remove const.

> 
> > -            const UChar periodSpace[2] = { '.', ' ' };
> > -            int periodSpaceWidth = font.width(TextRun(periodSpace, 2));
> > -            return IntRect(x(), y() + font.ascent(), itemWidth + periodSpaceWidth, font.height());
> > +            UChar suffixSpace[2];
> > +            suffixSpace[0] = listMarkerSuffix(type);
> > +            suffixSpace[1] = ' ';
> > +            int suffixSpaceWidth = font.width(TextRun(suffixSpace, 2));
> > +            return IntRect(x(), y() + font.ascent(), itemWidth + suffixSpaceWidth, font.height());
> 

Will change.

> 
> >          unsigned _box_direction : 1; // EBoxDirection (CSS3 box_direction property, flexible box layout module)
> > -        // 32 bits
> > +        // 33 bits
> 
> There is another comment a few lines later than also needs to be updated. The
> one that says "39 bits". And then one further down, the one that says "48
> bits".

Will update comments.

> > +     Afar, EthiopicHalehameAaEt, EthiopicHalehameAaEr, Amharic,
> > +     EthiopicHalehameAmEt, AmharicAbegede, EthiopicAbegedeAmEt,
> > +     CjkEarthlyBranch, CjkHeavenlyStem, Ethiopic, EthiopicHalehameGez,
> > +     EthiopicAbegede, EthiopicAbegedeGez, HangulConsonant, Hangul,
> > +     LowerNorwegian, Oromo, EthiopicHalehameOmEt, Sidama,
> > +     EthiopicHalehameSidEt, Somali, EthiopicHalehameSoEt, Tigre,
> > +     EthiopicHalehameTig, TigrinyaEr, EthiopicHalehameTiEr,
> > +     TigrinyaErAbegede, EthiopicAbegedeTiEr, TigrinyaEt,
> > +     EthiopicHalehameTiEt, TigrinyaEtAbegede, EthiopicAbegedeTiEt,
> > +     UpperGreek, UpperNorwegian,
> >       Hebrew, Armenian, Georgian, CJKIdeographic,
> >       Hiragana, Katakana, HiraganaIroha, KatakanaIroha, NoneListStyle
> 
> At a certain point these lists are far better if sorted, and one item per line.

Will sort alphabetically and put one item per line.

> 
> > +        /* 
> > +        The following styles are ordered as they appear in section 4.4. of the 
> > +        Draft 7 November 2002 draft of the CSS3 Lists module <http://www.w3.org/TR/css3-lists/#alphabetic>.
> > +        */
> 
> Ah, the answer to my question about ordering. I'm not sure what's best.
> 
> By the way, we almost always prefer "//" comments everywhere, even in CSS and
> test cases.

From my understanding, we can't use "//"-style comments in CSS as per section 4.1.9 of the CSS 2.1 spec. <http://www.w3.org/TR/CSS2/syndata.html#comments>. So, we have to use C-style comments (i.e. /* ... */).

> A test case is much more valuable if it can be a dumpAsText test rather than a
> render-tree-based test, since those usually need different results
> per-platform. That having been said, I can't think of a way to do this at the
> moment. We should try to think of a way, though, because I think it makes
> things easier to maintain and test later. It's also much better to have a test
> where we can see if we're passing or failing even if we don't have fonts that
> cover all the characters. Worth thinking on this too.

I included a render-tree dump, which includes the text for the list marker character. So, this can be tested using DRT.

Ideally, we should add a dumpListMarkersAsText() command to DRT so that we can just use dumpAsText. This is best addressed in a separate bug.

[Darin Adler]

(In reply to comment #11)
> Maybe we should remove this comment? or better yet, either refer to section
> CSS_PROP_LIST_STYLE_TYPE in file CSSValueKeywords.in or to the enum
> EListStyleType in RenderStyleConstants.h.

Yes, we should do something like that.

> For your reference, they are ordered according to how they appear in the spec.

Eventually I figured that out.

> > In most cases, we use names for characters rather than just putting the number
> > in. That's typically done by adding named constants to CharacterNames.h that
> > exactly match the names from the Unicode specification. The exception would be
> > in a table where there are so many characters it is not helpful. This seems
> > like a case where the name would be better.

You didn't do the "Will change" on this one. I hope you decide to do it.

> For your reference, the tables for hiragana-iroha, hiragana, katakana-iroha,
> katakana are currently in in the tree.

And they are probably wrong? That is worth fixing, although naturally not in this patch.

> From my understanding, we can't use "//"-style comments in CSS as per section
> 4.1.9 of the CSS 2.1 spec. <http://www.w3.org/TR/CSS2/syndata.html#comments>.
> So, we have to use C-style comments (i.e. /* ... */).

I see.

> I included a render-tree dump, which includes the text for the list marker
> character. So, this can be tested using DRT.

Yes, understood.
 
> Ideally, we should add a dumpListMarkersAsText() command to DRT so that we can
> just use dumpAsText. This is best addressed in a separate bug.

Or we could make dumping the list markers the default, and update existing test results. Most tests won't have any list markers. I think either of these is well worth doing.

[Daniel Bates]

(In reply to comment #12)

> > > In most cases, we use names for characters rather than just putting the number
> > > in. That's typically done by adding named constants to CharacterNames.h that
> > > exactly match the names from the Unicode specification. The exception would be
> > > in a table where there are so many characters it is not helpful. This seems
> > > like a case where the name would be better.
> 
> You didn't do the "Will change" on this one. I hope you decide to do it.

I can do this change, but I don't know what to name the character U+1366 (which is the suffix for all the ethiopic, and ethiopic-related alphabets).

> 
> > For your reference, the tables for hiragana-iroha, hiragana, katakana-iroha,
> > katakana are currently in in the tree.
> 
> And they are probably wrong? That is worth fixing, although naturally not in
> this patch.

Not sure what's wrong/missing, I don't know Japanese.

[Alexey Proskuryakov]

> I can do this change, but I don't know what to name the character U+1366 (which
> is the suffix for all the ethiopic, and ethiopic-related alphabets).

Its Unicode name is "ETHIOPIC PREFACE COLON".

[Daniel Bates]

Created attachment 46255 [details]
Patch with test case

This patch includes all the changes suggested by Darin except for alphabetizing the values in files css/CSSValueKeywords.in and rendering/style/RenderStyleConstants.h.

I did not alphabetize these lists because there is some subtlety to the current ordering with respect to how these values are used in the CSS parser. Among the possible changes that would need to be made if we reorganize these lists is line 2953 of CSSParser.cpp <http://trac.webkit.org/browser/trunk/WebCore/css/CSSParser.cpp?rev=52784#L2953>. In particular, it is unclear to me why we compute CSSValueDecimal - CSSValueDisc. The comment above the line states "// Make the list style default decimal", but I don't see how that corresponds to CSSValueDecimal - CSSValueDisc. Notice, from the generated file CSSValueKeywords.h, CSSValueDecimal - CSSValueDisc = 3. I'm not sure what to make of this.

I briefly looked at the method CSSPrimitiveValue::create(double value, UnitTypes type), but I'm unclear what the purpose of value is (with respect to the class in general - notice, it relates to m_value.num ). I mean, this method looks like it uses value to look up the appropriate cached CSSPrimitiveValue object (if one exists). Otherwise, it allocates a new CSSPrimitiveValue object with value (which sets m_value.num). But, it's not clear to me what value represents/means in general.

[Daniel Bates]

Comment on attachment 46255 [details]
Patch with test case

Marking for review.

I suggest we look to reorganize/alphabetize the list of list-style-types in css/CSSValueKeywords.in and rendering/style/RenderStyleConstants.h as part of a separate bug due to the intricacies of CSSParser.

[Darin Adler]

Comment on attachment 46255 [details]
Patch with test case

Thanks for your continued hard work on this.

> +/* _countof is only included in CE6; for CE5 we need to define it ourself */
> +#ifndef _countof
> +#define _countof(x) (sizeof(x) / sizeof((x)[0]))
> +#endif

This isn't quite right.

The code you moved is specific to Windows, and currently all WebKit uses of _countof are in Windows- or Windows-CE-specific source files. It defines the macro with the Windows name because it's intent is to smooth over differences between versions of Windows.

If we want our own count-of macro, we would not name it _countof. Using a leading underscore makes it non-portable in an undesirable way, since it could collide with macros and functions on other platforms.

The reason the macro is in Platform.h is that it's for smoothing over platform differences; even that is not quite correct. We do not want to put selected helpful macros into Platform.h -- that is not what the file is for. It's supposed to define platform settings. Macros like this one belong in files like StdLibExtras.h.

As a side note I also think we'd want to use a function template rather than a macro. I think we should define a WTF::arraySize function like this:

    namespace WTF {
        template <typename T, std::size_t size> inline std::size_t arraySize(T(&)[size]) { return size; }
    }

    using WTF::arraySize;

I'm not sure which WTF header to put it in. StdLibExtras.h is probably an acceptable place.

> +        case Afar:
> +            m_value.ident = CSSValueAfar;
> +            break;
> +        case EthiopicHalehameAaEt:

Could this switch statement at least be in alphabetical order. I understand the reason it was difficult to do that in some places, but I don't think those reasons apply here.

> Index: WebCore/inspector/front-end/CSSSourceSyntaxHighlighter.js
> ===================================================================
> --- WebCore/inspector/front-end/CSSSourceSyntaxHighlighter.js	(revision 53047)
> +++ WebCore/inspector/front-end/CSSSourceSyntaxHighlighter.js	(working copy)
> @@ -125,11 +125,17 @@ WebInspector.CSSSourceSyntaxHighlighter 
>          "source-atop", "destination-over", "destination-in", "destination-out", "destination-atop", "xor",
>          "plus-darker", "plus-lighter", "baseline", "middle", "sub", "super", "text-top", "text-bottom", "top",
>          "bottom", "-webkit-baseline-middle", "-webkit-auto", "left", "right", "center", "justify", "-webkit-left",
> -        "-webkit-right", "-webkit-center", "outside", "inside", "disc", "circle", "square", "decimal",
> -        "decimal-leading-zero", "lower-roman", "upper-roman", "lower-greek", "lower-alpha", "lower-latin",
> -        "upper-alpha", "upper-latin", "hebrew", "armenian", "georgian", "cjk-ideographic", "hiragana", "katakana",
> -        "hiragana-iroha", "katakana-iroha", "inline", "block", "list-item", "run-in", "compact", "inline-block",
> -        "table", "inline-table", "table-row-group", "table-header-group", "table-footer-group", "table-row",
> +        "-webkit-right", "-webkit-center", "outside", "inside", "afar", "amharic", "amharic-abegede", "armenian",
> +        "circle", "cjk-earthly-branch", "cjk-heavenly-stem", "cjk-ideographic", "decimal", "decimal-leading-zero",
> +        "disc", "ethiopic", "ethiopic-abegede", "ethiopic-abegede-am-et", "ethiopic-abegede-gez", "ethiopic-abegede-ti-er",
> +        "ethiopic-abegede-ti-et", "ethiopic-halehame-aa-er", "ethiopic-halehame-aa-et", "ethiopic-halehame-am-et",
> +        "ethiopic-halehame-gez", "ethiopic-halehame-om-et", "ethiopic-halehame-sid-et", "ethiopic-halehame-so-et",
> +        "ethiopic-halehame-ti-er", "ethiopic-halehame-ti-et", "ethiopic-halehame-tig", "georgian", "hangul",
> +        "hangul-consonant", "hebrew", "hiragana", "hiragana-iroha", "katakana", "katakana-iroha", "lower-alpha",
> +        "lower-greek", "lower-latin", "lower-norwegian", "lower-roman", "oromo", "sidama", "somali", "square", "tigre",
> +        "tigrinya-er", "tigrinya-er-abegede", "tigrinya-et", "tigrinya-et-abegede", "upper-alpha", "upper-greek",
> +        "upper-latin", "upper-norwegian", "upper-roman", "inline", "block", "list-item", "run-in", "compact",
> +        "inline-block", "table", "inline-table", "table-row-group", "table-header-group", "table-footer-group", "table-row",
>          "table-column-group", "table-column", "table-cell", "table-caption", "-webkit-box", "-webkit-inline-box",
>          "-wap-marquee", "auto", "crosshair", "default", "pointer", "move", "vertical-text", "cell", "context-menu",
>          "alias", "progress", "no-drop", "not-allowed", "-webkit-zoom-in", "-webkit-zoom-out", "e-resize", "ne-resize",

This seems like another place where we could safely use alphabetical order instead of random order.

> + * Copyright (C) 2009 Daniel Bates ([e-mail])

2010

> -            return toAlphabetic(value, lowerLatinAlphabet, 26);
> +            return toAlphabetic(value, lowerLatinAlphabet, _countof(lowerLatinAlphabet));

I think something like this will work:

    template <size_t size>
    static inline String toAlphabetic(int number, UChar(&)[size] alphabet)
    {
        return toAlphabetic(number, alphabet, size);
    }

Then you can just remove the counts entirely and let overloading handle the rest. If overloading doesn't work you can make this use a slightly different name than the underlying function that takes a pointer and size. The fact that the template is only for a small wrapper means we won't get any code bloat.

If this works, it means you won't need to add the arraySize function to WTF at all in this patch. That can be a separate thing.

>  enum EListStyleType {
> -     Disc, Circle, Square, DecimalListStyle, DecimalLeadingZero,
> -     LowerRoman, UpperRoman, LowerGreek,
> -     LowerAlpha, LowerLatin, UpperAlpha, UpperLatin,
> -     Hebrew, Armenian, Georgian, CJKIdeographic,
> -     Hiragana, Katakana, HiraganaIroha, KatakanaIroha, NoneListStyle
> +     Disc,
> +     Circle,
> +     Square,

I suggest adding a comment mentioning that the order of this enum has to match the order in that other file.

review- only because the countof_ thing is definitely wrong. Otherwise this looks very good.

[Daniel Bates]

Created attachment 46840 [details]
Patch with test case

Updated patch based on Darin's comments.

[Daniel Bates]

Comment on attachment 46840 [details]
Patch with test case

Clearing flags on attachment: 46840

Committed r53452: <http://trac.webkit.org/changeset/53452>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

Added expected results for Windows in <http://trac.webkit.org/changeset/53453>.