Bug 76253

Created attachment 122423 [details] Saved work Saving my work on this bug. The code should be complete, but not yet ready for review because there are no tests yet. I need to create tests for: 1. computed-style float, vec2, vec3, vec4 2. painting with float, vec2, vec3, vec4 3. [done] css parsing for float, vec2, vec3, vec4 was already done

Created attachment 122618 [details] Patch V1

Comment on attachment 122618 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=122618&action=review r- because you should really just have a single Vector parameter class with a variable number of elements rather than a bunch of vector classes of different sizes. > Source/WebCore/css/CSSStyleSelector.cpp:5281 > + // TODO: Implement other parameters types parsing. This should be FIXME > Source/WebCore/platform/graphics/filters/CustomFilterParameter.h:85 > +class CustomFilterNumberVectorOf2Parameter : public CustomFilterParameter { It would be much better to just have a single Vector class that can hold any number of values. Then you could use it for the current vector type's you've defined as well as all the matrix types. It would be way less code with not much runtime cost (and extra pointer and length field). Given that we don't expect thousands of custom filters on the page or thousands of parameters per custom filter, this cost seems reasonable. I think it's reasonable to keep CustomFilterNumberParameter (as opposed to just using a vector of 1 number) if you want. But even getting rid of that wouldn't be bad.

Created attachment 122751 [details] Patch V2 Thanks for the review! I've updated the patch. Now I only have one CustomFilterNumberParameter class for all the float values with a Vector<double, 4> inside. I've also removed the HashMap and used a simple Vector. I've enforced a sorted vector in CustomFilterOperation, so that blending of two CustomFilterOperations can be done fast.

