[webkit-reviews] review denied: [Bug 27749] StyleChange::init needs clean up before fixing the bug 23892 : [Attachment 34712] fixes the bug, a minor rebaseline is required

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 27749: StyleChange::init needs clean up before fixing the bug 23892
https://bugs.webkit.org/show_bug.cgi?id=27749

Attachment 34712: fixes the bug, a minor rebaseline is required
https://bugs.webkit.org/attachment.cgi?id=34712&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    void cleanupTextDecorationProperties(CSSMutableStyleDeclaration* style);


The verb "clean up" is two words, so this would be
"cleanUpTextDecorationProperties". But it's not clear what "clean up" means and
I'd like the function to have a better name. I think a better verb might be
"reconcile" or "standardize". This function needs a name that explains what it
does as clearly as possible.

The declaration of this function should not have a name for the argument
"style" since the type is unambiguous. Same for the other function added in
this patch.

> +    RefPtr<CSSValue> textDecorationsInEffect =
style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect);
> +    RefPtr<CSSValue> textDecoration =
style->getPropertyCSSValue(CSSPropertyTextDecoration);
> +    // We shouldn't have both text-decoration and
-webkit-text-decorations-in-effect because that wouldn't make sense.
> +    ASSERT(!textDecorationsInEffect || !textDecoration);

The textDecoration value fetched here is never used except in the assertion. It
would be slightly better not to even make the getPropertyCSSValue call in debug
builds.

> +    textDecoration = style->getPropertyCSSValue(CSSPropertyTextDecoration);
> +    if (textDecoration && !textDecoration->isValueList())
> +	   style->removeProperty(CSSPropertyTextDecoration);

I don't understand the purpose of removing the text-decoration property when
its value is not a value list. The code needs a comment explaining why this is
a good idea.

> +    if (equalIgnoringCase(style->getPropertyValue(CSSPropertyFontWeight),
"bold")) {

Even though the old code already did this, it would be more efficient to do
this check on CSSValue rather than constructing a string with the text form of
the value just so you can compare with a constant string. You can make a helper
function to help automate checking that it's a CSSPrimitiveValue, then calling
getIdent() and checking for the particular identifier value.

> +    if (equalIgnoringCase(style->getPropertyValue(CSSPropertyFontStyle),
"italic")
> +	   || equalIgnoringCase(style->getPropertyValue(CSSPropertyFontStyle),
"oblique")) {

Ditto.

> +    if (equalIgnoringCase(style->getPropertyValue(CSSPropertyVerticalAlign),
"sub")) {
> +	   style->removeProperty(CSSPropertyVerticalAlign);
> +	   m_applySubscript = true;
> +    } else if
(equalIgnoringCase(style->getPropertyValue(CSSPropertyVerticalAlign), "super"))
{
> +	   style->removeProperty(CSSPropertyVerticalAlign);
> +	   m_applySuperscript = true;
> +    }

Ditto. It would also be more efficient to not get the same property value
twice.

> +    if (RefPtr<CSSValue> colorValue =
style->getPropertyCSSValue(CSSPropertyColor)) {
> +	   RGBA32 rgba = 0;
> +	   CSSParser::parseColor(rgba, colorValue->cssText());
> +	   Color color(rgba);
> +	   m_applyFontColor = color.name();
> +	   style->removeProperty(CSSPropertyColor);
> +    }

Is there no more efficient way to find out what color is specified by a
CSSValue than to convert to a string and then parse the color? For example,
would the getRGBA32Value function cover enough cases? Or do we also have to
parse color strings?

> +    RefPtr<CSSValue> fontSize =
style->getPropertyCSSValue(CSSPropertyFontSize);
> +    if (fontSize) {
> +	   if (!fontSize->isPrimitiveValue())
> +	       style->removeProperty(CSSPropertyFontSize); // Can't make sense
of the number. Put no font size.
> +	   else {
> +	       CSSPrimitiveValue* value =
static_cast<CSSPrimitiveValue*>(fontSize.get());
>  
> +	       if (value->primitiveType() < CSSPrimitiveValue::CSS_PX ||
value->primitiveType() > CSSPrimitiveValue::CSS_PC)
> +		   ;// Size keyword or relative unit.

The formatting here is confusing. I suggest using braces instead of just a
semicolon. I also suggest a longer comment, and if it's two lines then it would
justify having braces! Understood that it's a size keyword or a relative unit,
but why are does this code do nothing in that case? Is it something we want to
handle later or is it already correct?

> +	   RefPtr<CSSValueList> resultValueList =
static_cast<CSSValueList*>(resultValue.get())->copy();
>	   CSSValueList* computedValueList =
static_cast<CSSValueList*>(computedValue.get());
> +	   for (size_t i = 0; i < computedValueList->length(); i++)
> +	       resultValueList->removeAll(computedValueList->item(i));
> +	   result->setProperty(textDecorationProperties[n],
resultValueList->cssText());

I don't understand why you're copying the CSSValueList here. If the list is
"live" and associated with the original property, then it seems that we could
just modify the list and we wouldn't need to call setProperty at all. If the
list is not live and is not associated with the property, then it seems we
could modify it without making a copy.

I'm going to say review- because there are a few items above that might inspire
code changes. Generally the patch seems quite good.