[webkit-reviews] review granted: [Bug 52683] perspective() transform function should take lengths : [Attachment 79394] Patch

[bugzilla-daemon at webkit.org]

Chris Marrin <cmarrin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 52683: perspective() transform function should take lengths
https://bugs.webkit.org/show_bug.cgi?id=52683

Attachment 79394: Patch
https://bugs.webkit.org/attachment.cgi?id=79394&action=review

------- Additional Comments from Chris Marrin <cmarrin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=79394&action=review

r=me after possibly clarifying a couple of lines and fixing the style issues.

>>> Source/WebCore/css/CSSStyleSelector.cpp:7057
>>> +			 // This is a quirk that should go away when 3d
transforms are finalized.
>> 
>> I think you should say it is deprecated, but in honesty I'm not sure we can
ever make it go away. I don't see what the deal is with interpreting a bare
number as px. In this case there is no possible confusion - the problem is with
properties like line-height in which a bare number is a multiplier, not a
length.
> 
> It should match translate() etc, for which I think we require units.

Also, you should say what the quirk is (which I think is that we still accept
bare naked numbers in perspective())

> Source/WebCore/css/CSSStyleSelector.cpp:7060
> +		       val = min<double>(val, numeric_limits<int>::max());

Wow, this looks icky. You really have to use templates here? It sure hides what
you're actually doing in all that noise.

>
Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp:3
8
> +    return static_cast<int>(max<double>(min<double>(val,
numeric_limits<int>::max()), 0));

Again, holy obfuscation Batman, what is this actually doing? Maybe at least
split this into 3 lines: max, min and cast?