Bug 71444 - [Part 4] Parse the custom() function in -webkit-filter: parse the matN() functions
Summary: [Part 4] Parse the custom() function in -webkit-filter: parse the matN() func...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandru Chiculita
URL:
Keywords:
Depends on: 71446
Blocks: 71395 113695
  Show dependency treegraph
 
Reported: 2011-11-03 01:18 PDT by Alexandru Chiculita
Modified: 2013-04-21 21:38 PDT (History)
11 users (show)

See Also:


Attachments
Patch. (93.27 KB, patch)
2013-04-21 11:41 PDT, Dirk Schulze
dino: review+
Details | Formatted Diff | Diff
Patch for landing. (92.62 KB, patch)
2013-04-21 21:03 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2011-11-03 01:18:48 PDT
Add code for parsing mat2/mat3/mat4 parameters.
Comment 1 Dirk Schulze 2013-04-21 11:41:32 PDT
Created attachment 198969 [details]
Patch.

Patch is big because of testing.
Comment 2 WebKit Commit Bot 2013-04-21 11:43:25 PDT
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.
Comment 3 Dean Jackson 2013-04-21 18:20:29 PDT
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?
Comment 4 Dirk Schulze 2013-04-21 21:03:40 PDT
Created attachment 198984 [details]
Patch for landing.
Comment 5 WebKit Commit Bot 2013-04-21 21:06:17 PDT
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.
Comment 6 WebKit Commit Bot 2013-04-21 21:38:00 PDT
Comment on attachment 198984 [details]
Patch for landing.

Clearing flags on attachment: 198984

Committed r148852: <http://trac.webkit.org/changeset/148852>
Comment 7 WebKit Commit Bot 2013-04-21 21:38:03 PDT
All reviewed patches have been landed.  Closing bug.