Bug 63640 - Switch RenderLayer::hitTest* to to new layout types
Summary: Switch RenderLayer::hitTest* to to new layout types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks: 63567
  Show dependency treegraph
 
Reported: 2011-06-29 11:42 PDT by Emil A Eklund
Modified: 2011-07-06 13:20 PDT (History)
3 users (show)

See Also:


Attachments
Patch (17.37 KB, patch)
2011-06-29 11:54 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (17.44 KB, patch)
2011-06-30 13:37 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.