Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
211513003:
Returning IntSize to store calculated kernel size values (Closed)
|
Created:
6 years, 9 months ago by Savago Modified:
6 years, 9 months ago CC:
blink-reviews, jamesr, krit, dsinclair, jbroman, danakj, Rik, Stephen Chennney, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionReturning IntSize to store calculated kernel size values
allows to have less function paramenters. This patch also
streamlines a bit the code in calculateUnscaledKernelSize.
BUG=356329
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170195
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressing reviewer's comments #Patch Set 3 : FloatPoint parameter #
Total comments: 4
Patch Set 4 : Remove const float from Filter.h #
Messages
Total messages: 22 (0 generated)
LGTM with nits https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... File Source/platform/graphics/filters/FEDropShadow.cpp (right): https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FEDropShadow.cpp:65: IntSize kernelSize(0, 0); Is this needed? https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... File Source/platform/graphics/filters/FEGaussianBlur.cpp (right): https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FEGaussianBlur.cpp:47: static const int gMaxKernelSize = 1000; Curious: Why int? Can this be changed to unsigned and "int size" below changed to "unsigned size"? https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FEGaussianBlur.cpp:307: IntSize kernelSize(0, 0); Is this needed? https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... File Source/platform/graphics/filters/FEGaussianBlur.h (right): https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FEGaussianBlur.h:44: static IntSize calculateUnscaledKernelSize(FloatPoint std); Can these functions take const references instead? static IntSize calculateKernelSize(const Filter&, float stdX, float stdY); static IntSize calculateUnscaledKernelSize(const FloatPoint&); https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... File Source/platform/graphics/filters/FilterOperations.cpp (right): https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FilterOperations.cpp:37: IntSize kernelSize(0, 0); Is this needed?
https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... File Source/platform/graphics/filters/FEDropShadow.cpp (right): https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FEDropShadow.cpp:65: IntSize kernelSize(0, 0); Just checked IntSize() and it already initializes its members to 0. Indeed, I can remove it. https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... File Source/platform/graphics/filters/FEGaussianBlur.cpp (right): https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FEGaussianBlur.cpp:47: static const int gMaxKernelSize = 1000; IntSize.setWidth/setHeight() operates on integers. It would require casting, so the path of less resistance is to change the const gMaxKernelSize to become an int. https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... File Source/platform/graphics/filters/FEGaussianBlur.h (right): https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FEGaussianBlur.h:44: static IntSize calculateUnscaledKernelSize(FloatPoint std); Indeed, they can. I will update the patch.
On 2014/03/26 00:12:23, Savago wrote: > https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... > File Source/platform/graphics/filters/FEDropShadow.cpp (right): > > https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... > Source/platform/graphics/filters/FEDropShadow.cpp:65: IntSize kernelSize(0, 0); > Just checked IntSize() and it already initializes its members to 0. Indeed, I > can remove it. > Why not write: IntSize kernelSize = FEGaussianBlur::calculateKernelSize(filter, m_stdX, m_stdY);?
(Some of these are covered by pdr's comments; I forgot to send this earlier.) https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... File Source/platform/graphics/filters/FEGaussianBlur.cpp (right): https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FEGaussianBlur.cpp:262: IntSize kernelSize(0, 0); No point in explicitly initializing this if you're going to assign it on the next line. Please combine the declaration with the next line. https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FEGaussianBlur.cpp:307: IntSize kernelSize(0, 0); Same here. https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... File Source/platform/graphics/filters/FEGaussianBlur.h (right): https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FEGaussianBlur.h:43: static IntSize calculateKernelSize(Filter*, float stdX, float stdY); Nit: it seems inconsistent for calculateUnscaledKernelSize to take a FloatPoint, but not calculateKernelSize. https://codereview.chromium.org/211513003/diff/1/Source/platform/graphics/fil... Source/platform/graphics/filters/FEGaussianBlur.h:44: static IntSize calculateUnscaledKernelSize(FloatPoint std); This should probably be const FloatPoint&.
Stephen Thanks for the review. I agree concerning the FloatPoint parameter, will upload a new patch ASAP.
On 2014/03/26 01:23:51, Savago wrote: > Stephen > > Thanks for the review. I agree concerning the FloatPoint parameter, will upload > a new patch ASAP. I have one question though: in FEGaussianRect::mapRect(), calculateKernelSize() is invoked passing as parameter the private data members m_stdX/stdY. Would make sense to turn them into FloatPoint instead? It could avoid the use of a temporary.
LGTM On 2014/03/26 01:34:11, Savago wrote: > I have one question though: in FEGaussianRect::mapRect(), calculateKernelSize() > is invoked passing as parameter the private data members m_stdX/stdY. Would make > sense to turn them into FloatPoint instead? It could avoid the use of a > temporary. Sure, sounds reasonable. Not required, up to you.
The CQ bit was checked by a.cavalcanti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.cavalcanti@samsung.com/211513003/180001
Post-commit nits. https://codereview.chromium.org/211513003/diff/180001/Source/platform/graphic... File Source/platform/graphics/filters/FEGaussianBlur.cpp (right): https://codereview.chromium.org/211513003/diff/180001/Source/platform/graphic... Source/platform/graphics/filters/FEGaussianBlur.cpp:253: FloatPoint stdError(filter->applyHorizontalScale(std.x()), filter->applyVerticalScale(std.y())); I wonder if we should just have an applyScale() that takes and returns a FloatPoint. There are probably other places we could use that. https://codereview.chromium.org/211513003/diff/180001/Source/platform/graphic... File Source/platform/graphics/filters/Filter.h (right): https://codereview.chromium.org/211513003/diff/180001/Source/platform/graphic... Source/platform/graphics/filters/Filter.h:64: virtual float applyHorizontalScale(const float value) const I think these are just noise. Might just be my opinion, though.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on win_blink_compile_dbg
https://codereview.chromium.org/211513003/diff/180001/Source/platform/graphic... File Source/platform/graphics/filters/FEGaussianBlur.cpp (right): https://codereview.chromium.org/211513003/diff/180001/Source/platform/graphic... Source/platform/graphics/filters/FEGaussianBlur.cpp:253: FloatPoint stdError(filter->applyHorizontalScale(std.x()), filter->applyVerticalScale(std.y())); I can look on that. https://codereview.chromium.org/211513003/diff/180001/Source/platform/graphic... File Source/platform/graphics/filters/Filter.h (right): https://codereview.chromium.org/211513003/diff/180001/Source/platform/graphic... Source/platform/graphics/filters/Filter.h:64: virtual float applyHorizontalScale(const float value) const I agree, it is a left over where I tried to pass by const ref (but it would require to change some other classes in SVG/rendering IIRC).
The CQ bit was checked by a.cavalcanti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.cavalcanti@samsung.com/211513003/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ bit was checked by a.cavalcanti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.cavalcanti@samsung.com/211513003/210001
Ok, I did some investigation on the 2 red bots: a) Linux has a failing test (audiobuffersource-multi-channels.html) which both doesn't use any css3/svg filter code and is known for being flaky. b) Android is failing thanks to a missing Skia header at [554]. The files I touched are in the range [203] to [218] and are compiling fine: http://build.chromium.org/p/tryserver.chromium/builders/blink_android_compile... Thus, I will mark the patch to be cq-ed again.
Message was sent while issue was closed.
Change committed as 170195 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
