Summary: | absoluteRects() seems superflous | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||
Component: | Layout and Rendering | Assignee: | Nikolas Zimmermann <zimmermann> | ||||
Status: | NEW --- | ||||||
Severity: | Normal | CC: | darin, dglazkov, dino, gustavo, hyatt, jamesr, leviw, mitz, simon.fraser, webkit.review.bot, xan.lopez | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
Nikolas Zimmermann
2010-06-25 03:36:28 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?
Attachment 59761 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3325705 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? Attachment 59761 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3313711 Attachment 59761 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3281771 Attachment 59761 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3294769 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 on attachment 59761 [details]
Initial patch
I support removing absoluteRects. Please upload the patch for review.
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 :) |