Bug 76026

Created attachment 121981 [details] Patch

Comment on attachment 121981 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121981&action=review > Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:204 > +typedef QRgb (*PixelFilterFunction)(QRgb, const double&); Is there an advantage/reason for passing const double& when many the calling conventions on many architectures allow for passing of doubles in registers? (SSE or integer register pair on ARM IIRC) > Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:316 > + static const PixelFilterFunction functions[] = {0, grayscale, sepia, saturate, hueRotate, invert, opacity, brightness, contrast }; Coding style: missing space after { > Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:317 > + PixelFilterFunction function = functions[type]; Perhaps it would make sense to have an ASSERT to ensure that type stays within the bounds of the functions array, as FilterOperation::OperationType may be re-ordered at any time.

> Is there an advantage/reason for passing const double& when many the calling conventions on many architectures allow for passing of doubles in registers? (SSE or integer register pair on ARM IIRC) Was following same style as in the software implementation of filters... I'll revise. > > Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:317 > > + PixelFilterFunction function = functions[type]; > > Perhaps it would make sense to have an ASSERT to ensure that type stays within the bounds of the functions array, as FilterOperation::OperationType may be re-ordered at any time. OK.

Comment on attachment 121981 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121981&action=review > Source/WebCore/ChangeLog:9 > + Also, added a fully working software implementation in the Qt backend of TextureMapper.r spelling issue at the end > Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:215 > + return qRgba( > + red * (0.2126 + 0.7874 * (1. - amount)) + green * (0.7152 - 0.7152 * (1. - amount)) + blue * (0.0722 - 0.0722 * (1. - amount)), > + red * (0.2126 - 0.2126 * (1. - amount)) + green * (0.7152 + 0.2848 * (1. - amount)) + blue * (0.0722 - 0.0722 * (1. - amount)), > + red * (0.2126 - 0.2126 * (1. - amount)) + green * (0.7152 - 0.7152 * (1. - amount)) + blue * (0.0722 + 0.9278 * (1. - amount)), > + qAlpha(color) any comment about what these numbers represents? > Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:360 > +// This is a "trick" algorithm for fast gaussian blur. It's not as accurate as FEGaussianBlur, > +// but can be potentially faster. > +static void quickBlur(QImage& image, const double& stdDeviation) Maybe this is not the right place for it then? > Source/WebCore/platform/graphics/texmap/TextureMapperFilterRenderer.cpp:2 > + Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies) Old code :) 2012 right?

(In reply to comment #4) > any comment about what these numbers represents? Will do. They're taken directly from the W3C filters spec. > > > Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:360 > > +// This is a "trick" algorithm for fast gaussian blur. It's not as accurate as FEGaussianBlur, > > +// but can be potentially faster. > > +static void quickBlur(QImage& image, const double& stdDeviation) > > Maybe this is not the right place for it then? Not sure what you mean.

Comment on attachment 121981 [details] Patch Removing review flags until all of the texture-mapper refactors are in.

Created attachment 127174 [details] Patch

Attachment 127174 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource Index mismatch: 883c7fb9c78a1d23f9131b69f7fa908cd9778764 != 1fd67dff9ac4ff9ce56307043a73020a333c7b89 rereading 35ee22447c69292de5be6ed45c2410bff7ce3dca M LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug11026-expected.png M LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug10565-expected.png M LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug1188-expected.png M LayoutTests/platform/chromium/test_expectations.txt A LayoutTests/platform/chromium-mac-snowleopard/tables/mozilla/bugs/bug11026-expected.png A LayoutTests/platform/chromium-mac-snowleopard/tables/mozilla/bugs/bug10565-expected.png M LayoutTests/platform/chromium-mac-snowleopard/tables/mozilla/bugs/bug1188-expected.png D LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug11026-expected.png D LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug10565-expected.png M LayoutTests/platform/chromium-mac-leopard/tables/mozilla/bugs/bug11026-expected.png M LayoutTests/platform/chromium-mac-leopard/tables/mozilla/bugs/bug10565-expected.png M LayoutTests/platform/chromium-mac-leopard/tables/mozilla/bugs/bug1188-expected.png M LayoutTests/platform/chromium-win/tables/mozilla/bugs/bug1188-expected.png M LayoutTests/platform/chromium-win/tables/mozilla/bugs/bug11026-expected.png M LayoutTests/platform/chromium-win/tables/mozilla/bugs/bug10565-expected.png M LayoutTests/ChangeLog W: -empty_dir: trunk/LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug10565-expected.png W: -empty_dir: trunk/LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug11026-expected.png 107807 = cd492113759b3acf3e51434935a37ee8efc9ac66 already exists! Why are we refetching it? at /usr/lib/git-core/git-svn line 5210 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 127174 [details] Patch Clearing flags on attachment: 127174 Committed r107814: <http://trac.webkit.org/changeset/107814>

All reviewed patches have been landed. Closing bug.

I had to update the expected results - http://trac.webkit.org/changeset/107898 I assume you committed Mac results, but the reference platform is Linux, not Mac. :(