[webkit-reviews] review granted: [Bug 83026] REGRESSION(r112177): listStyleType CSS property gets converted into listStyle : [Attachment 138692] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 83026: REGRESSION(r112177): listStyleType CSS property gets converted into
listStyle
https://bugs.webkit.org/show_bug.cgi?id=83026

Attachment 138692: Patch
https://bugs.webkit.org/attachment.cgi?id=138692&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=138692&action=review


> Source/WebCore/css/StylePropertySet.cpp:258
> +    if (!top || !top->value() || !right || !right->value() || !bottom ||
!bottom->value() || !left || !left->value()
> +	   || top->value()->isInitialValue() ||
right->value()->isInitialValue() || bottom->value()->isInitialValue() ||
bottom->value()->isInitialValue()
> +	   || top->isImportant() != right->isImportant() ||
right->isImportant() != bottom->isImportant()
> +	   || bottom->isImportant() != left->isImportant())
>	   return String();

This if statement is hard to read.

How about breaking it up into three separate if statements?

> Source/WebCore/css/StylePropertySet.cpp:398
> +    bool isImportant = false;

Maybe name this lastPropertyWasImportant?

> Source/WebCore/css/StylePropertySet.cpp:420
> +enum CommonValueMode { OmitShorthandWithUncommonValues,
ReturnNullOnUncommonValues };

This enum should not be here. We defined it in the header.

> Source/WebCore/css/StylePropertySet.cpp:425
> +    String result;

String is not a good way to build up a result; we’ll have to re-allocate 5
times. Instead we should use StringBuilder. I know the code you moved here was
using String.

> Source/WebCore/css/StylePropertySet.h:126
> +    enum CommonValueMode { OmitShorthandWithUncommonValues,
ReturnNullOnUncommonValues };

Wouldn’t the name of the first just be OmitUncommonValues? What is a “common
value” anyway?