Summary: | Const-ify static s_resourceType members in RenderSVGResource* classes | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zan Dobersek <zan> | ||||||
Component: | New Bugs | Assignee: | Zan Dobersek <zan> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, darin, dino, d-r, esprehn+autocc, fmalita, glenn, gyuyoung.kim, kondapallykalyan, pdr, schenney, sergio | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Zan Dobersek
2014-10-21 00:24:51 PDT
Created attachment 240191 [details]
Patch
Comment on attachment 240191 [details]
Patch
r=me
Created attachment 240341 [details]
Patch for landing
Running through the EWS once more.
Comment on attachment 240341 [details]
Patch for landing
Removing the r? flag since this was already reviewed.
Comment on attachment 240341 [details] Patch for landing Clearing flags on attachment: 240341 Committed r175117: <http://trac.webkit.org/changeset/175117> All reviewed patches have been landed. Closing bug. 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. (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. |