Bug 62130 – Convert RenderBox::absoluteRects to IntPoint

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Emil A Eklund]

Ongoing tx, ty removal.

[Emil A Eklund]

Created attachment 96113 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 96113 [details]
Patch

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.

[Emil A Eklund]

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

[Emil A Eklund]

Created attachment 96129 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 96129 [details]
Patch

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. :)

[Emil A Eklund]

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

[Emil A Eklund]

Created attachment 96141 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 96141 [details]:

http/tests/websocket/tests/frame-length-overflow.html bug 61507 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 96141 [details]
Patch for landing

Clearing flags on attachment: 96141

Committed r88202: <http://trac.webkit.org/changeset/88202>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nico Weber]

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.

[Emil A Eklund]

Thanks Nico! I'll be more careful.

[Emil A Eklund]

Reopening as the patch got reverted.

[Emil A Eklund]

Created attachment 96296 [details]
Patch

[WebKit Review Bot]

Comment on attachment 96296 [details]
Patch

Clearing flags on attachment: 96296

Committed r88297: <http://trac.webkit.org/changeset/88297>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.