[webkit-reviews] review denied: [Bug 36734] Implement symbolic CSS3 list-style-types : [Attachment 51965] Patch with test case

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Mar 29 17:51:31 PDT 2010


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

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

------- Additional Comments from Darin Adler <darin at apple.com>
> -	   isNegativeNumber = true;
> +	   isNegativeNumber = type == SymbolicSequence ? false : true; //
Symbolics cannot be negative.

For clarity I suggest writing either this:

    isNegativeNumber = type != SymbolicSequence; // Symbolics cannot be
negative.

Or this:

    if (type != SymbolicSequence)
	isNegativeNumber = true; // Symbolics cannot be negative.

It seems to me that the ?: expression is too oblique. Especially "? false :
true", which is just a sideways way to write "!".

Another way to do this is to let isNegativeNumber get set, and then change it
below to:

    if (isNegativeNumber && type != SymbolicSequence)
	letters[lettersSize - ++length] = hyphenMinus;

Because symbolics can indeed be negative. They just don't show a minus sign
when they are.

> +	   unsigned numSymbols = ceil((double) (numberShadow + 1) /
sequenceSize) - 1;

There is no need to involve floating point. The following expression gives the
same result for all the positive numbers I tried:

    unsigned numSymbols = numberShadow / sequenceSize;

You should use that instead unless there's a reason not to.

> +	       width = m_text.isEmpty() ? 0 : font.width(m_text); // no suffix
for these types

I don't think you need a special case here for the empty string.

> +	   case Asterisks:
> +	   case Footnotes: {
> +	       if (m_text.isEmpty())
> +		   return IntRect();
> +	       const Font& font = style()->font();
> +	       return IntRect(x(), y() + font.ascent(), font.width(m_text),
font.height());
> +	   }

I'm not sure you need it here either.

review- for the gratuitous floating point


More information about the webkit-reviews mailing list