Bug 93869 – [CSS Shaders] Implement normal blend mode and source-atop compositing mode

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Max Vujovic]

Instead of allowing direct texture access in an author's shader, CSS Shaders will blend special symbols in the author's shader (css_MixColor and css_ColorMatrix) with the DOM element texture. This requires WebKit to add code to the author's shader to perform blending, compositing, and texture access.

The author specifies the blend mode and composite mode via the CSS mix function like this:
-webkit-filter: custom(none mix(shader.fs normal source-atop));

First, we will implement normal and source-atop. The other blend modes and composite ops will come in later patches.

[Max Vujovic]

Created attachment 158428 [details]
Patch

Alex, could you take a look at this please?

[WebKit Review Bot]

Comment on attachment 158428 [details]
Patch

Attachment 158428 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13493840

[Early Warning System Bot]

Comment on attachment 158428 [details]
Patch

Attachment 158428 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13492747

[Early Warning System Bot]

Comment on attachment 158428 [details]
Patch

Attachment 158428 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13489905

[Peter Beverloo (cr-android ews)]

Comment on attachment 158428 [details]
Patch

Attachment 158428 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13512083

[Max Vujovic]

Created attachment 159172 [details]
Patch

I've refactored the patch based on a verbal discussion with Alex. The details are in WebCore/ChangeLog in the patch.

[WebKit Review Bot]

Comment on attachment 159172 [details]
Patch

Attachment 159172 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13530129

New failing tests:
css3/filters/custom/custom-filter-color-matrix.html
css3/filters/custom/filter-fallback-to-software.html
css3/filters/custom/invalid-custom-filter-shader.html
css3/filters/custom/custom-filter-composite-source-atop.html

[WebKit Review Bot]

Created attachment 159183 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Max Vujovic]

Created attachment 159237 [details]
Patch

Added test expectations for Chromium for new tests.

[WebKit Review Bot]

Comment on attachment 159237 [details]
Patch

Attachment 159237 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13529247

New failing tests:
css3/filters/custom/custom-filter-color-matrix.html

[WebKit Review Bot]

Created attachment 159252 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Max Vujovic]

Created attachment 159462 [details]
Patch

Added lines to chromium/TestExpectations requesting Win and Linux baselines for new tests. This is captured in bug 94492.

[Alexandru Chiculita]

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

Saving first round of this review. Some small improvements.

> Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.cpp:50
> +ANGLEWebKitBridge* CustomFilterGlobalContext::webglValidator()

I think naming this webGLShadersValidator() would make it more similar to cssShadersValidator(). I would also welcome an explanation about when we use webgl vs. css shaders.

> Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.cpp:64
> +PassOwnPtr<ANGLEWebKitBridge> CustomFilterGlobalContext::createValidator(ShShaderSpec shaderSpec)

And maybe this one would be createShadersValidator?

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:105
> +bool CustomFilterValidatedProgram::validateMixVertexShader(const String& originalShader, String& validatedShader, String& validationLog)

bool CustomFilterValidatedProgram::validateMixVertexShader(const String& originalShader, String& validationLog) {
m_validatedVertexShader = ...;
}

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:128
> +bool CustomFilterValidatedProgram::validateMixFragmentShader(const String& originalShader, String& validatedShader, String& validationLog)

This is doing more than just validating the shader.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:147
> +        uniform sampler2D css_u_texture;

Can you add a comment about why css_u_texture cannot be used in user code?

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:-198
> -    m_compiledProgram = m_globalContext->getCompiledProgram(m_program->programInfo());

Why not keep the compiled program in this patch. It would remove a lot of changes in this patch.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:279
> +    CustomFilterCompiledProgram* compiledProgram = m_validatedProgram->compiledProgram().get();

Add an assert here to check that the program is initialized.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:322
> +    offset += bindVertexAttribute(compiledProgram->positionAttribLocation(), 4, offset);

