Bug 47030 – Clean up resources when LayerRenderer changes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Victoria Kirst]

Clean up resources when LayerRenderer changes

[Victoria Kirst]

Created attachment 69545 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 69545 [details]
Patch

Looks fine although it will collide with : https://bugs.webkit.org/show_bug.cgi?id=46765   Should we merge the two into a single checkin?

[Hin-Chung Lam]

I'd let her checkin first and I'll merge later.

[James Robinson]

Comment on attachment 69545 [details]
Patch

This is copy-paste from ContentLayerChromium so we should move this logic up to the base class.  Specifically, add a virtual cleanupResources() function to LayerChromium and have LayerChromium::setLayerRenderer() invoke it.

[Victoria Kirst]

Created attachment 69875 [details]
Patch

[Victoria Kirst]

(In reply to comment #3)
> I'd let her checkin first and I'll merge later.
Alpha actually already committed his linked patch (http://trac.webkit.org/changeset/69163), so my patch here has been modified (slightly) to fit with the current code.


(In reply to comment #4)
> (From update of attachment 69545 [details])
> This is copy-paste from ContentLayerChromium so we should move this logic up to the base class.  Specifically, add a virtual cleanupResources() function to LayerChromium and have LayerChromium::setLayerRenderer() invoke it.

Thanks for the tip! I added this and updated the related files. Take another look?

[James Robinson]

Comment on attachment 69875 [details]
Patch

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

R=me.  Please kill the extra ; in the header - you can either do that while landing, or upload another patch and I'll r+/cq+

> WebCore/platform/graphics/chromium/LayerChromium.h:208
> +    virtual void cleanupResources() { };

no semicolon needed here

[Victoria Kirst]

(In reply to comment #7)
> > WebCore/platform/graphics/chromium/LayerChromium.h:208
> > +    virtual void cleanupResources() { };
> 
> no semicolon needed here

Removed. Thanks so much for reviewing, James! Committed by Alpha here:
http://trac.webkit.org/changeset/69166

[Vangelis Kokkevis]

Comment on attachment 69875 [details]
Patch

Sorry I'm late to this.  I like the idea of making cleanupResources a virtual.  How about calling it from the LayerChromium destructor as well instead of having to call it from each derived class destructor?