[webkit-reviews] review denied: [Bug 33089] Implement alphabetic CSS3 list-style-types : [Attachment 46255] 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 33089: Implement alphabetic CSS3 list-style-types
https://bugs.webkit.org/show_bug.cgi?id=33089

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

------- Additional Comments from Darin Adler <darin at apple.com>
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 (dbates at intudata.com)

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.