Since you are refactoring this code, what about using some const values for the offsets instead:
static const unsigned PositionAttributeOffset = 0;
static const unsigned TextureAttributeOffset = PositionAttributeOffset + sizeof(float) * 4;
...

> Source/WebCore/rendering/FilterEffectRenderer.cpp:358
> +PassRefPtr<FECustomFilter> FilterEffectRenderer::createCustomFilterEffect(Document* document, CustomFilterOperation* operation)

Can this be static?

> Source/WebCore/rendering/FilterEffectRenderer.cpp:362
> +        return effect;

why not just return 0?

> Source/WebCore/rendering/FilterEffectRenderer.cpp:366
> +        return effect;

ditto

> Source/WebCore/rendering/FilterEffectRenderer.cpp:372
> +        return effect;

ditto

> Source/WebCore/rendering/FilterEffectRenderer.cpp:374
> +    effect = FECustomFilter::create(this, globalContext, validatedProgram, operation->parameters(),

This is the first place where you actually need the RefPtr<FECustomFilter>

> Source/WebCore/rendering/FilterEffectRenderer.cpp:377
> +    m_hasCustomShaderFilter = true;

Can you move this back to the caller and keep this function static?

> Source/WebCore/rendering/FilterEffectRenderer.h:45
> +#include "FECustomFilter.h"

There's no need to include this file header.

> Source/WebCore/rendering/FilterEffectRenderer.h:124
> +    PassRefPtr<FECustomFilter> createCustomFilterEffect(Document*, CustomFilterOperation*);

Keep this as a static function in the file.

[Max Vujovic]

Created attachment 160038 [details]
Patch

Thanks for the review, Alex. I've uploaded another patch.

(In reply to comment #13)
> View in context: https://bugs.webkit.org/attachment.cgi?id=159237&action=review
> 
> Saving first round of this review. Some small improvements.
> 
> > Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.cpp:50
> > +ANGLEWebKitBridge* CustomFilterGlobalContext::webglValidator()
> 
> I think naming this webGLShadersValidator() would make it more similar to cssShadersValidator(). I would also welcome an explanation about when we use webgl vs. css shaders.

I renamed them to webglShaderValidator() and mixShaderValidator(). I call it mixShaderValidator() because it's used to validate a special type of CSS Shader- a shader referenced from a CSS mix function. I've added some comments to describe this distinction. 

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.cpp:64
> > +PassOwnPtr<ANGLEWebKitBridge> CustomFilterGlobalContext::createValidator(ShShaderSpec shaderSpec)
> 
> And maybe this one would be createShadersValidator?

I renamed it to createShaderValidator (shader singular, not shaders plural).

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:105
> > +bool CustomFilterValidatedProgram::validateMixVertexShader(const String& originalShader, String& validatedShader, String& validationLog)
> 
> bool CustomFilterValidatedProgram::validateMixVertexShader(const String& originalShader, String& validationLog) {
> m_validatedVertexShader = ...;
> }

Good idea. See the next point below.

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:128
> > +bool CustomFilterValidatedProgram::validateMixFragmentShader(const String& originalShader, String& validatedShader, String& validationLog)
> 
> This is doing more than just validating the shader.

I've moved the validation into the CustomFilterValidatedProgram constructor. It selects a validator like this:
ANGLEWebKitBridge* validator = programInfo.mixSettings().enabled ? m_globalContext->mixShaderValidator() : m_globalContext->webglShaderValidator();

Then, it performs validation.

I've changed these functions to simply:
void rewriteMixVertexShader();
void rewriteMixFragmentShader();

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:147
> > +        uniform sampler2D css_u_texture;
> 
> Can you add a comment about why css_u_texture cannot be used in user code?

Good idea. Done.

> 
> > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:-198
> > -    m_compiledProgram = m_globalContext->getCompiledProgram(m_program->programInfo());
> 
> Why not keep the compiled program in this patch. It would remove a lot of changes in this patch.
> 

Done.

> > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:279
> > +    CustomFilterCompiledProgram* compiledProgram = m_validatedProgram->compiledProgram().get();
> 
> Add an assert here to check that the program is initialized.
> 

I've added an assert a little earlier in FECustomFilter::bindProgramAndBuffers, which is the only caller of this method (FECustomFilter::bindProgramParameters).

> > Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:322
> > +    offset += bindVertexAttribute(compiledProgram->positionAttribLocation(), 4, offset);
> 
> Since you are refactoring this code, what about using some const values for the offsets instead:
> static const unsigned PositionAttributeOffset = 0;
> static const unsigned TextureAttributeOffset = PositionAttributeOffset + sizeof(float) * 4;
> ...

Done. I've put the constants in the function for now. Ideally, they should be members of CustomFilterMesh because CustomFilterMesh uses the same values. I've add a FIXME and a created bug 94755. I think this should be done regardless of how we factor this particular function.

> 
> > Source/WebCore/rendering/FilterEffectRenderer.cpp:358
> > +PassRefPtr<FECustomFilter> FilterEffectRenderer::createCustomFilterEffect(Document* document, CustomFilterOperation* operation)
> 
> Can this be static?

Done. However, I do have to pass the "this" pointer because FECustomFilter::create takes in the FilterEffectRenderer. I think that's fine though.

> 
> > Source/WebCore/rendering/FilterEffectRenderer.cpp:362
> > +        return effect;
> 
> why not just return 0?

Done. I like that better.

> 
> > Source/WebCore/rendering/FilterEffectRenderer.cpp:366
> > +        return effect;
> 
> ditto

Done.

> 
> > Source/WebCore/rendering/FilterEffectRenderer.cpp:372
> > +        return effect;
> 
> ditto

Done.

> 
> > Source/WebCore/rendering/FilterEffectRenderer.cpp:374
> > +    effect = FECustomFilter::create(this, globalContext, validatedProgram, operation->parameters(),
> 
> This is the first place where you actually need the RefPtr<FECustomFilter>

Now, I don't make the RefPtr. I just do "return FECustomFilter::create(...);".

> 
> > Source/WebCore/rendering/FilterEffectRenderer.cpp:377
> > +    m_hasCustomShaderFilter = true;
> 
> Can you move this back to the caller and keep this function static?

Done.

> 
> > Source/WebCore/rendering/FilterEffectRenderer.h:45
> > +#include "FECustomFilter.h"
> 
> There's no need to include this file header.
> 

I need it because of this line in FilterEffectRenderer.h:
static PassRefPtr<FECustomFilter> createCustomFilterEffect(Filter*, Document*, CustomFilterOperation*);

> > Source/WebCore/rendering/FilterEffectRenderer.h:124
> > +    PassRefPtr<FECustomFilter> createCustomFilterEffect(Document*, CustomFilterOperation*);
> 
> Keep this as a static function in the file.

Done.

[Max Vujovic]

Created attachment 160065 [details]
Patch

Rebased patch.

[WebKit Review Bot]

Comment on attachment 160065 [details]
Patch

Attachment 160065 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13565571

New failing tests:
css3/filters/custom/effect-custom.html

[WebKit Review Bot]

Created attachment 160076 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Max Vujovic]

Created attachment 160084 [details]
Patch

EWS was right- my vertex attribute offset calculation was off. Let's try this again.

[Max Vujovic]

Created attachment 160277 [details]
Patch

A few more small changes related to Alex's review:

(In reply to comment #14)
> > > Source/WebCore/rendering/FilterEffectRenderer.cpp:377
> > > +    m_hasCustomShaderFilter = true;
> >
> > Can you move this back to the caller and keep this function static?
>
> Done.
>

Now I made createCustomFilterEffect() a static function in FilterEffectRenderer.cpp.

> >
> > > Source/WebCore/rendering/FilterEffectRenderer.h:45
> > > +#include "FECustomFilter.h"
> >
> > There's no need to include this file header.
> >
>
> I need it because of this line in FilterEffectRenderer.h:
> static PassRefPtr<FECustomFilter> createCustomFilterEffect(Filter*, Document*, CustomFilterOperation*);

Nevermind, you were right. The include is gone now.

[Max Vujovic]

Comment on attachment 160277 [details]
Patch

Bots are looking good. Setting r?, cq?.

[Dean Jackson]

Comment on attachment 160277 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add a test for normal blend mode mode and source-atop compositing mode using css_MixColor in
> +        the shader.
> +        Add a test using css_ColorMatrix in the shader.

I think we should add a single line here explaining that we've added these two new attribs/uniforms to the shaders. The description "normal blend mode and source-atop compositing" is probably not enough to someone just looking at this changelog (and not following the bug link).

> LayoutTests/css3/filters/custom/custom-filter-composite-source-atop.html:35
> +        /* The fragment shader writes a blue square to css_MixColor in the lower right corner of the destination div. */
> +        /* It writes transparent elsewhere. */
> +        -webkit-filter: custom(url('../resources/pass-tex-coord.vs') mix(url('../resources/composite.fs') normal source-atop));
> +    }
> +    /* Draws a yellow square in the upper left corner of the destination div. */
> +    /* The rest of the destination div is transparent. */

This is a cool test!

> Source/WebCore/ChangeLog:10
> +        Instead of allowing direct texture access in an author's shader via u_texture, CSS
> +        Shaders blends special symbols in the author's shader (css_MixColor and
> +        css_ColorMatrix) with the DOM element texture.

It's a shame that this makes testing outside the Web more complicated, but that's a spec issue rather than this patch.

> Source/WebCore/ChangeLog:21
> +        This patch introduces a new class, CustomFilterValidatedProgram. 
> +        CustomFilterValidatedProgram validates the shader using ANGLE. If the shader uses

" ... a new class, CustomFilterValidatedProgram, which validates the shader..."

> Source/WebCore/ChangeLog:31
> +        After validation, CustomFilterValidatedProgram adds blending, compositing, and
> +        texture access shader code to the author's original shaders. The definitions for
> +        css_MixColor and css_ColorMatrix are added before the author's fragment shader
> +        code so that the author code can access them. The blending, compositing, and

I assume it is still ok if the author includes them in their shader (as long as they are defined the same way)? I'm thinking of a non-Web test system for just the standalone shaders.

> Source/WebCore/ChangeLog:41
> +

This is a great ChangeLog. Thankyou!

> Source/WebCore/WebCore.gyp/WebCore.gyp:-1618
> -        ['exclude', 'platform/graphics/ANGLEWebKitBridge\\.(cpp|h)$'],

I assume this won't cause any issues in Chromium?

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:34
> -#elif !PLATFORM(GTK) && !PLATFORM(EFL) && !PLATFORM(BLACKBERRY)
> +#elif !PLATFORM(GTK) && !PLATFORM(EFL) && !PLATFORM(BLACKBERRY) && !PLATFORM(CHROMIUM)

This probably answers my question above :)

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:56
> -    bool validateShaderSource(const char* shaderSource, ANGLEShaderType shaderType, String& translatedShaderSource, String& shaderValidationLog, int extraCompileOptions);
> +    bool validateShaderSource(const char* shaderSource, ANGLEShaderType, String& translatedShaderSource, String& shaderValidationLog, int extraCompileOptions = 0);

