[Webkit-unassigned] [Bug 130657] Link underline thickness should be uniform

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=130657


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #229205|review?                     |review-
               Flag|                            |




--- Comment #18 from Darin Adler <darin at apple.com>  2014-04-12 13:15:53 PST ---
(From update of attachment 229205)
View in context: https://bugs.webkit.org/attachment.cgi?id=229205&action=review

A bug fix requires a regression test. We need a test that shows what was wrong before and the fact that it’s now right. review- because of the lack of test or explanation of why a test is lacking

> Source/WebCore/rendering/InlineTextBox.cpp:1011
> +    RenderObject* decorationBox = renderer().getTextDecorationColors(decoration, underline, overline, linethrough, true);

It is wrong to name this “decorationBox” when the return value will not necessarily be a box.

> Source/WebCore/rendering/InlineTextBox.cpp:1013
> +        decorationBox = renderer().getTextDecorationColors(decoration, underline, overline, linethrough, true, true);

I’m not sure it’s right to overwrite the value we got from the first call with the value from the second call. We need test cases to demonstrate this is good behavior.

> Source/WebCore/rendering/RenderObject.cpp:2146
> +RenderObject* RenderObject::getTextDecorationColors(int decorations, Color& underline, Color& overline,
>                                             Color& linethrough, bool quirksMode, bool firstlineStyle)

Should merge this into one line, rather than just messing up the indentation. (The person who original wrote this shouldn’t have lined it up in a way that would get messed up if we changed the return value.)

> Source/WebCore/rendering/RenderObject.h:719
> -    void getTextDecorationColors(int decorations, Color& underline, Color& overline, Color& linethrough, bool quirksMode = false, bool firstlineStyle = false);
> +    RenderObject* getTextDecorationColors(int decorations, Color& underline, Color& overline, Color& linethrough, bool quirksMode = false, bool firstlineStyle = false);

This function now needs a comment to explain the otherwise-mysterious return value.

I think it would be better to have the function return a RenderStyle& rather than a RenderObject* since all the caller does is get at the style, and the function inside is already coming up with a style. I’m also not sure if there’s ever a good reason to return null for the style. Worst case we return the style for the this object itself.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.