Bug 130848 - Remove comment from Filter.h
Summary: Remove comment from Filter.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-27 10:55 PDT by Adenilson Cavalcanti Silva
Modified: 2014-03-27 13:08 PDT (History)
12 users (show)

See Also:


Attachments
Patch (4.46 KB, patch)
2014-03-27 11:02 PDT, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Patch (1.44 KB, patch)
2014-03-27 12:27 PDT, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.