Thanks. I probably should have done this in my original patch.

> Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.cpp:132
> +    m_internalTexAttribLocation = m_context->getAttribLocation(m_program, "css_a_texCoord");

m_internalTexCoordAttribLocation?

> Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.h:63
> +    int internalTexAttribLocation() const { return m_internalTexAttribLocation; }

Similarly, internalTexCoordAttribLocation() ?

> Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.cpp:62
> +ANGLEWebKitBridge* CustomFilterGlobalContext::webglShaderValidator()
> +{
> +    if (!m_webglShaderValidator)
> +        m_webglShaderValidator = createShaderValidator(SH_WEBGL_SPEC);
> +    return m_webglShaderValidator.get();
> +}
> +
> +ANGLEWebKitBridge* CustomFilterGlobalContext::mixShaderValidator()
> +{
> +    if (!m_mixShaderValidator)
> +        m_mixShaderValidator = createShaderValidator(SH_CSS_SHADERS_SPEC);
> +    return m_mixShaderValidator.get();
> +}

Is there any reason why we wouldn't always want to use the SH_CSS_SHADERS_SPEC validator, even if the shader doesn't use mix or the generated attribs/uniforms?

> Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.h:62
> +    // CSS shaders referenced from the CSS mix function should be validated slightly differently than WebGL shaders.
> +    // This ANGLE validator uses the SH_CSS_SHADERS_SPEC flag.
> +    // Under this flag, most notably:
> +    // - The "gl_FragColor" built-in is not available.
> +    // - Instead, the "css_MixColor" and "css_ColorMatrix" built-ins are available.

