[webkit-reviews] review granted: [Bug 136612] REGRESSION (r172153): Text drawn with wrong color when second text shadow has zero offset and blur (breaks buttons at aws.amazon.com) : [Attachment 237805] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 136612: REGRESSION (r172153): Text drawn with wrong color when second text
shadow has zero offset and blur (breaks buttons at aws.amazon.com)
https://bugs.webkit.org/show_bug.cgi?id=136612

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

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


> Source/WebCore/rendering/TextPainter.cpp:68
>  static bool isEmptyShadow(const ShadowData* shadow)
>  {
>      if (!shadow)
> -	   return true;
> +	   return false;
>      return shadow->location() == IntPoint() && !shadow->radius();
>  }

I think it’s confusing to change the behavior of this function without changing
its name. Maybe it should take a reference and the null check could be at the
call site. Or maybe we can just call this a pure bug fix and say that the old
behavior was a bug.

> Source/WebCore/rendering/TextPainter.cpp:86
> +	   if (isEmptyShadow(shadow)) {
> +	       shadow = shadow->next();
> +	       continue;
> +	   }

Should put this before the definitions of extraOffset and didSaveContext.

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:598
> +	   bool didSaveContext = false;

You forgot to change the code below to use this boolean! It still says “if
(shadow->next())” but should say “if (didSaveContext)”.