Bug 41207 – absoluteRects() seems superflous

• Status: NEW
• Component: Layout and Rendering

[Nikolas Zimmermann]

Most renderers contain an absoluteRects() function, which seems superflous nowadays, the only remaining users are:
- Node::hasNonEmptyBoundingBox() - can easily be reworked to use absoluteQuads - is only interessted whether the bbox is empty or not.
- RenderObject::absoluteBoundingBoxRect()

// FIXME: useTransforms should go away eventually
IntRect absoluteBoundingBoxRect(bool useTransforms = false);

For the useTransforms=true case, it already uses absoluteQuads. I propose that it should always use this code path, then we can remove the parameter 'useTransforms' alltogether.
I'll try coming up with a patch.

[Nikolas Zimmermann]

Created attachment 59761 [details]
Initial patch

Okay, removing absoluteRects() seems to work fine. I didn't see any regression, compared two runs with "run-webkit-tests --tolerance 0 -p".
(Note: I updated inbetween, and got a difference in fast/events/platform-wheelevent-in-scrolling-div.html, I can retry with a fresh build, if the absoluteRects() change is guilty for that.)

Does anyone know how to make a test for the absoluteBoundingBoxRect() change, that would fail before and is fixed now?

[Early Warning System Bot]

Attachment 59761 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3325705

[Nikolas Zimmermann]

Oh, renderRect() is a newly added function - that's why the build fails atm.
I'm going to upload a new patch, but would like to hear first, if that approach makes sense?

[WebKit Review Bot]

Attachment 59761 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3313711

[WebKit Review Bot]

Attachment 59761 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3281771

[WebKit Review Bot]

Attachment 59761 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3294769

[Nikolas Zimmermann]

Dan/Dave/Simon, what do you think about this patch? If removing absoluteRects() is the right way to go, then I'm going to upload a new version that builds against trunk.

[Eric Seidel (no email)]

Comment on attachment 59761 [details]
Initial patch

I support removing absoluteRects.  Please upload the patch for review.

[Levi Weintraub]

I'm about to file a bug to update absoluteRects to take an IntSize instead of a pair of ints, but came across this beforehand.  I see it now used in RenderObject::absoluteBoundingBoxRect as well as the aforementioned hasNonEmptyBoundingBox.

Nikolas, do you have a workaround for absoluteBoundingBoxRect as well? If not, I'm going to just update the signature and keep this function alive. I'd be happier to see it go, of course :)