[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
bugzilla-daemon at webkit.org
Thu Jan 26 09:40:49 PST 2012
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.
More information about the webkit-reviews
mailing list