Comment on attachment 122751 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=122751&action=review This is completly fine, but it needs refactoring! The code easily grows to be unreadable, this way. Please use smaller helper functions, to gain readability. r- for now: > LayoutTests/css3/filters/effect-custom-combined-missing.html:18 > + // We need to run the tests after the downloading succeeded. > + // Using a timer is not ideal, but there seems to be no better options. > + setTimeout(function() { As discussed on IRC, this is not a good idea. But there are already existing tests exhibing the same problem. http://trac.webkit.org/changeset/101858 fixed the same issue for SVG Fonts, we may reuse the idea behind this for shaders as well, and ilsten to the documents load event here. > LayoutTests/css3/filters/resources/vertex-offset-parameters.vs:13 > + One newline too much. > LayoutTests/css3/filters/resources/vertex-offset-parameters.vs:15 > + float perspective = - 1.0 / p; I'd avoid the local. > LayoutTests/css3/filters/resources/vertex-offset-parameters.vs:25 > + return mat4( > + 1.0, 0.0, 0.0, 0.0, Indentation. > LayoutTests/css3/filters/resources/vertex-offset-parameters.vs:63 > + gl_Position = u_projectionMatrix * perspectiveMatrix(perspective) * translateMatrix(vertex_offset) * rotateMatrix(rotate * 3.14 / 180.0) * a_position; Hm, no M_PI available.... Is 2 digit precision enough? I guess it doesn't matter. > Source/WebCore/ChangeLog:16 > + * WebCore.xcodeproj/project.pbxproj: What about all the other build systems? You still should add the headers, where needed, even if there's no cpp file to add. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:835 > + if (parameters.size()) { Early exit, if parameters.size is 0. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:838 > + for (CustomFilterParameterList::const_iterator iter = parameters.begin(), end = parameters.end(); iter != end; ++iter) { It's faster to traverse a vector by index, instead of using the iterators. size_t parametersSize = parameter.size(); for (size_t i = 0; i < parametersSize; ++i) { CustomFilterParameter* parameter = parameters[i].get(); ... > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:846 > + RefPtr<CSSValue> parameterCSSValue; > + > + switch (parameter->parameterType()) { > + case CustomFilterParameter::NUMBER: { > + CustomFilterNumberParameter* numberParameter = static_cast<CustomFilterNumberParameter*>(parameter); > + RefPtr<CSSValueList> numberParameterValue = CSSValueList::createSpaceSeparated(); That cries for a helper function, that returns PassRefPtr<CSSValue> to construct the "parameterCSSValue" out of a CustomFilterParameter. If you add more cases to the enum, it will get unreadable otherwise. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:852 > + default: I'd rather avoid the default: here, so any missing value from the enum can be detected at compile-time. > Source/WebCore/css/CSSStyleSelector.cpp:5228 > + // having just two big values: list of shaders and list of parameters. two big values? big? > Source/WebCore/css/CSSStyleSelector.cpp:5240 > + if (parametersValue) { Early exit, if parametersValue is 0. > Source/WebCore/css/CSSStyleSelector.cpp:5246 > + CSSValueListIterator parameterIterator(parametersValue); > + for (; parameterIterator.hasMore(); parameterIterator.advance()) { > + if (!parameterIterator.value()->isValueList()) > + return 0; > + CSSValueListIterator iterator(parameterIterator.value()); > + if (!iterator.isPrimitiveValue()) This whole procedure could be moved somewhere else, in a static inline helper function. > Source/WebCore/css/CSSStyleSelector.cpp:5283 > + if (firstPrimitiveValue->primitiveType() == CSSPrimitiveValue::CSS_NUMBER) { > + RefPtr<CustomFilterNumberParameter> numberParameter = CustomFilterNumberParameter::create(name); Same here, the creation of CustomFilterXXXParameters should be in a seperated method, to make this code more readable. > Source/WebCore/css/CSSValueList.h:98 > + bool hasAdvanced() { return m_position; } Why not just use index() ? > Source/WebCore/platform/graphics/filters/CustomFilterParameter.h:44 > + NUMBER Maybe add a FIXME here about the still missing types > Source/WebCore/platform/graphics/filters/CustomFilterParameter.h:60 > + Newline too much. > Source/WebCore/platform/graphics/filters/CustomFilterParameter.h:65 > +class CustomFilterNumberParameter : public CustomFilterParameter { We normally only use one class per file. > Source/WebCore/platform/graphics/filters/CustomFilterShader.h:67 > + int uniformLocationByName(const String& name); s/ name// > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:207 > + for (CustomFilterParameterList::iterator iter = m_parameters.begin(), end = m_parameters.end(); iter != end; ++iter) { Traverse Vector by index. > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:213 > + switch (parameter->parameterType()) { > + case CustomFilterParameter::NUMBER: { This could again, go into another function, that takes both a CustomFilterNumberParameter* and operates on m_context.

Created attachment 122906 [details] Patch V3 Thanks again for the feedback. I think I've covered all of it, except for the setTimeout in the test case. For that I've added https://bugs.webkit.org/show_bug.cgi?id=76535 to track that one.

Attachment 122906 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1 Source/WebCore/ChangeLog:27: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:34: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:35: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:72: Line contains tab character. [whitespace/tab] [5] Total errors found: 4 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 122909 [details] Patch V4 Removing tabs from changelog file.

Comment on attachment 122909 [details] Patch V4 View in context: https://bugs.webkit.org/attachment.cgi?id=122909&action=review Looks great to me. Much better readable, eh? :-) > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:730 > + // FIXME: Add here computed style for the other types: boolean, transform, matrix, texture. I'd ASSERT(parameter) here as well. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:861 > + const CustomFilterParameterList& parameters = customOperation->parameters(); > + if (!parameters.size()) > + break; > + > + RefPtr<CSSValueList> parametersCSSValue = CSSValueList::createCommaSeparated(); > + > + size_t parametersSize = parameters.size(); I'd write this as: size_t parametersSize = parameters.size(); if (!parametersSize) break; RefPtr<CSSValueList> .... you can also omit the extra newline. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:871 > + One newline too much.

Created attachment 122915 [details] Patch for landing

Comment on attachment 122915 [details] Patch for landing Clearing flags on attachment: 122915 Committed r105276: <http://trac.webkit.org/changeset/105276>

All reviewed patches have been landed. Closing bug.