Bug 137911 – Const-ify static s_resourceType members in RenderSVGResource* classes

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

Const-ify static s_resourceType members in RenderSVGResource* classes

[Zan Dobersek]

Created attachment 240191 [details]
Patch

[Andreas Kling]

Comment on attachment 240191 [details]
Patch

r=me

[Zan Dobersek]

Created attachment 240341 [details]
Patch for landing

Running through the EWS once more.

[Zan Dobersek]

Comment on attachment 240341 [details]
Patch for landing

Removing the r? flag since this was already reviewed.

[Zan Dobersek]

Comment on attachment 240341 [details]
Patch for landing

Clearing flags on attachment: 240341

Committed r175117: <http://trac.webkit.org/changeset/175117>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 240341 [details]
Patch for landing

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

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.h:57
> +    static const RenderSVGResourceType s_resourceType = ClipperResourceType;

Is s_resourceType needed? One way to get rid of it would be to make the virtual function final, or the entire class final. Then, despite the fact that it’s virtual, calling the inline final function would be just as efficient as getting at a data member with a similar name and we could omit the data member entirely.

If we didn't make the resourceType function final, then we’d want to make it private to help prevent clients from accidentally calling a virtual function to get something that’s known at compile time.

I don’t think the s_ prefix is not something we should be using in WebKit. Annoys me that we were using it here.

[Zan Dobersek]

(In reply to comment #7)
> Comment on attachment 240341 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240341&action=review
> 
> > Source/WebCore/rendering/svg/RenderSVGResourceClipper.h:57
> > +    static const RenderSVGResourceType s_resourceType = ClipperResourceType;
> 
> Is s_resourceType needed? One way to get rid of it would be to make the
> virtual function final, or the entire class final. Then, despite the fact
> that it’s virtual, calling the inline final function would be just as
> efficient as getting at a data member with a similar name and we could omit
> the data member entirely.
> 

The s_resourceType constant is only ever referenced in this template:
https://trac.webkit.org/browser/trunk/Source/WebCore/rendering/svg/RenderSVGResource.h#L70

I think it should be possible to remove these constants and this cast<>() template in favor of downcast<>(). I created bug #138290 for that.