Bug 60585 – Convert RenderBox::setLocation, setLogicalLocation and setLogicalSize to IntPoint/IntSize

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Emil A Eklund]

Convert RenderBox to use IntPoint for setLocation and setLogicalLocation instead of x,y pairs and setLogicalSize to use IntSize.

[Emil A Eklund]

Created attachment 93036 [details]
Patch

[WebKit Review Bot]

Comment on attachment 93036 [details]
Patch

Attachment 93036 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8691028

[Emil A Eklund]

Created attachment 93209 [details]
Patch

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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?

[Emil A Eklund]

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()));

[Darin Adler]

(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?

[Emil A Eklund]

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.

[Eric Seidel (no email)]

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.

[Emil A Eklund]

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.

[Darin Adler]

(In reply to comment #10)
> Couldn't come up with a good name for one, will add one once I do.

One idea: flipAxes.

[Emil A Eklund]

Created attachment 93323 [details]
Patch

Turns out we already have IntSize::transposedSize and IntPoint::transposedPoint.

[Darin Adler]

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.

[Emil A Eklund]

> 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!

[Emil A Eklund]

Created attachment 93343 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 93343 [details]
Patch for landing

Clearing flags on attachment: 93343

Committed r86395: <http://trac.webkit.org/changeset/86395>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.