Not really an issue for this patch, but this answers my question above about always using the CSS shader validation. It would be really nice if gl_FragColor was auto-swappable with css_MixColor. Oh well.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:91
> +    if (!vertexShaderValid || !fragmentShaderValid)
> +        // FIXME: Report the validation errors.
> +        // https://bugs.webkit.org/show_bug.cgi?id=74416
> +        return;

Please add {} since you have multiline comments.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.h:77
> +    // 'detachGlobalContext' is called when the CustomFilterGlobalContext is deleted
> +    // and there's no need for the callback anymore. 

You mean 'detachFromGlobalContext'

[Dean Jackson]

Comment on attachment 160277 [details]
Patch

Oops. Removing cq+ in case Max wants to change things.

[Max Vujovic]

Comment on attachment 160277 [details]
Patch

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

>> LayoutTests/ChangeLog:10
>> +        Add a test using css_ColorMatrix in the shader.
> 
> I think we should add a single line here explaining that we've added these two new attribs/uniforms to the shaders. The description "normal blend mode and source-atop compositing" is probably not enough to someone just looking at this changelog (and not following the bug link).

Will do.

>> LayoutTests/css3/filters/custom/custom-filter-composite-source-atop.html:35
>> +    /* The rest of the destination div is transparent. */
> 
> This is a cool test!

