Bug 40665 – Provide floating-point support for text selection framework

• Component: Platform
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Font::selectionRectForText() / Font::offsetForPosition() take integer parameters, limiting text selection to integer precision.
offsetForPosition() already operates using floats internally, and just converts the incoming integer to a float - fix that, by allowing to pass in a float directly.
selectionRectForText() returns a FloatRect, but takes an IntPoint parameter for the origin - that is wrong.

This is a preparation for the SVG text rewrite.

[Nikolas Zimmermann]

Created attachment 58877 [details]
Initial patch

[Dirk Schulze]

Comment on attachment 58877 [details]
Initial patch

r=me

[Nikolas Zimmermann]

Landed in r61253.

Comment on attachment 58877 [details]
Initial patch

> +        , m_glyphScale(1.0f)

Please use “1” whenever you used “1.0f”. See <http://webkit.org/coding/coding-style.html>. Why isn’t the definition, initialization, and use of m_glyphScale surrounded by #if ENABLE(SVG)? What is the purpose of this new member variable?

[Nikolas Zimmermann]

(In reply to comment #4)
> (From update of attachment 58877 [details])
> > +        , m_glyphScale(1.0f)
> 
> Please use “1” whenever you used “1.0f”. See <http://webkit.org/coding/coding-style.html>.
Oh forgot about the new rule! Will fix in a follow up commit.

> Why isn’t the definition, initialization, and use of m_glyphScale surrounded by #if ENABLE(SVG)?
I can include that as well in a follow-up commit.

> What is the purpose of this new member variable?
SVG text has been rewritten to use offsetForPosition() (patch still to be uploaded). When we're rendering stretched/squeezed text (lengthAdjust="spacingAndGlyphs", textLength="150"), this variable will contain the textLength / computedTextLength scale factor. This way offsetForPosition() still returns the correct offset while selecting adjusted text. I have numerous new testcases for this, it works just well. This was the less intrusive approach, to teach WidthIterator about this scale factor.

Shall I upload a fix patch here, would you be willing to review?

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 58877 [details] [details])
> > > +        , m_glyphScale(1.0f)
> > 
> > Please use “1” whenever you used “1.0f”. See <http://webkit.org/coding/coding-style.html>.
> Oh forgot about the new rule! Will fix in a follow up commit.
> 
> > Why isn’t the definition, initialization, and use of m_glyphScale surrounded by #if ENABLE(SVG)?
> I can include that as well in a follow-up commit.

Thanks in advance for doing so!

> > What is the purpose of this new member variable?
> SVG text has been rewritten to use offsetForPosition() (patch still to be uploaded). When we're rendering stretched/squeezed text (lengthAdjust="spacingAndGlyphs", textLength="150"), this variable will contain the textLength / computedTextLength scale factor. This way offsetForPosition() still returns the correct offset while selecting adjusted text. I have numerous new testcases for this, it works just well. This was the less intrusive approach, to teach WidthIterator about this scale factor.

Is it necessary and correct to adjust the width of each individual glyph in WidthIterator? My concern is of course the cost of the extra branch impose on all non-SVG text. I also wonder if there could be a better name for this member variable and its accessors. Perhaps something to the effect that it’s applied to the horizontal axis only. Even replacing “scale” with “stretch” may help to convey this.

> Shall I upload a fix patch here, would you be willing to review?

Sure.

[Nikolas Zimmermann]

Created attachment 59177 [details]
Follow-up patch

Fix issues Dan reported.

[Nikolas Zimmermann]

Created attachment 59180 [details]
Follow-up patch v2

Dirk pointed out that I forgot to rename glyphScale to glyphStretch. Fixed.

Comment on attachment 59180 [details]
Follow-up patch v2

> +        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!

[Nikolas Zimmermann]

(In reply to comment #9)
> (From update of attachment 59180 [details])
> > +        Rename 'glyphScale' to 'horizontalGlyphStretch' upon Dan Bernsteins request.
> 
> Either too much last name or too little apostrophe.
Rephrased, "upon Dans' request" ;-)
 
> > +    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”.
Will remove it, because your suggestion below is just fine.

> 
> > +#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!
You're probably right, I could just fixup m_runWidthSoFar. Will try and upload a new patch soon.

[Nikolas Zimmermann]

(In reply to comment #10)
> > 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!
> You're probably right, I could just fixup m_runWidthSoFar. Will try and upload a new patch soon.

Okay, this doesn't work.

WidthIterator::advance() stores "float runWidthSoFar = m_runWidthSoFar;" in a local variable, and adds "runWidthSoFar += width;" (pseudo code) in the actual loop. At the end it sets m_runWidthSoFar = runWidthSoFar;

That means I'm forced to apply the scale to the "width" variable, as applying it to runWidthSoFar would reapply it to the already fixed up run width, that is set in m_runWidthSoFar after the first call to advance().

Uploading a new patch soon.

[Nikolas Zimmermann]

Created attachment 59367 [details]
Follow-up patch v3

[Dirk Schulze]

Comment on attachment 59367 [details]
Follow-up patch v3

Even if horizontalGlyphStretch is a bit long for my taste, looks ok. :-)