[webkit-reviews] review denied: [Bug 122097] ASSERTION FAILED: !style->propertyIsImportant(propertyID) in WebCore::setTextDecorationProperty : [Attachment 214148] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Santosh Mahto
<santosh.ma at samsung.com>'s request for review:
Bug 122097: ASSERTION FAILED: !style->propertyIsImportant(propertyID) in
WebCore::setTextDecorationProperty
https://bugs.webkit.org/show_bug.cgi?id=122097

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=214148&action=review


> Source/WebCore/ChangeLog:25
> +	   (WebCore::StyleChange::extractTextStyles): Remove property even if 
> +	   it is declared !important.

Why?

> Source/WebCore/editing/EditingStyle.cpp:1446
> -	   setTextDecorationProperty(style, newTextDecoration.get(),
CSSPropertyTextDecoration);
> +	   if (newTextDecoration->length())
> +	       setTextDecorationProperty(style, newTextDecoration.get(),
CSSPropertyTextDecoration);
> +	   else
> +	       style->removeProperty(CSSPropertyTextDecoration);
> +

This doesn't make any sense. The whole point of setTextDecorationProperty is to
take care of that.
We should figure out why setTextDecorationProperty has the assertion instead of
working around it here.

> LayoutTests/editing/execCommand/remove-format-textdecoration-in-iframe.html:6

> +    <iframe onload=" {     document.designMode=&apos;on&apos;;
> +		      document.execCommand(&apos;selectall&apos;);
> +		      document.execCommand(&apos;RemoveFormat&apos;);	   } ">


Please fix the indentation.
Also, we should use dump-as-text or dump-as-markup.
There is no reason for this test to be a render tree dump.
r- because of this.