[webkit-reviews] review granted: [Bug 86517] text-decoration should not be propagated through absolutely positioned elements to <a> tags : [Attachment 144699] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Shane Stephens
<shanestephens at google.com>'s request for review:
Bug 86517: text-decoration should not be propagated through absolutely
positioned elements to <a> tags
https://bugs.webkit.org/show_bug.cgi?id=86517

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

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


> Source/WebCore/css/StyleResolver.cpp:2067
>      // Finally update our text decorations in effect, but don't allow
text-decoration to percolate through
>      // tables, inline blocks, inline tables, run-ins, or shadow DOM.

The comment no longer matches the code below. That needs to be fixed.

A situation like this shows why WebKit practice is to not write a comment that
says the same thing as the code below it. If the comment was instead about why
we don’t want text decoration to propagate, it would be more likely to still be
correct. A comment that says the same thing as the code has to be updated every
time the code is updated.

> Source/WebCore/css/StyleResolver.cpp:2070
>      if (style->display() == TABLE || style->display() == INLINE_TABLE ||
style->display() == RUN_IN
> -	   || style->display() == INLINE_BLOCK || style->display() ==
INLINE_BOX || isAtShadowBoundary(e))
> +	   || style->display() == INLINE_BLOCK || style->display() ==
INLINE_BOX || isAtShadowBoundary(e)
> +	   || style->isFloating() || style->isPositioned())

With this expression getting more and more complicated, it seems clear we
should factor this predicate out into a function. The name of that function
would help document what’s going on here, and the body of the function might be
a cleaner place to write comments about why we need to not do this for various
cases.