Thanks!

>> Source/WebCore/ChangeLog:10
>> +        css_ColorMatrix) with the DOM element texture.
> 
> It's a shame that this makes testing outside the Web more complicated, but that's a spec issue rather than this patch.

Yeah, it is too bad.

There's no technical barrier to letting authors use gl_FragColor instead of css_MixColor in mix shaders. We just need some spec changes and ANGLE changes.

The original intention for using css_MixColor instead of gl_FragColor was to make it clear that an author is not writing a real pixel value. That particular functionality is intended for the pure GLSL shaders that don't use mix. One day, I hope we can add u_texture access back to the pure GLSL shaders, with the necessary restrictions in place. Then, I'm thinking most people will write non-mix, pure GLSL shaders. At that point, mix shaders will be syntactic sugar for certain types of effects that particularly lend themselves to using a color matrix or blending and compositing.

>> Source/WebCore/ChangeLog:21
>> +        CustomFilterValidatedProgram validates the shader using ANGLE. If the shader uses
> 
> " ... a new class, CustomFilterValidatedProgram, which validates the shader..."

That's better. Thanks.

>> Source/WebCore/ChangeLog:31
>> +        code so that the author code can access them. The blending, compositing, and
> 
> I assume it is still ok if the author includes them in their shader (as long as they are defined the same way)? I'm thinking of a non-Web test system for just the standalone shaders.

It's an error if the author defines css_MixColor or css_ColorMatrix explicitly, just like its an error if the author tries to define gl_FragColor. For example, this would be an error:
"vec4 css_MixColor = vec4(1.0);"

This is because css_MixColor and css_ColorMatrix are built-ins just like gl_FragColor, not uniforms or attributes.

