[webkit-reviews] review denied: [Bug 130657] Link underline thickness should be uniform : [Attachment 229205] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jeongeun Kim
<je_julie.kim at samsung.com>'s request for review:
Bug 130657: Link underline thickness should be uniform
https://bugs.webkit.org/show_bug.cgi?id=130657

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

------- Additional Comments from Darin Adler <darin at apple.com>
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.