[webkit-reviews] review denied: [Bug 36840] SVG @font-face breaks text-overflow: ellipsis : [Attachment 80535] Patch (revised), with layout test

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Emil Schutte
<emilschutte at gmail.com>'s request for review:
Bug 36840: SVG @font-face breaks text-overflow: ellipsis
https://bugs.webkit.org/show_bug.cgi?id=36840

Attachment 80535: Patch (revised), with layout test
https://bugs.webkit.org/attachment.cgi?id=80535&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=80535&action=review

Great that you're working on that. Just some notes. Is it possiblw to add a
selection test? IIRC we have such a general selection test in svg/W3C... maybe
it is possible to adapt this test to simulate the selection with such a test.
Doesn't the event sender support mouse control? So you may can simulate it with
the event sender.

> Source/WebCore/ChangeLog:6
> +	   Implement offsetForPositionForTextUsingSVGFont for SVG fonts,
enabling per-character SVG text selection
> +	   and fixing text-overflow: ellipsis.

You should add a more detailed instruction what you changed, how it works.

> Source/WebCore/svg/SVGFont.cpp:579
> +    int boundedFrom = std::max(0, std::min(len, from));
> +    int boundedTo = std::max(0, std::min(len, to));

Add using std; at the  beginning please.

> Source/WebCore/svg/SVGFont.cpp:644
> +    data.at = from;
> +    data.from = from;

from is still 0 here. I'd initialize the int before the first real use.

> Source/WebCore/svg/SVGFont.cpp:648
> +    data.scale = convertEmUnitToPixel(this->size(),
fontFaceElement->unitsPerEm(), 1.0f);

s/1.0f/1/

> Source/WebCore/svg/SVGFont.cpp:649
> +    data.length = 0.0f;

Just 0

> LayoutTests/fast/css/resources/CartoGothicStd-Bold-webfont.svg:3
> +<?xml version="1.0" standalone="no"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN"
"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" >
> +<svg xmlns="http://www.w3.org/2000/svg">

We have multiple SVGFonts on different places, also see LayoutTests/svg/. Do we
need this font? Can't we just use one of the fonts of svg?