Bug 71444

Summary:

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

Product:

WebKit

Reporter:

Alexandru Chiculita <achicu>

Component:

CSS

Assignee:

Alexandru Chiculita <achicu>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

allan.jensen, commit-queue, dino, esprehn+autocc, kling, krit, macpherson, menard, michelangelo, mvujovic, syoichi

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

71446

Bug Blocks:

71395, 113695

Attachments:

Description	Flags
Patch.	dino: review+
Patch for landing.	none

Alexandru Chiculita

Reported 2011-11-03 01:18:48 PDT

Add code for parsing mat2/mat3/mat4 parameters.

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Dirk Schulze

Comment 1 2013-04-21 11:41:32 PDT

Created attachment 198969 [details] Patch. Patch is big because of testing.

WebKit Commit Bot

Comment 2 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.

Dean Jackson

Comment 3 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?

Dirk Schulze

Comment 4 2013-04-21 21:03:40 PDT

Created attachment 198984 [details] Patch for landing.

WebKit Commit Bot

Comment 5 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.

WebKit Commit Bot

Comment 6 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>

WebKit Commit Bot

Comment 7 2013-04-21 21:38:03 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.