Bug 63640

Summary: Switch RenderLayer::hitTest* to to new layout types
Product: WebKit Reporter: Emil A Eklund <eae>
Component: Layout and RenderingAssignee: Emil A Eklund <eae>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, leviw, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 63567    
Attachments:
Description Flags
Patch
none
Patch none

Description Emil A Eklund 2011-06-29 11:42:01 PDT
Convert RenderLayer hit testing functions to new layout types.
Comment 1 Emil A Eklund 2011-06-29 11:54:12 PDT
Created attachment 99119 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-06-29 12:05:55 PDT
Comment on attachment 99119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99119&action=review

> Source/WebCore/rendering/LayoutTypes.h:51
> +    return roundedIntPoint(point);

Is this always what we want?
Comment 3 Emil A Eklund 2011-06-29 12:07:28 PDT
> Is this always what we want?

No. Perhaps roundedLayoutPoint would be a better name?
Comment 4 Emil A Eklund 2011-06-30 13:37:07 PDT
Created attachment 99362 [details]
Patch
Comment 5 Eric Seidel (no email) 2011-06-30 13:50:56 PDT
Comment on attachment 99362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99362&action=review

> Source/WebCore/rendering/LayoutTypes.h:52
> +inline LayoutPoint roundedLayoutPoint(FloatPoint point)
> +{
> +    return roundedIntPoint(point);
> +}

These methods scare me.  What is this going to do when you move LayoutPoint to float?  Is it going to still round?
Comment 6 Emil A Eklund 2011-06-30 13:52:35 PDT
(In reply to comment #5)
> These methods scare me.  What is this going to do when you move LayoutPoint to float?  Is it going to still round?

No, I tried to explain the rational in the change log. Once we switch to float or fixed those methods will be no-ops. They're only used in placed where we do rounding today where we won't have to do rounding in the future.
Comment 7 Eric Seidel (no email) 2011-06-30 13:53:28 PDT
Won't there also be cases where we want to round in both worlds?  How will we separate those?
Comment 8 Emil A Eklund 2011-06-30 13:59:11 PDT
(In reply to comment #7)
> Won't there also be cases where we want to round in both worlds?  How will we separate those?

There might be. For those we plan to move the rounding into semantically meaningful helper methods on one of the the rendering base classes.

The roundedLayout* methods used in this and my other pending patches are all for cases where we do not want to do rounding going forward. Perhaps we should try to make this clearer by naming them something else? I'm not sure what that name would be though.
Comment 9 Emil A Eklund 2011-07-06 08:57:59 PDT
Eric, any suggestions?
Comment 10 Eric Seidel (no email) 2011-07-06 12:58:38 PDT
Comment on attachment 99362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99362&action=review

>> Source/WebCore/rendering/LayoutTypes.h:52
>> +}
> 
> These methods scare me.  What is this going to do when you move LayoutPoint to float?  Is it going to still round?

I'm OK with this, assuming you all plan to audit all these callsites at some point.
Comment 11 Emil A Eklund 2011-07-06 13:05:05 PDT
Yay, thanks Eric. Filed bug 64021 to audit all the callsites when we make the switch.
Comment 12 WebKit Review Bot 2011-07-06 13:20:18 PDT
Comment on attachment 99362 [details]
Patch

Clearing flags on attachment: 99362

Committed r90485: <http://trac.webkit.org/changeset/90485>
Comment 13 WebKit Review Bot 2011-07-06 13:20:22 PDT
All reviewed patches have been landed.  Closing bug.