Each FECustomFilter is creating its own GraphicsContext3D and always compiles the Shader. There should be only one GraphicsContext3D for all the FECustomFilters and they should share the precompiled shaders.
Created attachment 145173 [details] Patch V1
Comment on attachment 145173 [details] Patch V1 Attachment 145173 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12861493 New failing tests: http/tests/media/media-source/video-media-source-event-attributes.html
Created attachment 145205 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #3) > Created an attachment (id=145205) [details] > Archive of layout-test-results from ec2-cr-linux-01 > > The attached test failures were seen while running run-webkit-tests on the chromium-ews. > Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick This seems to be flaky. I've seen it on other bugs.
Comment on attachment 145173 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=145173&action=review I think it looks ok. I'd like to get some more hints about what things are coming in the future so it doesn't look like you're just adding a single function that you'll remove later. I'm hesitant to mark r+ because I worry about adding the new inheritance to FrameView. > Source/WebCore/ChangeLog:13 > + OwnPtr<CustomFilterHost>. Currently it only contains a single function that returns the HostWindow required > + to create GraphicsContext3D objects. Could you mention what things will be added in the future? > Source/WebCore/WebCore.xcodeproj/project.pbxproj:1930 > + 7E99AF510B13846468FB01A5 /* WindowFocusAllowedIndicator.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 7E99AF520B13846468FB01A5 /* WindowFocusAllowedIndicator.cpp */; }; > + 7E99AF530B13846468FB01A5 /* WindowFocusAllowedIndicator.h in Headers */ = {isa = PBXBuildFile; fileRef = 7E99AF540B13846468FB01A5 /* WindowFocusAllowedIndicator.h */; settings = {ATTRIBUTES = (Private, ); }; }; Oops. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:-5431 > - 7E99AF510B13846468FB01A5 /* WindowFocusAllowedIndicator.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 7E99AF520B13846468FB01A5 /* WindowFocusAllowedIndicator.cpp */; }; > - 7E99AF530B13846468FB01A5 /* WindowFocusAllowedIndicator.h in Headers */ = {isa = PBXBuildFile; fileRef = 7E99AF540B13846468FB01A5 /* WindowFocusAllowedIndicator.h */; settings = {ATTRIBUTES = (Private, ); }; }; Oops. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:9009 > + 7E99AF520B13846468FB01A5 /* WindowFocusAllowedIndicator.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = WindowFocusAllowedIndicator.cpp; sourceTree = "<group>"; }; > + 7E99AF540B13846468FB01A5 /* WindowFocusAllowedIndicator.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WindowFocusAllowedIndicator.h; sourceTree = "<group>"; }; Oops. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:-12570 > - 7E99AF520B13846468FB01A5 /* WindowFocusAllowedIndicator.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = WindowFocusAllowedIndicator.cpp; sourceTree = "<group>"; }; > - 7E99AF540B13846468FB01A5 /* WindowFocusAllowedIndicator.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WindowFocusAllowedIndicator.h; sourceTree = "<group>"; }; Oops. > Source/WebCore/page/FrameView.h:61 > -class FrameView : public ScrollView { > +class FrameView : public ScrollView > +#if ENABLE(CSS_SHADERS) && ENABLE(WEBGL) > + , public CustomFilterHostHandler > +#endif I'd like someone who knows more about FrameView to make sure this is ok.
Comment on attachment 145173 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=145173&action=review Thanks! I will update the patch and ask someone else for the FrameView. >> Source/WebCore/ChangeLog:13 >> + to create GraphicsContext3D objects. > > Could you mention what things will be added in the future? Sure. >> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-5431 >> - 7E99AF530B13846468FB01A5 /* WindowFocusAllowedIndicator.h in Headers */ = {isa = PBXBuildFile; fileRef = 7E99AF540B13846468FB01A5 /* WindowFocusAllowedIndicator.h */; settings = {ATTRIBUTES = (Private, ); }; }; > > Oops. Sorry, I usually check the patch for such things, but this time I missed it. xCode usually doesn't do that though.
Comment on attachment 145173 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=145173&action=review > Source/WebCore/platform/graphics/filters/CustomFilterHost.h:39 > +class CustomFilterHost { "FilterHost" is a pretty nondescript name. It's really hard to tell what this object does from that name. What does it mean to "host" a filter?
Comment on attachment 145173 [details] Patch V1 I'm working on a fix.
Created attachment 146067 [details] Patch V2
Comment on attachment 146067 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=146067&action=review I'm interested to see what you think about changing the name, so I'm not marking as cq+. As I read more of the code, I don't think this new class controls anything, so Controller is the wrong name. But I didn't have any good ideas. > Source/WebCore/ChangeLog:8 > + An object called CustomFilterController is created the first time a new FECustomFilter is needed in a document. I wonder if CustomFilterController is the best name. It's not really controlling anything - it's just holding the objects that are needed to process the custom filter. It could almost be called CustomFilterGlobalContext or something, because it holds things related to drawing and tied to the context that it creates. > Source/WebCore/platform/graphics/filters/CustomFilterController.h:34 > +#include <wtf/PassRefPtr.h> Do you need this include? > Source/WebCore/platform/graphics/filters/CustomFilterController.h:50 > + void prepareContextIfNeeded(HostWindow*); > +private: Need a space between here.
(In reply to comment #10) > (From update of attachment 146067 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146067&action=review Thanks for the review! > I'm interested to see what you think about changing the name, so I'm not marking as cq+. As I read more of the code, I don't think this new class controls anything, so Controller is the wrong name. But I didn't have any good ideas. Yes, maybe CustomFilterGlobalContext makes more sense. It will also keep the precompiled shaders around, so CustomFilterGlobalContext makes more sense for that. > > Source/WebCore/platform/graphics/filters/CustomFilterController.h:34 > > +#include <wtf/PassRefPtr.h> > > Do you need this include? Oops, I will check that before committing. > > Source/WebCore/platform/graphics/filters/CustomFilterController.h:50 > > + void prepareContextIfNeeded(HostWindow*); > > +private: > > Need a space between here. Ok.
Created attachment 148409 [details] Patch to commit
Comment on attachment 148409 [details] Patch to commit Clearing flags on attachment: 148409 Committed r120757: <http://trac.webkit.org/changeset/120757>
All reviewed patches have been landed. Closing bug.
*** Bug 85013 has been marked as a duplicate of this bug. ***