Bug 103450 – [CSS Exclusions] Add helper functions for converting floats to LayoutUnits

• Component: CSS
• Status: RESOLVED FIXED

[Hans Muller]

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.

[Hans Muller]

Created attachment 178057 [details]
Patch

[Bem Jones-Bey]

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?

[Hans Muller]

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

[Bem Jones-Bey]

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?

[Hans Muller]

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

[Dirk Schulze]

Comment on attachment 178057 [details]
Patch

I am not convinced that this is faster or better. What is the context of this change?

[Hans Muller]

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

[Dirk Schulze]

Comment on attachment 178057 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 178057 [details]
Patch

Clearing flags on attachment: 178057

Committed r137914: <http://trac.webkit.org/changeset/137914>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.