Bug 60585

Summary: Convert RenderBox::setLocation, setLogicalLocation and setLogicalSize to IntPoint/IntSize
Product: WebKit Reporter: Emil A Eklund <eae>
Component: Layout and RenderingAssignee: 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 Flags
Patch
none
Patch
none
Patch
eric: review+
Patch
none
Patch for landing none

Description Emil A Eklund 2011-05-10 14:52:01 PDT
Convert RenderBox to use IntPoint for setLocation and setLogicalLocation instead of x,y pairs and setLogicalSize to use IntSize.
Comment 1 Emil A Eklund 2011-05-10 16:15:12 PDT
Created attachment 93036 [details]
Patch
Comment 2 WebKit Review Bot 2011-05-10 17:01:48 PDT
Comment on attachment 93036 [details]
Patch

Attachment 93036 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8691028
Comment 3 Emil A Eklund 2011-05-11 16:29:46 PDT
Created attachment 93209 [details]
Patch
Comment 4 Eric Seidel (no email) 2011-05-11 20:12:54 PDT
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 5 Eric Seidel (no email) 2011-05-11 20:13:21 PDT
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 6 Emil A Eklund 2011-05-12 10:40:15 PDT
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()));
Comment 7 Darin Adler 2011-05-12 11:19:53 PDT
(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?
Comment 8 Emil A Eklund 2011-05-12 11:23:12 PDT
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 9 Eric Seidel (no email) 2011-05-12 11:27:29 PDT
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.
Comment 10 Emil A Eklund 2011-05-12 11:40:50 PDT
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.
Comment 11 Darin Adler 2011-05-12 11:49:20 PDT
(In reply to comment #10)
> Couldn't come up with a good name for one, will add one once I do.

One idea: flipAxes.
Comment 12 Emil A Eklund 2011-05-12 12:15:09 PDT
Created attachment 93323 [details]
Patch

Turns out we already have IntSize::transposedSize and IntPoint::transposedPoint.
Comment 13 Darin Adler 2011-05-12 12:19:48 PDT
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.
Comment 14 Emil A Eklund 2011-05-12 12:24:36 PDT
> 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!
Comment 15 Emil A Eklund 2011-05-12 14:26:41 PDT
Created attachment 93343 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2011-05-12 16:30:20 PDT
Comment on attachment 93343 [details]
Patch for landing

Clearing flags on attachment: 93343

Committed r86395: <http://trac.webkit.org/changeset/86395>
Comment 17 WebKit Commit Bot 2011-05-12 16:30:26 PDT
All reviewed patches have been landed.  Closing bug.