Bug 75778

Created attachment 139364 [details] Patch

Comment on attachment 139364 [details] Patch Attachment 139364 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12558638

Comment on attachment 139364 [details] Patch Attachment 139364 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12555673

Created attachment 139365 [details] Patch

Comment on attachment 139365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139365&action=review Can't say much about the OpenGL code, but it looks alright. The rest are mostly nitpicks. > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:541 > + if (!filter) > + continue; In which case can this happen? Ideally this shouldn't be there. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:212 > m_textureMapperShaderProgramMap.clear(); > +#if ENABLE(CSS_FILTERS) > + m_filterMap.clear(); > +#endif Wouldn't those be implicit anyway? > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:66 > +#if USE(UI_SIDE_COMPOSITING) && ENABLE(CSS_FILTERS) > +#include <WebCore/FilterOperations.h> > +#endif Could you limit the guards in WebCoreArgumentCoders to ENABLE(CSS_FILTERS) only? The Animation coders aren't guarded, you also wouldn't need to guard the include in that case.

> > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:66 > > +#if USE(UI_SIDE_COMPOSITING) && ENABLE(CSS_FILTERS) > > +#include <WebCore/FilterOperations.h> > > +#endif > > Could you limit the guards in WebCoreArgumentCoders to ENABLE(CSS_FILTERS) only? > The Animation coders aren't guarded, you also wouldn't need to guard the include in that case. The animation coders are guarded with PLATFORM(QT), to smfr's request. The above guard seems reasonable. If another ports wants to unguard them, they can go ahead.

(In reply to comment #6) > The animation coders are guarded with PLATFORM(QT), to smfr's request. The above guard seems reasonable. If another ports wants to unguard them, they can go ahead. Ok I didn't see that, makes sense.

Created attachment 139796 [details] Patch

Comment on attachment 139796 [details] Patch (In reply to comment #5) > > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:212 > > m_textureMapperShaderProgramMap.clear(); > > +#if ENABLE(CSS_FILTERS) > > + m_filterMap.clear(); > > +#endif > > Wouldn't those be implicit anyway? Any comment on this? For the rest, r=me

> > Wouldn't those be implicit anyway? > > Any comment on this? > For the rest, r=me Oh, thought I'd removed them. I'll remove this before committing.

Created attachment 139806 [details] Patch for landing

Comment on attachment 139806 [details] Patch for landing Rejecting attachment 139806 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebKit2/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/12595809

Created attachment 139812 [details] Patch for Landing

Comment on attachment 139812 [details] Patch for Landing Clearing flags on attachment: 139812 Committed r115838: <http://trac.webkit.org/changeset/115838>

All reviewed patches have been landed. Closing bug.

Comment on attachment 139812 [details] Patch for Landing View in context: https://bugs.webkit.org/attachment.cgi?id=139812&action=review Sorry for adding comments so late. I'm not a reviewer, so feel free to ignore them :) > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:512 > + RefPtr<StandardFilterProgram> program = data().sharedGLData().textureMapperShaderManager.getShaderForFilter(filter); Not an issue right now, but I suppose you will need multiple passes for the blur and drop-shadow (ie. vertical, horizontal) > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:521 > + glUniform1i(program->textureUniform(), 0); Some GL commands have GL_CMD and some don't. Is there a reason not to use it here? > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:214 > + glDetachShader(m_id, m_vertexShader); Do you need to make the GL context current here? What thread is going to run the destructor? I suppose it's the same as the one for ~TextureMapperShaderManager. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:345 > + glGetShaderInfoLog(fragmentShader, 100, &len, log); The "log" seems to be required only for debugging reasons. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:418 > + program->prepare(filter); It seems weird to call prepare in a method called "getShaderForFilter".

(In reply to comment #16) > (From update of attachment 139812 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139812&action=review > > Sorry for adding comments so late. I'm not a reviewer, so feel free to ignore them :) Already committed, but you comments are always welcome! > > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:512 > > + RefPtr<StandardFilterProgram> program = data().sharedGLData().textureMapperShaderManager.getShaderForFilter(filter); > > Not an issue right now, but I suppose you will need multiple passes for the blur and drop-shadow (ie. vertical, horizontal) Yes. The idea would be that this function would get a "pass" number. > > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:521 > > + glUniform1i(program->textureUniform(), 0); > > Some GL commands have GL_CMD and some don't. Is there a reason not to use it here? Just an oversight, no reason :) > > > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:214 > > + glDetachShader(m_id, m_vertexShader); > > Do you need to make the GL context current here? What thread is going to run the destructor? I suppose it's the same as the one for ~TextureMapperShaderManager. Yes. TextureMapperGL is only created and destroyed in a thread where the context is available. > > > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:345 > > + glGetShaderInfoLog(fragmentShader, 100, &len, log); > > The "log" seems to be required only for debugging reasons. Oh, forgot it in. > > > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:418 > > + program->prepare(filter); > > It seems weird to call prepare in a method called "getShaderForFilter". I think we need to refactor TextureMapperShaderManager at some point... not sure how to go about it.

Reopening to attach new patch.

Created attachment 139836 [details] Patch for landing

Comment on attachment 139836 [details] Patch for landing Clearing flags on attachment: 139836 Committed r115855: <http://trac.webkit.org/changeset/115855>

All reviewed patches have been landed. Closing bug.