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: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Hans Muller
2012-11-27 12:53:40 PST
Created attachment 178057 [details]
Patch
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? (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?(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? (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 on attachment 178057 [details]
Patch
I am not convinced that this is faster or better. What is the context of this change?
(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 on attachment 178057 [details]
Patch
r=me
Comment on attachment 178057 [details] Patch Clearing flags on attachment: 178057 Committed r137914: <http://trac.webkit.org/changeset/137914> All reviewed patches have been landed. Closing bug. |