NEW 41207
absoluteRects() seems superflous
https://bugs.webkit.org/show_bug.cgi?id=41207
Summary absoluteRects() seems superflous
Nikolas Zimmermann
Reported 2010-06-25 03:36:28 PDT
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.
Attachments
Initial patch (18.52 KB, patch)
2010-06-25 05:42 PDT, Nikolas Zimmermann
eric: review-
Nikolas Zimmermann
Comment 1 2010-06-25 05:42:45 PDT
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
Comment 2 2010-06-25 05:52:11 PDT
Nikolas Zimmermann
Comment 3 2010-06-25 06:22:42 PDT
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
Comment 4 2010-06-25 06:29:38 PDT
WebKit Review Bot
Comment 5 2010-06-25 07:06:24 PDT
WebKit Review Bot
Comment 6 2010-06-25 12:34:44 PDT
Nikolas Zimmermann
Comment 7 2010-06-29 05:00:09 PDT
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 8 2010-08-06 13:35:32 PDT
Comment on attachment 59761 [details] Initial patch I support removing absoluteRects. Please upload the patch for review.
Levi Weintraub
Comment 9 2011-05-26 13:23:47 PDT
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 :)
Note You need to log in before you can comment on or make changes to this bug.