[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