Bug 114639 - [CSS Shaders] Extract validation logic out of CustomFilterValidatedProgram into a CustomFilterValidator class
Summary: [CSS Shaders] Extract validation logic out of CustomFilterValidatedProgram in...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 71392
  Show dependency treegraph
Reported: 2013-04-15 14:26 PDT by Max Vujovic
Modified: 2014-03-02 08:52 PST (History)
8 users (show)

See Also:

Patch (63.76 KB, patch)
2013-05-07 17:02 PDT, Max Vujovic
achicu: review-
mvujovic: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Max Vujovic 2013-04-15 14:26:29 PDT
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.
Comment 1 Max Vujovic 2013-05-07 17:02:34 PDT
Created attachment 200998 [details]

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).
Comment 2 WebKit Commit Bot 2013-05-07 17:05:13 PDT
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.
Comment 3 Alexandru Chiculita 2013-05-10 17:03:12 PDT
Comment on attachment 200998 [details]

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.