Bug 71444 – [Part 4] Parse the custom() function in -webkit-filter: parse the matN() functions

• Component: CSS
• Status: RESOLVED FIXED

[Alexandru Chiculita]

Add code for parsing mat2/mat3/mat4 parameters.

[Dirk Schulze]

Created attachment 198969 [details]
Patch.

Patch is big because of testing.

[WebKit Commit Bot]

Attachment 198969 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/parsing-custom-function-invalid-expected.txt', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/parsing-custom-function-valid-expected.txt', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/parsing-parameters-property-invalid-expected.txt', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/parsing-parameters-property-valid-expected.txt', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/script-tests/parsing-custom-function-invalid.js', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/script-tests/parsing-custom-function-valid.js', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/script-tests/parsing-parameters-property-invalid.js', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/script-tests/parsing-parameters-property-valid.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.vcproj/WebCore.vcproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParser.h', u'Source/WebCore/css/CSSValue.cpp', u'Source/WebCore/css/CSSValue.h', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/StyleResolver.h', u'Source/WebCore/css/WebKitCSSMatFunctionValue.cpp', u'Source/WebCore/css/WebKitCSSMatFunctionValue.h', u'Source/WebCore/platform/graphics/filters/CustomFilterArrayParameter.h', u'Source/WebCore/platform/graphics/filters/CustomFilterParameter.h', u'Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp']" exit_code: 1
Source/WebCore/platform/graphics/filters/CustomFilterParameter.h:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/WebKitCSSMatFunctionValue.cpp:51:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 198969 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=198969&action=review

> Source/WebCore/ChangeLog:23
> +        * GNUmakefile.list.am: Added WebKitCSSMatFunctionValue to individual build systems.
> +        * Target.pri:
> +        * WebCore.vcproj/WebCore.vcproj:
> +        * WebCore.vcxproj/WebCore.vcxproj:
> +        * WebCore.vcxproj/WebCore.vcxproj.filters:
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * css/CSSComputedStyleDeclaration.cpp:
> +        (WebCore::valueForCustomFilterMatParameter):

I really like per-function comments for significant changes.

> Source/WebCore/ChangeLog:29
> +        (WebCore):

And hate these useless lines! :)

> Source/WebCore/css/CSSParser.cpp:8483
> +    // The name length must be 5;
> +    if (name.length() != 5)

This comment adds nothing.

> Source/WebCore/css/CSSParser.cpp:8499
> +    UChar characterBuffer[5];
> +    if (name.is8Bit()) {
> +        const LChar* characters8 = name.characters8();
> +        for (unsigned i = 0; i < 5; ++i)
> +            characterBuffer[i] = characters8[i];
> +        characters = characterBuffer;
> +    } else
> +        characters = name.characters16();
> +
> +    if (!((characters[0] == 'm' || characters[0] == 'M')
> +        && (characters[1] == 'a' || characters[1] == 'A')
> +        && (characters[2] == 't' || characters[2] == 'T')
> +        && characters[4] == '('))
> +        return 0;

Is there a reason you didn't do something like equalIgnoringCase(value->function->name, "mat2(") rather than this more complex approach?

> Source/WebCore/css/WebKitCSSMatFunctionValue.cpp:2
> + * Copyright (C) 2012 Adobe Systems Incorporated. All rights reserved.

It is 2013. Stop living in the past!!

> Source/WebCore/css/WebKitCSSMatFunctionValue.cpp:57
> +        return "mat2(" + CSSValueList::customCssText() + ')';
> +    else if (length() == 9)
> +        return "mat3(" + CSSValueList::customCssText() + ')';
> +    else if (length() == 16)
> +        return "mat4(" + CSSValueList::customCssText() + ')';
> +    else {

Use StringBuilder?

[Dirk Schulze]

Created attachment 198984 [details]
Patch for landing.

[WebKit Commit Bot]

Attachment 198984 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/parsing-custom-function-invalid-expected.txt', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/parsing-custom-function-valid-expected.txt', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/parsing-parameters-property-invalid-expected.txt', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/parsing-parameters-property-valid-expected.txt', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/script-tests/parsing-custom-function-invalid.js', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/script-tests/parsing-custom-function-valid.js', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/script-tests/parsing-parameters-property-invalid.js', u'LayoutTests/css3/filters/custom-with-at-rule-syntax/script-tests/parsing-parameters-property-valid.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.vcproj/WebCore.vcproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParser.h', u'Source/WebCore/css/CSSValue.cpp', u'Source/WebCore/css/CSSValue.h', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/StyleResolver.h', u'Source/WebCore/css/WebKitCSSMatFunctionValue.cpp', u'Source/WebCore/css/WebKitCSSMatFunctionValue.h', u'Source/WebCore/platform/graphics/filters/CustomFilterArrayParameter.h', u'Source/WebCore/platform/graphics/filters/CustomFilterParameter.h', u'Source/WebCore/platform/graphics/filters/CustomFilterRenderer.cpp']" exit_code: 1
Source/WebCore/platform/graphics/filters/CustomFilterParameter.h:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 198984 [details]
Patch for landing.

Clearing flags on attachment: 198984

Committed r148852: <http://trac.webkit.org/changeset/148852>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.