Bug 130848

Summary: Remove comment from Filter.h
Product: WebKit Reporter: Adenilson Cavalcanti Silva <savagobr>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, d-r, fmalita, gyuyoung.kim, kondapallykalyan, krit, pdr, schenney, sergio, simon.fraser, zimmermann
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch none

Description Adenilson Cavalcanti Silva 2014-03-27 10:55:11 PDT 
As noticed in the TODO in #130779, it was possible to make the parameters in applyHorizontalScale()/VerticalScale() const refs if small changes were made on the derived classes (e.g. SVGFilter).
Comment 1 Adenilson Cavalcanti Silva 2014-03-27 11:02:40 PDT 
Created attachment 227960 [details]
Patch
Comment 2 Dirk Schulze 2014-03-27 11:51:32 PDT 
Comment on attachment 227960 [details]
Patch

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

Not sure why you are doing that. passing a reference to a float is not more or less expecive than passing the value itself. All the float references should be turned back to normal float arguments.

> Source/WebCore/platform/graphics/filters/Filter.h:50
> +    virtual float applyHorizontalScale(const float& value) const { return value * m_filterResolution.width(); }
> +    virtual float applyVerticalScale(const float& value) const { return value * m_filterResolution.height(); }

there is no need to make the float a reference, it doesn't save anything.

> Source/WebCore/svg/graphics/filters/SVGFilter.cpp:39
> +float SVGFilter::applyHorizontalScale(const float& value) const

Ditto.
Comment 3 Adenilson Cavalcanti Silva 2014-03-27 11:56:17 PDT 
Dirk

Thanks for the review. 

I have one question: const float would be ok?
Comment 4 Simon Fraser (smfr) 2014-03-27 12:26:59 PDT 
Comment on attachment 227960 [details]
Patch

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

>> Source/WebCore/platform/graphics/filters/Filter.h:50
>> +    virtual float applyVerticalScale(const float& value) const { return value * m_filterResolution.height(); }
> 
> there is no need to make the float a reference, it doesn't save anything.

Nor a const float. The const doesn't add anything.
Comment 5 Adenilson Cavalcanti Silva 2014-03-27 12:27:51 PDT 
Created attachment 227964 [details]
Patch
Comment 6 Adenilson Cavalcanti Silva 2014-03-27 12:35:33 PDT 
Gentlemen

Thanks for the review. I agree that it won't fly, so I'm just renaming the bug title and simply removing the TODO comment.
Comment 7 WebKit Commit Bot 2014-03-27 13:08:09 PDT 
Comment on attachment 227964 [details]
Patch

Clearing flags on attachment: 227964

Committed r166368: <http://trac.webkit.org/changeset/166368>
Comment 8 WebKit Commit Bot 2014-03-27 13:08:17 PDT 
All reviewed patches have been landed.  Closing bug.