RESOLVED FIXED 40665
Provide floating-point support for text selection framework
https://bugs.webkit.org/show_bug.cgi?id=40665
Summary Provide floating-point support for text selection framework
Nikolas Zimmermann
Reported 2010-06-16 02:23:13 PDT
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.
Attachments
Initial patch (28.48 KB, patch)
2010-06-16 04:36 PDT, Nikolas Zimmermann
krit: review+
Follow-up patch (5.00 KB, patch)
2010-06-18 23:26 PDT, Nikolas Zimmermann
no flags
Follow-up patch v2 (5.04 KB, patch)
2010-06-19 01:20 PDT, Nikolas Zimmermann
mitz: review-
Follow-up patch v3 (4.88 KB, patch)
2010-06-22 08:05 PDT, Nikolas Zimmermann
krit: review+
Nikolas Zimmermann
Comment 1 2010-06-16 04:36:36 PDT
Created attachment 58877 [details] Initial patch
Dirk Schulze
Comment 2 2010-06-16 06:49:46 PDT
Comment on attachment 58877 [details] Initial patch r=me
Nikolas Zimmermann
Comment 3 2010-06-16 06:52:58 PDT
Landed in r61253.
mitz
Comment 4 2010-06-16 08:18:35 PDT
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
Comment 5 2010-06-16 08:57:35 PDT
(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?
mitz
Comment 6 2010-06-16 09:10:48 PDT
(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
Comment 7 2010-06-18 23:26:34 PDT
Created attachment 59177 [details] Follow-up patch Fix issues Dan reported.
Nikolas Zimmermann
Comment 8 2010-06-19 01:20:42 PDT
Created attachment 59180 [details] Follow-up patch v2 Dirk pointed out that I forgot to rename glyphScale to glyphStretch. Fixed.
mitz
Comment 9 2010-06-20 11:13:18 PDT
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
Comment 10 2010-06-22 06:58:34 PDT
(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
Comment 11 2010-06-22 08:04:24 PDT
(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
Comment 12 2010-06-22 08:05:14 PDT
Created attachment 59367 [details] Follow-up patch v3
Dirk Schulze
Comment 13 2010-06-29 00:08:17 PDT
Comment on attachment 59367 [details] Follow-up patch v3 Even if horizontalGlyphStretch is a bit long for my taste, looks ok. :-)
Note You need to log in before you can comment on or make changes to this bug.