Created attachment 100338 [details] Patch

Comment on attachment 100338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100338&action=review Otherwise looks fine. > Source/WebCore/platform/Length.h:158 > + default: > + return static_cast<float>(undefinedLength); Normally we don't use default: cases for switch(enum), but maybe it makes sense here? > Source/WebCore/platform/Length.h:162 > + float calcFloatValue(float maxValue) const Did you copy paste this same function twice?

Comment on attachment 100338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100338&action=review >> Source/WebCore/platform/Length.h:158 >> + return static_cast<float>(undefinedLength); > > Normally we don't use default: cases for switch(enum), but maybe it makes sense here? This function remains unchanged, I just fixed the indenting for webkit-style. >> Source/WebCore/platform/Length.h:162 >> + float calcFloatValue(float maxValue) const > > Did you copy paste this same function twice? I'm adding a version that takes a float maxValue.

Comment on attachment 100338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100338&action=review >>> Source/WebCore/platform/Length.h:162 >>> + float calcFloatValue(float maxValue) const >> >> Did you copy paste this same function twice? > > I'm adding a version that takes a float maxValue. Shouldn't we just templatize this sucker?

Comment on attachment 100338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100338&action=review >>>> Source/WebCore/platform/Length.h:162 >>>> + float calcFloatValue(float maxValue) const >>> >>> Did you copy paste this same function twice? >> >> I'm adding a version that takes a float maxValue. > > Shouldn't we just templatize this sucker? Ultimately I believe lengths should stop being ints at all, but I don't know that templates will help us here since we generally create lengths at parse-time. Anyways I think I'll revisit this when we're a bit closer to switching over. This whole class could use some cleanup at that point.

Created attachment 100348 [details] Patch

(In reply to comment #6) > Created an attachment (id=100348) [details] > Patch Ping for re-review :)

Comment on attachment 100348 [details] Patch This changes some platform things to know about Layout*. Darin may feel this is also a layering violation. You should check.

Comment on attachment 100348 [details] Patch This looks like a layering violation to me. The transforms themselves are part of the graphics framework, not the rendering system, so they should not be using layout types. Instead they could be overloaded to work on both int and float, or changed to always use float.

Comment on attachment 100348 [details] Patch Agree with Darin.

Created attachment 102050 [details] Patch

Can I get a review? This just works :)

Comment on attachment 102050 [details] Patch Thanks for the review, Simon.

Comment on attachment 102050 [details] Patch Clearing flags on attachment: 102050 Committed r91880: <http://trac.webkit.org/changeset/91880>

All reviewed patches have been landed. Closing bug.