[webkit-reviews] review denied: [Bug 99295] removeAttribute('style') not working in certain circumstances : [Attachment 168872] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Takashi Sakamoto <tasak at google.com>'s
request for review:
Bug 99295: removeAttribute('style') not working in certain circumstances
https://bugs.webkit.org/show_bug.cgi?id=99295

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

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


> Source/WebCore/ChangeLog:10
> +	   When Element::removeAttribute is invoked, check whether the given
name
> +	   is 'style' or not. If the element has no style attribute and style
is
> +	   given, reset the element's style by using removeBlockProperties.

This is not a good comment. It repeats what the code does in English, but
doesn’t answer the question why this is the right thing to do.

> Source/WebCore/ChangeLog:18
> +	   If 'style' is given but the element has no style attribute, invoke
> +	   removeBlockProperties to reset the style. After the reset, invoke
> +	   didRemoveAttribute to notify that the element's style is updated.

And this is just that same text again. The change log needs to say why this is
being done, not just restate what’s in the code.

> Source/WebCore/dom/Element.cpp:1578
> +	   if (localName == styleAttr && style() && style()->length() > 0) {
> +	       style()->makeMutable()->removeBlockProperties();
> +	       didRemoveAttribute(QualifiedName(nullAtom, localName,
nullAtom));
> +	   }

All the other code to special case the style attribute is in the StyledElement
class. It’s not appropriate to put this mysterious code here in the Element
class; the code to handle removal belongs in the StyledElement class with the
rest of the code.

I suggest investigating whether this can be fixed in the
StyledElement::attributeChanged function instead of here. I’m pretty sure it
can be.

It’s incorrect for performance reasons to call style(), which will force a
CSSStyleDeclaration to be created. The work should be done directly on the
StylePropertySet. The function inlineStyle() should be called, and checked for
null. Then if the style is non-empty, we should remove all the properties and
call inlineStyleChanged.

It’s incorrect to call removeBlockProperties. We need to remove all the
properties, not just the block properties. We may need to add a function to
StylePropertySet that removes all properties.