SVG does not need the full generality of the TransformationMatrix class and could benefit from the AffineTransform class being added back and being used by SVG.
Do you have any performance data to show that TransformationMatrix is an issue?
Created attachment 46721 [details] Stress test We came to this conclusion looking at the attached stress test with 50,000 rects. It seems every RenderPath has at least one TransformationMatrix.
It is planned to introduce 3D transformations to SVG 2.0 (http://www.w3.org/TR/2009/WD-SVG-Transforms-20090320/). Even if we concentrate on SVG 1.1 at the moment, we might want to implement it later. Thus we should let TransformationMatrix in and try to optimize the use of it. olliej did allready a great step forward with bug 33766 and bug 30055.
Created attachment 47771 [details] reimplementation of AffineTransformation After a discussion on IRC, all agreed to reimplement AffineTransform in a platform independent way. This is the first and implements AffineTransform and let Canvas make use of it. Next patches will introduce AffineTransform to SVG.
Attachment 47771 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Last 3072 characters of output: bCore/platform/graphics/transforms/AffineTransform.cpp:74: More than one command on the same line [whitespace/newline] [4] WebCore/platform/graphics/transforms/AffineTransform.cpp:75: More than one command on the same line [whitespace/newline] [4] WebCore/platform/graphics/transforms/AffineTransform.cpp:76: More than one command on the same line [whitespace/newline] [4] WebCore/platform/graphics/transforms/AffineTransform.cpp:119: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/transforms/AffineTransform.cpp:120: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/transforms/AffineTransform.cpp:130: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/transforms/AffineTransform.cpp:136: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/qt/TransformationMatrixQt.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/cg/TransformationMatrixCG.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/html/canvas/CanvasRenderingContext2D.h:30: Alphabetical sorting problem. [build/include_order] [4] WebCore/platform/graphics/wx/TransformationMatrixWx.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/html/canvas/CanvasRenderingContext2D.cpp:369: Missing spaces around / [whitespace/operators] [3] WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:26: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:50: cairo_matrix_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:54: Extra space before ( in function call [whitespace/parens] [4] WebCore/platform/graphics/transforms/AffineTransform.h:30: Should have a space between // and comment [whitespace/comments] [4] WebCore/platform/graphics/transforms/AffineTransform.h:126: Missing spaces around *= [whitespace/operators] [3] WebCore/platform/graphics/transforms/AffineTransform.h:143: cairo_matrix_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 27 If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 47771 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/222034
Created attachment 47795 [details] reimplementation of AffineTransformation build fix and some fixes of the style issues mentioned by the bot.
Attachment 47795 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Last 3072 characters of output: ontextSkia.cpp:821: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:822: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:823: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/skia/TransformationMatrixSkia.cpp:31: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/cairo/PathCairo.cpp:333: c_matrix is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/graphics/transforms/AffineTransform.cpp:118: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/transforms/AffineTransform.cpp:119: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/transforms/AffineTransform.cpp:129: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/transforms/AffineTransform.cpp:135: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/qt/TransformationMatrixQt.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/cg/TransformationMatrixCG.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/wx/TransformationMatrixWx.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:26: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:50: cairo_matrix_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:54: Extra space before ( in function call [whitespace/parens] [4] WebCore/platform/graphics/transforms/AffineTransform.h:130: Missing spaces around *= [whitespace/operators] [3] WebCore/platform/graphics/transforms/AffineTransform.h:147: cairo_matrix_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 19 If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 47795 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/223435
Created attachment 47797 [details] reimplementation of AffineTransformation grrr... typo in GraphicsContextSkia
Attachment 47797 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Last 3072 characters of output: ontextSkia.cpp:821: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:822: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:823: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/skia/TransformationMatrixSkia.cpp:31: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/cairo/PathCairo.cpp:333: c_matrix is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/graphics/transforms/AffineTransform.cpp:118: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/transforms/AffineTransform.cpp:119: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/transforms/AffineTransform.cpp:129: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/transforms/AffineTransform.cpp:135: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/qt/TransformationMatrixQt.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/cg/TransformationMatrixCG.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/wx/TransformationMatrixWx.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:26: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:50: cairo_matrix_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:54: Extra space before ( in function call [whitespace/parens] [4] WebCore/platform/graphics/transforms/AffineTransform.h:130: Missing spaces around *= [whitespace/operators] [3] WebCore/platform/graphics/transforms/AffineTransform.h:147: cairo_matrix_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 19 If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #11) > Attachment 47797 [details] did not pass style-queue: I won't fix the mentioned style issues with the following reasons: > > Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 > Last 3072 characters of output: > ontextSkia.cpp:821: One space before end of line comments > [whitespace/comments] [5] > WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:822: One space before > end of line comments [whitespace/comments] [5] > WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:823: One space before > end of line comments [whitespace/comments] [5] Looks like a style decission of the chromium guys and it makes sense here for a better code reading. > WebCore/platform/graphics/skia/TransformationMatrixSkia.cpp:31: Found other > header before a header this file implements. Should be: config.h, primary > header, blank line, and then alphabetically sorted. [build/include_order] [4] AffineTransform is also a primary header. The names of TransformationMatrix*.cpp might change to AffineTransform*.cpp later. > WebCore/platform/graphics/cairo/PathCairo.cpp:333: c_matrix is incorrectly > named. Don't use underscores in your identifier names. [readability/naming] Gtk port uses underscores for variables. > [4] > WebCore/platform/graphics/transforms/AffineTransform.cpp:118: Tests for > true/false, null/non-null, and zero/non-zero should all be done without > equality comparisons. [readability/comparison_to_zero] [5] > WebCore/platform/graphics/transforms/AffineTransform.cpp:119: Tests for > true/false, null/non-null, and zero/non-zero should all be done without > equality comparisons. [readability/comparison_to_zero] [5] > WebCore/platform/graphics/transforms/AffineTransform.cpp:129: Tests for > true/false, null/non-null, and zero/non-zero should all be done without > equality comparisons. [readability/comparison_to_zero] [5] > WebCore/platform/graphics/transforms/AffineTransform.cpp:135: Tests for > true/false, null/non-null, and zero/non-zero should all be done without > equality comparisons. [readability/comparison_to_zero] [5] It's necessary to check for 0 here, since values are allowed to be negative. > WebCore/platform/graphics/qt/TransformationMatrixQt.cpp:27: Found other header > before a header this file implements. Should be: config.h, primary header, > blank line, and then alphabetically sorted. [build/include_order] [4] see above > WebCore/platform/graphics/cg/TransformationMatrixCG.cpp:27: Found other header > before a header this file implements. Should be: config.h, primary header, > blank line, and then alphabetically sorted. [build/include_order] [4] see above > WebCore/platform/graphics/wx/TransformationMatrixWx.cpp:27: Found other header > before a header this file implements. Should be: config.h, primary header, > blank line, and then alphabetically sorted. [build/include_order] [4] see above > WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:26: Found other > header before a header this file implements. Should be: config.h, primary > header, blank line, and then alphabetically sorted. [build/include_order] [4] see above > WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:50: > cairo_matrix_t is incorrectly named. Don't use underscores in your identifier > names. [readability/naming] [4] Style guide of the gtk port. > WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:54: Extra space > before ( in function call [whitespace/parens] [4] Style decission of the gtk port. > WebCore/platform/graphics/transforms/AffineTransform.h:130: Missing spaces > around *= [whitespace/operators] [3] Found it that way in all other headers for operator like FloatPoint, Size, Rect as well as TransformationMatrix and others. Maybe also a false positive? > WebCore/platform/graphics/transforms/AffineTransform.h:147: cairo_matrix_t is > incorrectly named. Don't use underscores in your identifier names. > [readability/naming] [4] see above > Total errors found: 19 > > > If any of these errors are false positives, please file a bug against > check-webkit-style.
> Looks like a style decission of the chromium guys and it makes sense here for a > better code reading. I can't speak to the other issues, but the chromium port is intended to be 100% in compliance with the WebKit style guide. Any deviations are errors and should be corrected as convenient.
(In reply to comment #13) > > Looks like a style decission of the chromium guys and it makes sense here for a > > better code reading. > > I can't speak to the other issues, but the chromium port is intended to be 100% > in compliance with the WebKit style guide. Any deviations are errors and > should be corrected as convenient. Did you take a look at the affected code part? I have no problem to fix it with an upcomming patch, but it makes more sense to have more spaces here.
> Did you take a look at the affected code part? I have no problem to fix it with > an upcomming patch, but it makes more sense to have more spaces here. Situations like that are actually called out specifically in the Google Style Guide (which agrees with the WebKit Style Guide on this point). I don't mean to be the style enforcer. I mostly wanted to make that comment for future reference. Other platforms (e.g., Gtk, Qt) do use different style in parts of WebKit to match their respective platforms. In the case of Chromium, we've changed the platform to match WebKit style (which is one reason it took a while to upstream the "glue" layer). If you'd like to remove the whitespace, that would be great. If you feel like other things are more deserving of your attention, that's fine too. :)
(In reply to comment #15) > > Did you take a look at the affected code part? I have no problem to fix it with > > an upcomming patch, but it makes more sense to have more spaces here. > > Situations like that are actually called out specifically in the Google Style > Guide (which agrees with the WebKit Style Guide on this point). > > I don't mean to be the style enforcer. I mostly wanted to make that comment > for future reference. Other platforms (e.g., Gtk, Qt) do use different style > in parts of WebKit to match their respective platforms. In the case of > Chromium, we've changed the platform to match WebKit style (which is one reason > it took a while to upstream the "glue" layer). > > If you'd like to remove the whitespace, that would be great. If you feel like > other things are more deserving of your attention, that's fine too. :) Ok, thanks for the clarification. :-)
Comment on attachment 47797 [details] reimplementation of AffineTransformation Clearing review flag. Patch landed in http://trac.webkit.org/changeset/54112
(In reply to comment #17) > (From update of attachment 47797 [details]) > Clearing review flag. Patch landed in http://trac.webkit.org/changeset/54112 WebCore.vcproj missed a </File> line, and Windows build has the following error even if we fixed WebCore.vcproj. We'll roll this back. 7> Creating library e:\cg\src\third_party\WebKit\WebKitBuild\lib\WebKit.lib an d object e:\cg\src\third_party\WebKit\WebKitBuild\lib\WebKit.exp 7>LINK : warning LNK4199: /DELAYLOAD:QuartzCore.dll ignored; no imports found fr om QuartzCore.dll 7>LINK : warning LNK4199: /DELAYLOAD:QuartzCoreInterface.dll ignored; no imports found from QuartzCoreInterface.dll 7>WebCore_debug.lib(GraphicsContextCG.obj) : error LNK2019: unresolved external symbol "public: void __thiscall WebCore::GraphicsContextPlatformPrivate::concatC TM(class WebCore::AffineTransform const &)" (?concatCTM@GraphicsContextPlatformP rivate@WebCore@@QAEXABVAffineTransform@2@@Z) referenced in function "public: voi d __thiscall WebCore::GraphicsContext::concatCTM(class WebCore::AffineTransform const &)" (?concatCTM@GraphicsContext@WebCore@@QAEXABVAffineTransform@2@@Z) 7>e:\cg\src\third_party\WebKit\WebKitBuild\bin\WebKit.dll : fatal error LNK1120: 1 unresolved externals
Created attachment 47815 [details] reimplementation of AffineTransformation fixes windows build.
Attachment 47815 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Last 3072 characters of output: ontextSkia.cpp:821: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:822: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:823: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/skia/TransformationMatrixSkia.cpp:31: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/cairo/PathCairo.cpp:333: c_matrix is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/graphics/transforms/AffineTransform.cpp:118: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/transforms/AffineTransform.cpp:119: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/transforms/AffineTransform.cpp:129: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/transforms/AffineTransform.cpp:135: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/qt/TransformationMatrixQt.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/cg/TransformationMatrixCG.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/wx/TransformationMatrixWx.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:26: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:50: cairo_matrix_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/graphics/cairo/TransformationMatrixCairo.cpp:54: Extra space before ( in function call [whitespace/parens] [4] WebCore/platform/graphics/transforms/AffineTransform.h:130: Missing spaces around *= [whitespace/operators] [3] WebCore/platform/graphics/transforms/AffineTransform.h:147: cairo_matrix_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 19 If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #20) > Attachment 47815 [details] did not pass style-queue: See comment https://bugs.webkit.org/show_bug.cgi?id=33750#c12 .
Attachment 47815 [details] was posted by a committer and has review+, assigning to Dirk Schulze for commit.
Created attachment 48307 [details] Add back AffineTransform for SVG The complete patch to get AffineTransform back in action. I also upload the patch to check if it builds on all platforms. I'll let the r? online, if builds pass.
Comment on attachment 47815 [details] reimplementation of AffineTransformation patch landed in r54137.
Attachment 48307 [details] did not pass style-queue: Failed to run "['git', 'reset', '--hard', 'HEAD']" exit_code: 128 fatal: Could not reset index file to revision 'HEAD'. If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 48307 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/244189
Sorry about the botspam. I think the bots were having some trouble.
(In reply to comment #26) > Attachment 48307 [details] did not build on gtk: > Build output: http://webkit-commit-queue.appspot.com/results/244189 I'm pretty sure, that it works on gtk. (Tested it with trunk on my own system) I also run check-webkit-style. I got warnings, all about == 0 checks. They were found in AffineTransform and are neccessary there.
Created attachment 48349 [details] Add back AffineTransform for SVG update was needed after Nikos DRT patch.
Attachment 48349 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/graphics/transforms/AffineTransform.h:120: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/graphics/skia/ImageSkia.cpp:33: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/svg/SVGTransformDistance.cpp:254: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 3 If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #30) > Attachment 48349 [details] did not pass style-queue: > > Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 > WebCore/platform/graphics/transforms/AffineTransform.h:120: Tests for > true/false, null/non-null, and zero/non-zero should all be done without > equality comparisons. [readability/comparison_to_zero] [5] > WebCore/platform/graphics/skia/ImageSkia.cpp:33: Found other header before a > header this file implements. Should be: config.h, primary header, blank line, > and then alphabetically sorted. [build/include_order] [4] > WebCore/svg/SVGTransformDistance.cpp:254: Tests for true/false, null/non-null, > and zero/non-zero should all be done without equality comparisons. > [readability/comparison_to_zero] [5] > Total errors found: 3 > > > If any of these errors are false positives, please file a bug against > check-webkit-style. see also comment 28
Comment on attachment 48349 [details] Add back AffineTransform for SVG Looks fine, r=me. Please change the static_cast<float> to narrowPrecisionToFloat, as discussed on IRC.
Comment on attachment 48349 [details] Add back AffineTransform for SVG landed in http://trac.webkit.org/changeset/54503 . Build fixes for chromium and win were added in 54504, 54509