[webkit-reviews] review granted: [Bug 125718] Faster implementation of text-decoration-skip: ink : [Attachment 219378] 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 125718: Faster implementation of text-decoration-skip: ink
https://bugs.webkit.org/show_bug.cgi?id=125718

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

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


> Source/WebCore/platform/graphics/Font.h:28
> +#include "DashArray.h"

We don’t need to include the DashArray header just to declare a function with a
DashArray return type. That can and should be done by just adding a forward
declaration to this header.

> Source/WebCore/platform/graphics/Font.h:106
> +#if PLATFORM(MAC)
> +    DashArray intersectionPoints(const TextRun&, const FloatPoint&
textOrigin, int from, int to, const FloatRect& lineExtents) const;
> +#endif

I’m not sure everyone else agrees with me, but I prefer to have functions like
this be unconditional in the header even when the implementation is not always
there. I think it’s tidier to not pollute the header with the conditionals; it
does no harm to compile declarations of functions that aren’t defined on a
particular platform as long as we don’t try to call the function on platforms
where it’s not implemented, the interface itself is platform-independent and we
don’t bother compiling an empty function body somewhere.

> Source/WebCore/rendering/InlineTextBox.cpp:132
> +    DashArray intersections;
> +#if PLATFORM(MAC)
> +    intersections = textPainter.intersectionPoints(underlineBoundingBox,
m_start, m_start + m_len);
> +#endif
> +    DashArray a =
translateIntersectionPointsToSkipInkBoundaries(intersections,
underlineBoundingBox.height(), width);

I don’t understand this conditional. Is there a point in compiling the rest of
this function if we can’t compute intersection points? Does this function do
any good without the intersectionPoints function? If not, then I suggest we put
this feature under a more appropriate #if rather than just
ENABLE(CSS3_TEXT_DECORATION).

The change log sheds no light on this point.

Generally it’s mixed-up to have the functions needed for “skip ink” to be
partly under PLATFORM(MAC) #if and partly under ENABLE(CSS3_TEXT_DECORATION)
#if. We should think this through and do something as logical as possible, and
as little PLATFORM(MAC)-ish as possible. For example, maybe this should be
under a ENABLE(CSS3_TEXT_DECORATION_SKIP_INK) conditional. I’m not saying that
conditional needs to be separately configurable on the command line, but rather
that we find one place to set it, and then use it elsewhere rather than
literally writing PLATFORM(MAC) at each conditional site.

> Source/WebCore/rendering/TextPainter.h:27
> +#include "DashArray.h"

Same thing about not needing an include just to declare a function with a
return value.

> Source/WebCore/rendering/TextPainter.h:71
> +#if PLATFORM(MAC)

Same thing about not needing a conditional where a function is declared.