>> Source/WebCore/ChangeLog:41
>> +
> 
> This is a great ChangeLog. Thankyou!

Welcome! :)

>> Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:34
>> +#elif !PLATFORM(GTK) && !PLATFORM(EFL) && !PLATFORM(BLACKBERRY) && !PLATFORM(CHROMIUM)
> 
> This probably answers my question above :)

Yup, I've tried it on Chromium mac. Also, the cr-linux bot seems happy linking Chromium's copy of ANGLE.

>> Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.cpp:132
>> +    m_internalTexAttribLocation = m_context->getAttribLocation(m_program, "css_a_texCoord");
> 
> m_internalTexCoordAttribLocation?

Thanks! Good eyes!

>> Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.h:63
>> +    int internalTexAttribLocation() const { return m_internalTexAttribLocation; }
> 
> Similarly, internalTexCoordAttribLocation() ?

Yup.

>> Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.h:62
>> +    // - Instead, the "css_MixColor" and "css_ColorMatrix" built-ins are available.
> 
> Not really an issue for this patch, but this answers my question above about always using the CSS shader validation. It would be really nice if gl_FragColor was auto-swappable with css_MixColor. Oh well.

By auto-swappable, do you mean if an author writes "gl_FragColor", then we pretend they wrote "css_MixColor" in mix shaders? That is, we make gl_FragColor and css_MixColor synonyms.

>> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:91
>> +        return;
> 
> Please add {} since you have multiline comments.

Will do.

>> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.h:77
>> +    // and there's no need for the callback anymore. 
> 
> You mean 'detachFromGlobalContext'

Yup, thanks!

[Max Vujovic]

Created attachment 161311 [details]
Patch for Landing

Added the changes from Dean's review. Rebased the patch.

[Max Vujovic]

Comment on attachment 161311 [details]
Patch for Landing

Setting cq-. I collided with http://trac.webkit.org/changeset/127046

[Max Vujovic]

Created attachment 161333 [details]
Patch for Landing

Rebased patch again.

[Max Vujovic]

Comment on attachment 161333 [details]
Patch for Landing

Setting cq?

[Alexandru Chiculita]

Comment on attachment 161333 [details]
Patch for Landing

Setting the CQ+.

[Max Vujovic]

Created attachment 161364 [details]
Patch for Landing

Let's try this again, and I want to make sure cr-linux is green before setting cq?.

I had an ASCIILiteral related macro expansion issue while merging the changes from: http://trac.webkit.org/changeset/127062

[Max Vujovic]

Created attachment 161410 [details]
Patch for Landing

Rebased again. The previous patch went out of date before cr-linux got to it.

[Max Vujovic]

Created attachment 161501 [details]
Patch for Landing

Rebased patch.

[Joshua Netterfield]

My patch depends on this. Is it safe to land now?

[Max Vujovic]

(In reply to comment #32)
> My patch depends on this. Is it safe to land now?

Yes- it looks safe. I'll ask Alex to set cq+.

[Alexandru Chiculita]

Comment on attachment 161501 [details]
Patch for Landing

Added CQ+

[WebKit Review Bot]

Comment on attachment 161501 [details]
Patch for Landing

Rejecting attachment 161501 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ram.cpp
patching file Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.h
patching file Source/WebCore/platform/graphics/filters/FECustomFilter.cpp
patching file Source/WebCore/platform/graphics/filters/FECustomFilter.h
patching file Source/WebCore/rendering/FilterEffectRenderer.cpp
patching file Source/WebCore/rendering/FilterEffectRenderer.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13682863

[Max Vujovic]

Created attachment 161557 [details]
Patch for Landing

Another collision. Merged and rebased patch. Setting cq? because we passed the cr-linux bot.

[WebKit Review Bot]

Comment on attachment 161557 [details]
Patch for Landing

Clearing flags on attachment: 161557

Committed r127217: <http://trac.webkit.org/changeset/127217>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.