[webkit-reviews] review granted: [Bug 237175] [css] text-decoration is not implemented as a shorthand of its longhands : [Attachment 453413] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Oriol Brufau <obrufau at igalia.com>'s
request for review:
Bug 237175: [css] text-decoration is not implemented as a shorthand of its
longhands
https://bugs.webkit.org/show_bug.cgi?id=237175

Attachment 453413: Patch

https://bugs.webkit.org/attachment.cgi?id=453413&action=review




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 453413
  --> https://bugs.webkit.org/attachment.cgi?id=453413
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453413&action=review

This looks good to me overall. Could use better test coverage for some of the
new behaviors and at least one function has gotten noticeably less clear.

> Source/WebCore/ChangeLog:28
> +	    - Use 'text-decoration-line' get getting values.

get getting -> when getting

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3524
> +		   return nullptr;

Is it really OK to just return null in cases like this? Can we add test
coverage for this so we can compare behavior with other browser engines for
interoperability?

> Source/WebCore/css/StyleProperties.cpp:286
> +		       return String();

What is the impact of returning null string in cases like this? Again can we
create test coverage?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:6288
> +	   RefPtr<CSSValue> line = consumeTextDecorationLine(m_range);

auto

> Source/WebCore/editing/EditingStyle.cpp:602
> +	   style->setProperty(CSSPropertyTextDecoration, valueList->cssText());

This seems really unfortunate, to be serializing snd then parsing the text
form. The rest of this code goes out of its way to use the object model, not
text. Presumably for multiple reasons including performance.

> Source/WebCore/editing/EditingStyle.cpp:604
> +	   style->setProperty(CSSPropertyTextDecoration, "none");

Ditto.

> Source/WebCore/editing/EditingStyle.cpp:903
> +		       newInlineStyle->setProperty(CSSPropertyTextDecoration,
newValueList->cssText());

Again.

> Source/WebCore/editing/EditingStyle.cpp:1467
>  void EditingStyle::removeEquivalentProperties(T& style)

Disappointing how much more complex the algorithm has become. Worth considering
refactoring further to make it easier to understand.

> Source/WebCore/editing/markup.cpp:939
> +			  
fullySelectedRootStyle->style()->setProperty(CSSPropertyTextDecoration,
"none");

Again.