Bug 103450

Summary:

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

Product:

WebKit

Reporter:

Hans Muller <giles_joplin>

Component:

CSS

Assignee:

Hans Muller <giles_joplin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bjonesbe, eric, ojan, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none

Hans Muller

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

Attachments
Patch (4.84 KB, patch) 2012-12-06 12:14 PST, Hans Muller	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Hans Muller

Comment 1 2012-12-06 12:14:22 PST

Created attachment 178057 [details] Patch

Bem Jones-Bey

Comment 2 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?

Hans Muller

Comment 3 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.

Bem Jones-Bey

Comment 4 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?

Hans Muller

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

Dirk Schulze

Comment 6 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?

Hans Muller

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

Dirk Schulze

Comment 8 2012-12-17 08:48:46 PST

Comment on attachment 178057 [details] Patch r=me

WebKit Review Bot

Comment 9 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>

WebKit Review Bot

Comment 10 2012-12-17 09:12:01 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.