Summary: | Convert RenderBox::setLocation, setLogicalLocation and setLogicalSize to IntPoint/IntSize | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Emil A Eklund <eae> | ||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Enhancement | CC: | ap, commit-queue, darin, dglazkov, eric, simon.fraser, webkit.review.bot | ||||||||||||
Priority: | P3 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 60318 | ||||||||||||||
Attachments: |
|
Description
Emil A Eklund
2011-05-10 14:52:01 PDT
Created attachment 93036 [details]
Patch
Comment on attachment 93036 [details] Patch Attachment 93036 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8691028 Created attachment 93209 [details]
Patch
Comment on attachment 93209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93209&action=review > Source/WebCore/rendering/RenderBox.cpp:1374 > + setLocation(roundedAwayFromZeroIntPoint(FloatPoint(box->x(), box->y()))); Seems we should add a box->position() accessor... Except aren't x() y() ints here? Comment on attachment 93209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93209&action=review > Source/WebCore/platform/graphics/FloatPoint.h:207 > +inline IntPoint roundedAwayFromZeroIntPoint(const FloatPoint& p) > +{ > + return IntPoint(lroundf(p.x()), lroundf(p.y())); I don't understand why this is the right name? Comment on attachment 93209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93209&action=review >> Source/WebCore/platform/graphics/FloatPoint.h:207 >> + return IntPoint(lroundf(p.x()), lroundf(p.y())); > > I don't understand why this is the right name? I don't particularly like the name either but it was the best I could come up with given that roundedIntPoint was already taken and the difference between lround and other similar rounding functions such rint is the handling of the half way case. Anyway, I suppose I should be using roundedIntPoint instead, it'll introduce an extra cast but simplifies the code. >> Source/WebCore/rendering/RenderBox.cpp:1374 >> + setLocation(roundedAwayFromZeroIntPoint(FloatPoint(box->x(), box->y()))); > > Seems we should add a box->position() accessor... Except aren't x() y() ints here? The patch for bug 44412 adds a box->position method, once that has been landed this will change to setLocation(roundedAwayFromZeroIntPoint(box->location())); (In reply to comment #6) > the difference between lround and other similar rounding functions such rint is the handling of the half way case We should never use rint because we don’t want our code’s behavior to change based on the prevailing rounding mode. For some reason roundedIntPoint uses roundf, but it should probably use lroundf. I don’t see why we can’t use roundedIntPoint here. Does it give the wrong results for something? Created attachment 93313 [details] Patch > I don’t see why we can’t use roundedIntPoint here. Does it give the wrong results for something? No, it gives the same result for all cases I can come up with. Changed RenderBox::positionLineBox to use roundedIntPoint and removed new rounding function. Comment on attachment 93313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93313&action=review OK. > Source/WebCore/rendering/RenderBox.h:83 > + setLocation(IntPoint(location.y(), location.x())); Seems we want some sort of helper function to do this flip for us. Not necessarily in this change, but eventually. :) > Source/WebCore/rendering/RenderBox.h:104 > + setSize(IntSize(size.height(), size.width())); Again here. Thanks Eric!
> Seems we want some sort of helper function to do this flip for us. Not necessarily in this change, but eventually. :)
Couldn't come up with a good name for one, will add one once I do.
I'll wait a couple of hours before landing this to give others a chance to object.
(In reply to comment #10) > Couldn't come up with a good name for one, will add one once I do. One idea: flipAxes. Created attachment 93323 [details]
Patch
Turns out we already have IntSize::transposedSize and IntPoint::transposedPoint.
Comment on attachment 93323 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93323&action=review > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:329 > + IntPoint location(width() - borderRight() - paddingRight() - spinBox->width(), > + (diff / 2) + (diff % 2)); This indentation is not the WebKit coding style. I think you should just add IntPoint around (x,y) instead of changing the code so much. > I think you should just add IntPoint around (x,y) instead of changing the code so much.
Yeah, probably makes more sense in this case. I'll change it as suggested.
Thanks for the review Darin!
Created attachment 93343 [details]
Patch for landing
Comment on attachment 93343 [details] Patch for landing Clearing flags on attachment: 93343 Committed r86395: <http://trac.webkit.org/changeset/86395> All reviewed patches have been landed. Closing bug. |