[webkit-reviews] review denied: [Bug 40665] Provide floating-point support for text selection framework : [Attachment 59180] Follow-up patch v2

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Nikolas Zimmermann <zimmermann at kde.org>'s request
for review:
Bug 40665: Provide floating-point support for text selection framework
https://bugs.webkit.org/show_bug.cgi?id=40665

Attachment 59180: Follow-up patch v2
https://bugs.webkit.org/attachment.cgi?id=59180&action=review

------- Additional Comments from mitz at webkit.org
> +	   Rename 'glyphScale' to 'horizontalGlyphStretch' upon Dan Bernsteins
request.

Either too much last name or too little apostrophe.

> +    bool applyHorizontalGlyphStretching() const { return
m_horizontalGlyphStretch != 1; }

This name reads like a verb, as if calling this method applies horizontal glyph
stretching. Since it’s a predicate, it should be named
“hasHorizontalGlyphStretch” or “shouldStretchGlyphsHorizontally”.

> +#if ENABLE(SVG)
> +	       // SVG uses horizontalGlyphStretch(), when textLength is used to
stretch/squeeze text.
> +	       if (m_run.applyHorizontalGlyphStretching())
> +		   width *= m_run.horizontalGlyphStretch();
> +#endif

Do you know if the extra branch is cheaper than just multiplying by 1
unconditionally?

Also, it’s till not clear to me why this has to be applied to each individual
glyph and not to the entire run. Can you explain this? Thanks!