Bug 41207 - absoluteRects() seems superflous
Summary: absoluteRects() seems superflous
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-25 03:36 PDT by Nikolas Zimmermann
Modified: 2017-07-18 08:30 PDT (History)
11 users (show)

See Also:


Attachments
Initial patch (18.52 KB, patch)
2010-06-25 05:42 PDT, Nikolas Zimmermann
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 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.
Comment 1 Nikolas Zimmermann 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?
Comment 2 Early Warning System Bot 2010-06-25 05:52:11 PDT
Attachment 59761 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3325705
Comment 3 Nikolas Zimmermann 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?
Comment 4 WebKit Review Bot 2010-06-25 06:29:38 PDT
Attachment 59761 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3313711
Comment 5 WebKit Review Bot 2010-06-25 07:06:24 PDT
Attachment 59761 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3281771
Comment 6 WebKit Review Bot 2010-06-25 12:34:44 PDT
Attachment 59761 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3294769
Comment 7 Nikolas Zimmermann 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.
Comment 8 Eric Seidel (no email) 2010-08-06 13:35:32 PDT
Comment on attachment 59761 [details]
Initial patch

I support removing absoluteRects.  Please upload the patch for review.
Comment 9 Levi Weintraub 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 :)