Bug 137911 - Const-ify static s_resourceType members in RenderSVGResource* classes
Summary: Const-ify static s_resourceType members in RenderSVGResource* classes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-21 00:24 PDT by Zan Dobersek
Modified: 2014-11-02 05:42 PST (History)
12 users (show)

See Also:

Attachments
Patch (14.54 KB, patch)
2014-10-21 00:29 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Patch for landing (12.94 KB, patch)
2014-10-23 04:04 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2014-10-21 00:24:51 PDT 
Const-ify static s_resourceType members in RenderSVGResource* classes
Comment 1 Zan Dobersek 2014-10-21 00:29:40 PDT 
Created attachment 240191 [details]
Patch
Comment 2 Andreas Kling 2014-10-21 13:58:30 PDT 
Comment on attachment 240191 [details]
Patch

r=me
Comment 3 Zan Dobersek 2014-10-23 04:04:38 PDT 
Created attachment 240341 [details]
Patch for landing

Running through the EWS once more.
Comment 4 Zan Dobersek 2014-10-23 04:06:00 PDT 
Comment on attachment 240341 [details]
Patch for landing

Removing the r? flag since this was already reviewed.
Comment 5 Zan Dobersek 2014-10-23 05:20:41 PDT 
Comment on attachment 240341 [details]
Patch for landing

Clearing flags on attachment: 240341

Committed r175117: <http://trac.webkit.org/changeset/175117>
Comment 6 Zan Dobersek 2014-10-23 05:20:54 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 7 Darin Adler 2014-10-28 09:21:18 PDT 
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.
Comment 8 Zan Dobersek 2014-11-02 05:42:16 PST 
(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.