Bug 62130 - Convert RenderBox::absoluteRects to IntPoint
: Convert RenderBox::absoluteRects to IntPoint
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Emil A Eklund
:
Depends on: 62182
Blocks: 60318
  Show dependency treegraph
 
Reported: 2011-06-06 10:58 PDT by Emil A Eklund
Modified: 2011-06-07 17:30 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2011-06-06 10:58:55 PDT
Ongoing tx, ty removal.
Comment 1 Emil A Eklund 2011-06-06 13:02:53 PDT
Created attachment 96113 [details]
Patch
Comment 2 Eric Seidel 2011-06-06 14:07:28 PDT
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.
Comment 3 Emil A Eklund 2011-06-06 14:50:50 PDT
(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 Emil A Eklund 2011-06-06 14:51:40 PDT
Created attachment 96129 [details]
Patch
Comment 5 Eric Seidel 2011-06-06 15:10:31 PDT
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. :)
Comment 6 Emil A Eklund 2011-06-06 15:27:00 PDT
> 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 Emil A Eklund 2011-06-06 15:40:11 PDT
Created attachment 96141 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2011-06-06 16:44:03 PDT
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 WebKit Commit Bot 2011-06-06 16:45:35 PDT
Comment on attachment 96141 [details]
Patch for landing

Clearing flags on attachment: 96141

Committed r88202: <http://trac.webkit.org/changeset/88202>
Comment 10 WebKit Commit Bot 2011-06-06 16:45:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Nico Weber 2011-06-06 20:55:19 PDT
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 Emil A Eklund 2011-06-07 10:39:43 PDT
Thanks Nico! I'll be more careful.
Comment 13 Emil A Eklund 2011-06-07 10:40:35 PDT
Reopening as the patch got reverted.
Comment 14 Emil A Eklund 2011-06-07 14:21:29 PDT
Created attachment 96296 [details]
Patch
Comment 15 WebKit Review Bot 2011-06-07 17:30:20 PDT
Comment on attachment 96296 [details]
Patch

Clearing flags on attachment: 96296

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