Summary: | [CSS Shaders] Extract validation logic out of CustomFilterValidatedProgram into a CustomFilterValidator class | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Max Vujovic <mvujovic> | ||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED INVALID | ||||||
Severity: | Normal | CC: | achicu, commit-queue, dino, esprehn+autocc, glenn, krit, michelangelo, simon.fraser | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 71392 | ||||||
Attachments: |
|
Description
Max Vujovic
2013-04-15 14:26:29 PDT
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).
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 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. |