WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
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
Attachment 59761
[details]
did not build on qt: Build output:
http://webkit-commit-queue.appspot.com/results/3325705
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
Attachment 59761
[details]
did not build on chromium: Build output:
http://webkit-commit-queue.appspot.com/results/3313711
WebKit Review Bot
Comment 5
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
WebKit Review Bot
Comment 6
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
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.
Top of Page
Format For Printing
XML
Clone This Bug