[webkit-reviews] review granted: [Bug 77095] Refactor application of additional style attributes for table elements. : [Attachment 124119] Probably a patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <kling at webkit.org>'s
request for review:
Bug 77095: Refactor application of additional style attributes for table
elements.
https://bugs.webkit.org/show_bug.cgi?id=77095

Attachment 124119: Probably a patch
https://bugs.webkit.org/attachment.cgi?id=124119&action=review

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


> Source/WebCore/html/HTMLTableElement.cpp:472
> +    if (m_borderColorAttr) {
> +	   DEFINE_STATIC_LOCAL(RefPtr<CSSMutableStyleDeclaration>, style, ());
> +	   if (!style) {
> +	       style = CSSMutableStyleDeclaration::create();
> +	       style->setProperty(CSSPropertyBorderTopStyle, CSSValueSolid);
> +	       style->setProperty(CSSPropertyBorderBottomStyle, CSSValueSolid);

> +	       style->setProperty(CSSPropertyBorderLeftStyle, CSSValueSolid);
> +	       style->setProperty(CSSPropertyBorderRightStyle, CSSValueSolid);
> +	   }
> +	   return style;
> +    } else {
> +	   DEFINE_STATIC_LOCAL(RefPtr<CSSMutableStyleDeclaration>, style, ());
> +	   if (!style) {
> +	       style = CSSMutableStyleDeclaration::create();
> +	       style->setProperty(CSSPropertyBorderTopStyle, CSSValueOutset);
> +	       style->setProperty(CSSPropertyBorderBottomStyle,
CSSValueOutset);
> +	       style->setProperty(CSSPropertyBorderLeftStyle, CSSValueOutset);
> +	       style->setProperty(CSSPropertyBorderRightStyle, CSSValueOutset);

> +	   }
> +	   return style;

We can make this a lot cleaner with a helper function:

    static CSSMutableStyleDeclaration* leakBorderStyle(int value)
    {
	RefPtr<CSSMutableStyleDeclaration> style =
CSSMutableStyleDeclaration::create();
	style->setProperty(CSSPropertyBorderTopStyle, CSSValueOutset);
	style->setProperty(CSSPropertyBorderBottomStyle, CSSValueOutset);
	style->setProperty(CSSPropertyBorderLeftStyle, CSSValueOutset);
	style->setProperty(CSSPropertyBorderRightStyle, CSSValueOutset);
	return style.release().leakRef();
    }

    if (m_borderColorAttr) {
	static CSSMutableStyleDeclaration* solidBorderStyle =
leakBorderStyle(CSSValueSolid);
	return solidBorderStyle;
    }
    static CSSMutableStyleDeclaration* outsetBorderStyle =
leakBorderStyle(CSSValueOutset);
    return outsetBorderStyle;

> Source/WebCore/html/HTMLTableElement.cpp:504
> +PassRefPtr<CSSMutableStyleDeclaration>
HTMLTableElement::additionalCellStyle()
> +{
> +    if (m_sharedCellStyle)
> +	   return m_sharedCellStyle;
> +
> +    m_sharedCellStyle = CSSMutableStyleDeclaration::create();

I think it’s better to put the management of the data member into a separate
function from the one with the case statement that creates the style. The
function could even be inlined; the out of line part would be the “create the
declaration” bit.

> Source/WebCore/html/HTMLTableElement.cpp:548
> +    if (m_padding) {
> +	   String value = String::number(m_padding);
> +	   m_sharedCellStyle->setProperty(CSSPropertyPaddingTop, value);
> +	   m_sharedCellStyle->setProperty(CSSPropertyPaddingBottom, value);
> +	   m_sharedCellStyle->setProperty(CSSPropertyPaddingLeft, value);
> +	   m_sharedCellStyle->setProperty(CSSPropertyPaddingRight, value);
>      }

I notice we’re not adding “px” here. Is that OK?

> Source/WebCore/html/HTMLTableElement.cpp:578
> +    if (rows) {
> +	   DEFINE_STATIC_LOCAL(RefPtr<CSSMutableStyleDeclaration>, style, ());
> +	   if (!style) {
> +	       style = CSSMutableStyleDeclaration::create();
> +	       style->setProperty(CSSPropertyBorderTopWidth, CSSValueThin);
> +	       style->setProperty(CSSPropertyBorderBottomWidth, CSSValueThin);
> +	       style->setProperty(CSSPropertyBorderTopStyle, CSSValueSolid);
> +	       style->setProperty(CSSPropertyBorderBottomStyle, CSSValueSolid);

>	   }
> -
> -	   setMappedAttributeDecl(ePersistent, rulesAttr, rulesValue, decl);
> -	   decl->setMappedState(ePersistent, rulesAttr, rulesValue);
> +	   return style;
> +    } else {
> +	   DEFINE_STATIC_LOCAL(RefPtr<CSSMutableStyleDeclaration>, style, ());
> +	   if (!style) {
> +	       style = CSSMutableStyleDeclaration::create();
> +	       style->setProperty(CSSPropertyBorderLeftWidth, CSSValueThin);
> +	       style->setProperty(CSSPropertyBorderRightWidth, CSSValueThin);
> +	       style->setProperty(CSSPropertyBorderLeftStyle, CSSValueSolid);
> +	       style->setProperty(CSSPropertyBorderRightStyle, CSSValueSolid);
> +	   }
> +	   return style;
>      }

Again, I suggest using helper functions so we don’t need the “if (!style)”
here.