Bug 63640 – Switch RenderLayer::hitTest* to to new layout types

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

[Emil A Eklund]

Convert RenderLayer hit testing functions to new layout types.

[Emil A Eklund]

Created attachment 99119 [details]
Patch

[Eric Seidel (no email)]

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?

[Emil A Eklund]

> Is this always what we want?

No. Perhaps roundedLayoutPoint would be a better name?

[Emil A Eklund]

Created attachment 99362 [details]
Patch

[Eric Seidel (no email)]

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?

[Emil A Eklund]

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

[Eric Seidel (no email)]

Won't there also be cases where we want to round in both worlds?  How will we separate those?

[Emil A Eklund]

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

[Emil A Eklund]

Eric, any suggestions?

[Eric Seidel (no email)]

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.

[Emil A Eklund]

Yay, thanks Eric. Filed bug 64021 to audit all the callsites when we make the switch.

[WebKit Review Bot]

Comment on attachment 99362 [details]
Patch

Clearing flags on attachment: 99362

Committed r90485: <http://trac.webkit.org/changeset/90485>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.