Bug 40665 - Provide floating-point support for text selection framework
Summary: Provide floating-point support for text selection framework
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 40663 40673
  Show dependency treegraph
 
Reported: 2010-06-16 02:23 PDT by Nikolas Zimmermann
Modified: 2010-06-29 00:08 PDT (History)
2 users (show)

See Also:


Attachments
Initial patch (28.48 KB, patch)
2010-06-16 04:36 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff
Follow-up patch (5.00 KB, patch)
2010-06-18 23:26 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Follow-up patch v2 (5.04 KB, patch)
2010-06-19 01:20 PDT, Nikolas Zimmermann
mitz: review-
Details | Formatted Diff | Diff
Follow-up patch v3 (4.88 KB, patch)
2010-06-22 08:05 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 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.
Comment 1 Nikolas Zimmermann 2010-06-16 04:36:36 PDT
Created attachment 58877 [details]
Initial patch
Comment 2 Dirk Schulze 2010-06-16 06:49:46 PDT
Comment on attachment 58877 [details]
Initial patch

r=me
Comment 3 Nikolas Zimmermann 2010-06-16 06:52:58 PDT
Landed in r61253.
Comment 4 mitz 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?
Comment 5 Nikolas Zimmermann 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?
Comment 6 mitz 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.
Comment 7 Nikolas Zimmermann 2010-06-18 23:26:34 PDT
Created attachment 59177 [details]
Follow-up patch

Fix issues Dan reported.
Comment 8 Nikolas Zimmermann 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.
Comment 9 mitz 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!
Comment 10 Nikolas Zimmermann 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.
Comment 11 Nikolas Zimmermann 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.
Comment 12 Nikolas Zimmermann 2010-06-22 08:05:14 PDT
Created attachment 59367 [details]
Follow-up patch v3
Comment 13 Dirk Schulze 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. :-)