Bug 62130 - Convert RenderBox::absoluteRects to IntPoint
: Convert RenderBox::absoluteRects to IntPoint
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 62182
: 60318
  Show dependency treegraph
 
Reported: 2011-06-06 10:58 PST by
Modified: 2011-06-07 17:30 PST (History)


Attachments
Patch (12.22 KB, patch)
2011-06-06 13:02 PST, Emil A Eklund
no flags Review Patch | Details | Formatted Diff | Diff
Patch (12.19 KB, patch)
2011-06-06 14:51 PST, Emil A Eklund
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (12.73 KB, patch)
2011-06-06 15:40 PST, Emil A Eklund
no flags Review Patch | Details | Formatted Diff | Diff
Patch (15.78 KB, patch)
2011-06-07 14:21 PST, Emil A Eklund
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-06-06 10:58:55 PST
Ongoing tx, ty removal.
------- Comment #1 From 2011-06-06 13:02:53 PST -------
Created an attachment (id=96113) [details]
Patch
------- Comment #2 From 2011-06-06 14:07:28 PST -------
(From update of attachment 96113 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=96113&action=review

> Source/WebCore/dom/Node.cpp:842
> -    renderer()->absoluteRects(rects, absPos.x(), absPos.y());
> +    renderer()->absoluteRects(rects, flooredIntPoint(absPos));

Confused.  Is that what the old behavior did?

> Source/WebCore/rendering/RenderBlock.cpp:5692
> +        continuation()->absoluteRects(rects, accumulatedOffset - toSize(location() +
> +                inlineElementContinuation()->containingBlock()->location()));

I might have broken this out into a local IntPOint variable.  But looks fine.

> Source/WebCore/rendering/RenderInline.cpp:474
> +            rects.append(enclosingIntRect(FloatRect(accumulatedOffset + IntSize(curr->x(), curr->y()), IntSize(curr->width(), curr->height()))));

curr->location()?  curr->size()?

> Source/WebCore/rendering/RenderText.cpp:275
> +        rects.append(enclosingIntRect(FloatRect(accumulatedOffset + IntSize(box->x(), box->y()), IntSize(box->x(), box->y()))));

you meant box width/height.
------- Comment #3 From 2011-06-06 14:50:50 PST -------
(In reply to comment #2)
> > Source/WebCore/dom/Node.cpp:842
> > -    renderer()->absoluteRects(rects, absPos.x(), absPos.y());
> > +    renderer()->absoluteRects(rects, flooredIntPoint(absPos));
> 
> Confused.  Is that what the old behavior did?

Yeah, before there was an implicit cast from float to int, now it's explicit.

> > Source/WebCore/rendering/RenderInline.cpp:474
> > +            rects.append(enclosingIntRect(FloatRect(accumulatedOffset + IntSize(curr->x(), curr->y()), IntSize(curr->width(), curr->height()))));
> 
> curr->location()?  curr->size()?

Sadly we don't have InlineBox::size yet. Changed x, y to topLeft which is the location equivalent for InlineBox.

> 
> > Source/WebCore/rendering/RenderText.cpp:275
> > +        rects.append(enclosingIntRect(FloatRect(accumulatedOffset + IntSize(box->x(), box->y()), IntSize(box->x(), box->y()))));
> 
> you meant box width/height.

Good catch, thank you!
------- Comment #4 From 2011-06-06 14:51:40 PST -------
Created an attachment (id=96129) [details]
Patch
------- Comment #5 From 2011-06-06 15:10:31 PST -------
(From update of attachment 96129 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=96129&action=review

> Source/WebCore/rendering/RenderInline.cpp:474
> +            rects.append(enclosingIntRect(FloatRect(accumulatedOffset + curr->topLeft(), IntSize(curr->width(), curr->height()))));

It might be worth adding a InlineBox::size(), even just for thsi one caller.

> Source/WebCore/rendering/RenderText.cpp:275
> +        rects.append(enclosingIntRect(FloatRect(accumulatedOffset + box->topLeft(), IntSize(box->width(), box->height()))));

two callers. :)
------- Comment #6 From 2011-06-06 15:27:00 PST -------
> It might be worth adding a InlineBox::size(), even just for thsi one caller.
> ...
> two callers. :)

Good point, will fix before landing.

Thanks for the review Eric!
------- Comment #7 From 2011-06-06 15:40:11 PST -------
Created an attachment (id=96141) [details]
Patch for landing
------- Comment #8 From 2011-06-06 16:44:03 PST -------
The commit-queue encountered the following flaky tests while processing attachment 96141 [details]:

http/tests/websocket/tests/frame-length-overflow.html bug 61507 (author: abarth@webkit.org)
The commit-queue is continuing to process your patch.
------- Comment #9 From 2011-06-06 16:45:35 PST -------
(From update of attachment 96141 [details])
Clearing flags on attachment: 96141

Committed r88202: <http://trac.webkit.org/changeset/88202>
------- Comment #10 From 2011-06-06 16:45:40 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #11 From 2011-06-06 20:55:19 PST -------
This broke the clang build thusly:

In file included from third_party/WebKit/Source/WebCore/rendering/svg/RenderSVGContainer.h:28:
third_party/WebKit/Source/WebCore/rendering/svg/RenderSVGModelObject.h:58:18:error: 'WebCore::RenderSVGModelObject::absoluteRects' hides overloaded virtual function [-Werror,-Woverloaded-virtual]
    virtual void absoluteRects(Vector<IntRect>&, int tx, int ty);
                 ^
third_party/WebKit/Source/WebCore/rendering/RenderObject.h:593:18: note: hidden overloaded virtual function 'WebCore::RenderObject::absoluteRects' declared here
    virtual void absoluteRects(Vector<IntRect>&, const IntPoint&) { }
                 ^


Please be careful when doing refactorings. We have a chromium bot upstream (the clang bot) that catches some of the problems (as in this case), but it doesn't catch everything.
------- Comment #12 From 2011-06-07 10:39:43 PST -------
Thanks Nico! I'll be more careful.
------- Comment #13 From 2011-06-07 10:40:35 PST -------
Reopening as the patch got reverted.
------- Comment #14 From 2011-06-07 14:21:29 PST -------
Created an attachment (id=96296) [details]
Patch
------- Comment #15 From 2011-06-07 17:30:20 PST -------
(From update of attachment 96296 [details])
Clearing flags on attachment: 96296

Committed r88297: <http://trac.webkit.org/changeset/88297>
------- Comment #16 From 2011-06-07 17:30:26 PST -------
All reviewed patches have been landed.  Closing bug.