Bug 87878 – [CSS Shaders] Create a shared object between FECustomFilter objects to store the shared resources

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alexandru Chiculita]

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.

[Alexandru Chiculita]

Created attachment 145173 [details]
Patch V1

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Alexandru Chiculita]

(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.

[Dean Jackson]

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.

[Alexandru Chiculita]

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.

[Simon Fraser (smfr)]

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?

[Alexandru Chiculita]

Comment on attachment 145173 [details]
Patch V1

I'm working on a fix.

[Alexandru Chiculita]

Created attachment 146067 [details]
Patch V2

[Dean Jackson]

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.

[Alexandru Chiculita]

(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.

[Alexandru Chiculita]

Created attachment 148409 [details]
Patch to commit

[WebKit Review Bot]

Comment on attachment 148409 [details]
Patch to commit

Clearing flags on attachment: 148409

Committed r120757: <http://trac.webkit.org/changeset/120757>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexandru Chiculita]

*** Bug 85013 has been marked as a duplicate of this bug. ***