Bug 52683

Created attachment 79391 [details] Patch

Attachment 79391 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1 Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.h:44: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/css/CSSParser.cpp:5319: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 79391 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7594202

Created attachment 79394 [details] Patch

Attachment 79394 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1 Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.h:44: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/css/CSSParser.cpp:5319: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 79391 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7633060

Comment on attachment 79394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79394&action=review > LayoutTests/transforms/3d/general/3dtransform-values.html:47 > + { 'transform' : 'perspective(banana)', 'result' : 'none' }, // unit must be length Test for bare perspective() ? { 'transform' : 'perspective()', 'result' : 'none' } Although maybe it's already there, just not in this diff. > Source/WebCore/ChangeLog:9 > + be a Length, rather than a bare number. Add a comment to say that bare numbers are still accepted (and are evaluated as px, as $DEITY intended)? > 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. > Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp:59 > + toT.applyPerspective(toP.calcFloatValue(1)); This is a question to educate me rather than a comment, but why do you pass 1 in as the maxvalue for calcFloatValue? If it is a percentage, then why clamp it? (and what does a percentage used in perspective mean?).

(In reply to comment #7) > (From update of attachment 79394 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79394&action=review > > > LayoutTests/transforms/3d/general/3dtransform-values.html:47 > > + { 'transform' : 'perspective(banana)', 'result' : 'none' }, // unit must be length > > Test for bare perspective() ? It's handled, but I could add a test, yeah. > > Source/WebCore/ChangeLog:9 > > + be a Length, rather than a bare number. > > Add a comment to say that bare numbers are still accepted (and are evaluated as px, as $DEITY intended)? Sure. > > 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. > > Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp:59 > > + toT.applyPerspective(toP.calcFloatValue(1)); > > This is a question to educate me rather than a comment, but why do you pass 1 in as the maxvalue for calcFloatValue? If it is a percentage, then why clamp it? (and what does a percentage used in perspective mean?). I believe the argument is unused for non-percentage units.

(In reply to comment #8) > > > 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. Obviously I wouldn't mind if translate didn't require units :) > > > > Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp:59 > > > + toT.applyPerspective(toP.calcFloatValue(1)); > > > > This is a question to educate me rather than a comment, but why do you pass 1 in as the maxvalue for calcFloatValue? If it is a percentage, then why clamp it? (and what does a percentage used in perspective mean?). > > I believe the argument is unused for non-percentage units. That's true, but why clamp it if I put perspective(120%) ? Maybe I misunderstand.

(In reply to comment #9) > (In reply to comment #8) > > > > > 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. > > Obviously I wouldn't mind if translate didn't require units :) We have to use units in CSS. This ain't SVG! > > > > Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp:59 > > > > + toT.applyPerspective(toP.calcFloatValue(1)); > > > > > > This is a question to educate me rather than a comment, but why do you pass 1 in as the maxvalue for calcFloatValue? If it is a percentage, then why clamp it? (and what does a percentage used in perspective mean?). > > > > I believe the argument is unused for non-percentage units. > > That's true, but why clamp it if I put perspective(120%) ? Maybe I misunderstand. Percentages are not allowed in perspective(), only lengths.

Comment on attachment 79394 [details] Patch 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:38 > + 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?