Bug 103450 - [CSS Exclusions] Add helper functions for converting floats to LayoutUnits
Summary: [CSS Exclusions] Add helper functions for converting floats to LayoutUnits
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Muller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-27 12:53 PST by Hans Muller
Modified: 2012-12-17 09:12 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.84 KB, patch)
2012-12-06 12:14 PST, Hans Muller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Muller 2012-11-27 12:53:40 PST
When a float logicalTop value is converted to a LayoutUnit its necessary to use LayoutUnit::fromFloatCeil().  This ensures that we're snapping to a value that's inside the ExclusionShape.  Similarly, to convert a logicalBottom value from float to LayoutUnit we use LayoutUnit::fromFloatFloor().  See https://bugs.webkit.org/show_bug.cgi?id=100996.

These conversionsares likely to be needed in several places, for example see https://bugs.webkit.org/show_bug.cgi?id=103327, so moving them to an aptly named pair of helper functions would be useful.
Comment 1 Hans Muller 2012-12-06 12:14:22 PST
Created attachment 178057 [details]
Patch
Comment 2 Bem Jones-Bey 2012-12-06 14:03:55 PST
Since these are private methods, I don't see how this accomplishes the goal of code reuse. As it is, it seems like the layer of indirection only serves to make things harder to read. Am I missing something?
Comment 3 Hans Muller 2012-12-06 14:45:21 PST
(In reply to comment #2)
> Since these are private methods, I don't see how this accomplishes the goal of code reuse. As it is, it seems like the layer of indirection only serves to make things harder to read. Am I missing something?

At the moment they're only used by ExclusionShapeInsideInfo methods so private scope is adequate.  If we find that they're more broadly needed, it's easy to expand their scope.
Comment 4 Bem Jones-Bey 2012-12-06 14:48:48 PST
Then why add them now as opposed to when they are more widely needed?(In reply to comment #3)
> (In reply to comment #2)
> > Since these are private methods, I don't see how this accomplishes the goal of code reuse. As it is, it seems like the layer of indirection only serves to make things harder to read. Am I missing something?
> 
> At the moment they're only used by ExclusionShapeInsideInfo methods so private scope is adequate.  If we find that they're more broadly needed, it's easy to expand their scope.

Then why add them now as opposed to when they are more widely needed?
Comment 5 Hans Muller 2012-12-06 14:51:03 PST
(In reply to comment #4)
> Then why add them now as opposed to when they are more widely needed?(In reply to comment #3)
> > (In reply to comment #2)
> > > Since these are private methods, I don't see how this accomplishes the goal of code reuse. As it is, it seems like the layer of indirection only serves to make things harder to read. Am I missing something?
> > 
> > At the moment they're only used by ExclusionShapeInsideInfo methods so private scope is adequate.  If we find that they're more broadly needed, it's easy to expand their scope.
> 
> Then why add them now as opposed to when they are more widely needed?

Because they clarify and isolate the use of  LayoutUnit::fromFloatCeil() and LayoutUnit::fromFloatFloor().
Comment 6 Dirk Schulze 2012-12-06 15:33:44 PST
Comment on attachment 178057 [details]
Patch

I am not convinced that this is faster or better. What is the context of this change?
Comment 7 Hans Muller 2012-12-06 15:43:50 PST
(In reply to comment #6)
> (From update of attachment 178057 [details])
> I am not convinced that this is faster or better. What is the context of this change?

Previously the implementation contained three occurrences of an idiom similar to this:

// Use fromFloatCeil() to ensure that the returned LayoutUnit value is within the shape's bounds.
LayoutUnit newLineTop = LayoutUnit::fromFloatCeil(floatNewLineTop);

The comment was necessary because the code alone is somewhat obscure.  Replacing this idiom with an inline with a suggestive name makes the code a little more readable and obviates replicating the comment at each site.
Comment 8 Dirk Schulze 2012-12-17 08:48:46 PST
Comment on attachment 178057 [details]
Patch

r=me
Comment 9 WebKit Review Bot 2012-12-17 09:11:58 PST
Comment on attachment 178057 [details]
Patch

Clearing flags on attachment: 178057

Committed r137914: <http://trac.webkit.org/changeset/137914>
Comment 10 WebKit Review Bot 2012-12-17 09:12:01 PST
All reviewed patches have been landed.  Closing bug.