Bug 65722

Summary: Rename absoluteQuadsForRange and InlineTextBox::selectionRect to local*
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Emil A Eklund <eae>
Severity: Normal CC: leviw, rniwa, simon.fraser, thorton, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch none

Description Simon Fraser (smfr) 2011-08-04 15:02:45 PDT
RenderText::absoluteQuadsForRange() has:

            FloatRect rect = absoluteQuadForTextBox(box, start, end, useSelectionHeight);
            if (!rect.isZero())

This is converting local to absolute twice.
Comment 1 Simon Fraser (smfr) 2011-08-04 15:03:02 PDT
Regression from http://trac.webkit.org/changeset/91763
Comment 2 Emil A Eklund 2011-08-05 10:58:24 PDT
Actually I think it's just an unfortunate (and incorrect) name. absoluteQuadForTextBox doesn't convert to absolute, I'll make sure this is the case and rename it to localQuadForTextBox.
Comment 3 Emil A Eklund 2011-08-05 13:47:02 PDT
Created attachment 103101 [details]
Comment 4 Simon Fraser (smfr) 2011-08-05 13:51:37 PDT
Comment on attachment 103101 [details]

Not sure this is right. quadForTextBox() calls selectionRect(), which uses selectionRectForRepaint(). The 'for repaint' indicates that it's already been converted to the coordinate system of the repaintContainer (or the root).

If there aren't repaint tests for this code, you should make some.
Comment 5 Emil A Eklund 2011-08-05 14:41:41 PDT
The change 91763 didn't change change any logic apart from replacing a check for (!r.isEmpty()) with (r.height()) to match absoluteRectsForRange. Just as before it calls selectionRect and then adjust the rect and converts it to the absolute coordinate space using localToAbsoluteQuad.

As far as I can tell quadForTextBox calls InlineTextBox::selectionRect which in turn returns the selection rect in the local coordinate space.

There are a couple of tests that cover this code already and they helped me catch a a mistake in the first version of my patch.

If you still do not trust this to be correct I'd gladly revert change 91763.
Comment 6 Simon Fraser (smfr) 2011-08-05 14:47:06 PDT
Ah, InlineTextBox::selectionRect() is in local coords, RenderObject::selectionRect() is in "repaint" coords. The former should be renamed ("localSelectionRect()"?)
Comment 7 Emil A Eklund 2011-08-05 14:57:47 PDT
(In reply to comment #6)
> Ah, InlineTextBox::selectionRect() is in local coords, RenderObject::selectionRect() is in "repaint" coords. The former should be renamed ("localSelectionRect()"?)

Makes sense, I'll make the change.
Comment 8 Emil A Eklund 2011-08-05 16:13:50 PDT
Created attachment 103128 [details]
Comment 9 Emil A Eklund 2011-08-05 16:21:06 PDT
Thanks Simon!
Comment 10 WebKit Review Bot 2011-08-08 13:11:36 PDT
Comment on attachment 103128 [details]

Clearing flags on attachment: 103128

Committed r92625: <http://trac.webkit.org/changeset/92625>
Comment 11 WebKit Review Bot 2011-08-08 13:11:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Simon Fraser (smfr) 2012-10-23 15:55:00 PDT
RenderText::absoluteRectsForRange still has:

            // FIXME: This code is wrong. It's converting local to absolute twice. http://webkit.org/b/65722
Comment 13 Tim Horton 2013-04-12 01:16:01 PDT
The confusing comment remains and I have no idea if it's true or not anymore (but it seems not).
Comment 14 Tim Horton 2017-07-21 16:09:34 PDT
It's still there :(
Comment 15 Tim Horton 2017-07-21 17:13:31 PDT
No longer: https://trac.webkit.org/changeset/219753/webkit