Bug 114639 – [CSS Shaders] Extract validation logic out of CustomFilterValidatedProgram into a CustomFilterValidator class

• Component: Layout and Rendering
• Status: RESOLVED INVALID

[Max Vujovic]

Alex's comment:
"""
Let's extract the validation out of CustomFilterValidatedProgram into a separate class like CustomFilterValidator where we could store the mix/webgl shader validators and have all the blending code. CustomFilterValidatedProgram should only contain the "storing" logic, it is just the result of the CustomFilterValidator. I think we could consolidate that with CustomFilterProgram (which is the 'raw' program information).
"""

In addition, I don't think we need to keep around the validated shader strings after we have a compiled program.

[Max Vujovic]

Created attachment 200998 [details]
Patch

Some areas in the patch I'd like feedback on:

(1) CustomFilterValidator is a class with only static methods, and it should never be initialized. I've made its constructor private. I'm thinking I should additionally disallow the copy constructor and assignment. I could only find the macro "DISALLOW_IMPLICIT_CONSTRUCTORS" defined in wtf/dtoa/utils.h. It doesn't seem correct to include that file here, since the file is all about double to string conversion. I'm wondering if we have some convention in WebKit for static classes.

(2) I used only file static methods in CustomFilterValidator, since I try to keep things out of headers. Any thoughts on that? I could instead use class static methods or some mix between the two.

(3) Does the static local initialization of ANGLEWebKitBridge in CustomFilterValidator.cpp look ok? I didn't want to use the DEFINE_STATIC_LOCAL macro because it only supports sending arguments to the ANGLEWebKitBridge constructor. I need to both send arguments to the constructor *and* perform some initialization after running the constructor (see the createShaderValidator function).

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.cpp', u'Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.h', u'Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp', u'Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.h', u'Source/WebCore/platform/graphics/filters/CustomFilterValidator.cpp', u'Source/WebCore/platform/graphics/filters/CustomFilterValidator.h', u'Source/WebCore/rendering/RenderLayer.cpp']" exit_code: 1
Source/WebCore/platform/graphics/filters/CustomFilterValidator.cpp:284:  css_SetSatHelper is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 9 files


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

[Alexandru Chiculita]

Comment on attachment 200998 [details]
Patch

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

Thanks Max! I think this is a step in the right direction.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:45
> +    : m_programInfo(program->programInfo())

Let's remove this programInfo and just store the validated program info. See below.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:50
> +    m_isInitialized = CustomFilterValidator::validateShaderStrings(program, m_validatedVertexShader, m_validatedFragmentShader);

I think this might look better. This might also fix the questions about static methods & constructors.
CustomFilterValidator customFilterValidator();
if (customFilterValidator.validate(program)) {
    m_programInfo = CustomFilterProgramInfo(customFilterValidator.vertexShader(), customFilterValidator.fragmentShader(), programInfo.programType(), programInfo.mixSettings(), programInfo.meshType());
    m_initialized = true;
}

We would need a follow up patch to remove ProgramInfo and only store the required settings for the platform layer.

> Source/WebCore/platform/graphics/filters/CustomFilterValidator.cpp:104
> +static String blendFunctionString(BlendMode blendMode)

Let's add a bug to break this up in smaller functions. I would like to see these two functions (blendFunctionString and compositeFunctionString) extracted to a separate file. They seem independent from the validation part of the other class. The code might be useful in other parts in the future (ie. blending & compositing).

> Source/WebCore/platform/graphics/filters/CustomFilterValidator.cpp:593
> +        // FIXME: Report the validation errors.

Having this as an object might help do this patch in the future.