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

[bugzilla-daemon at webkit.org]

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





--- Comment #34 from Myles C. Maxfield <mmaxfield at apple.com>  2014-05-20 17:36:25 PST ---
(From update of attachment 230169)
View in context: https://bugs.webkit.org/attachment.cgi?id=230169&action=review

> Source/WebCore/rendering/InlineTextBox.cpp:1038
> +    getThicknessForDecoration(isFirstLine(), firstLineStyle, style, thickness, textDecorationThickness);

This code, too, has been refactored. Make sure that visual overflow bounds are updated according to this (possibly thicker) underline.

> Source/WebCore/rendering/InlineTextBox.cpp:1039
> +    float wavyOffset = 2.f;

I have changed this. Please update your source tree.

> Source/WebCore/rendering/InlineTextBox.cpp:-1066
> -        float wavyOffset = 2.f;

You should update your source tree; I have changed this code recently.

> Source/WebCore/rendering/RenderObject.cpp:2127
> +    for (const auto& renderStyle : style) {

Shouldn't this be auto* instead?

In addition, the ":" syntax uses iterators under the hood. However, the style guide says not to use iterators on vectors. You might want to ask someone else on the team about whether or not this style is allowed on Vectors.

> Source/WebCore/rendering/RenderObject.cpp:2128
> +        index = nextIndex;

If you're going to calculate a nextIndex anyway, might as well use a regular for loop (without ":")

> Source/WebCore/rendering/RenderObject.cpp:2131
> +        RenderStyle* baseStyle = (isFirstLine && firstLineStyle->at(index)) ? firstLineStyle->at(index) : renderStyle;

Might want to put firstLineStyle->at(index) into a reference so you don't have to call it twice

> Source/WebCore/rendering/RenderObject.cpp:2152
> +    style.fill(nullptr, TextDecorationRenderStyleEnd);

Rather than do this on the heap, try to do this on the stack (avoiding a malloc()). This is constant size, after all.

It seems like relying on the TextDecorationRenderStyleXXXX values being contiguous and 0-indexed is brittle.

> Source/WebCore/rendering/RenderObject.cpp:2155
>          styleToUse = firstlineStyle ? &curr->firstLineStyle() : &curr->style();

I believe there is a lineStyle() function that does this for you, though I may be mistaken.

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