Improve ellipse path generation
Created attachment 69887 [details] Patch
The attached patch removes some multiplications and divisions from an inner loop in svg's ellipse path generation, replacing them with a single addition of a pre-calculated delta. This provides a 5-10% increase in frame-rate on themaninblue's svg performance test (http://www.themaninblue.com/writing/perspective/2010/03/22/). This is a short-term fix that will be superseded by some of the changes discussed in 30994, 44374, and 46052. However, this change only affects a handful of layout tests and is hence reasonably easy to commit.
Created attachment 69892 [details] Patch
Comment on attachment 69892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69892&action=review This doesn't change any pixel results? Also did you update to the latest DRT updates on trunk? Something went wrong with applying the patch to the bots. > WebCore/platform/graphics/Path.cpp:235 > + static const unsigned step = 100; Statics should be at the top, right after the header includes. please name it something like gCircleFragments. > WebCore/platform/graphics/Path.cpp:240 > + float delta = 2.0 * piFloat / static_cast<float>(step); > + for (unsigned i = 1; i < step; i++) { float delta = 2 * .... ++i instead of i++
Created attachment 70042 [details] Patch
Attachment 70042 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 LayoutTests/platform/chromium/test_expectations.txt:3159: Duplicate expectation. svg/W3C-SVG-1.1/animate-elem-80-t.svg [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3172: Duplicate expectation. svg/clip-path/clip-in-mask-objectBoundingBox.svg [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3173: Duplicate expectation. svg/clip-path/clip-in-mask-userSpaceOnUse.svg [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3182: Duplicate expectation. svg/filters/filter-clip.svg [test/expectations] [5] Total errors found: 4 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 70042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70042&action=review The algorithm looks good. But I guess you have to take git-svn or something like that to create patches that can be handled by the bots. You also did not answer the question. Did you update to the latest results? Does this really not change pixel tests? > WebCore/platform/graphics/Path.cpp:39 > static const float QUARTER = 0.552f; // approximation of control point positions on a bezier > // to simulate a quarter of a circle. When you are on it, can you rename QUARTER please? Should match the naming schema of consts (Beginning with a g). Quarter is not really meaningful. Maybe you find a better name. And align the comments please > WebCore/platform/graphics/Path.cpp:42 > +// number of path segments used to approximate circles and ellipses. > +static const unsigned gEllipseFragments = 100; Not sure if we need the comment at all. The name says all.
(In reply to comment #6) > Attachment 70042 [details] did not pass style-queue: > > Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 > LayoutTests/platform/chromium/test_expectations.txt:3159: Duplicate expectation. svg/W3C-SVG-1.1/animate-elem-80-t.svg [test/expectations] [5] > LayoutTests/platform/chromium/test_expectations.txt:3172: Duplicate expectation. svg/clip-path/clip-in-mask-objectBoundingBox.svg [test/expectations] [5] > LayoutTests/platform/chromium/test_expectations.txt:3173: Duplicate expectation. svg/clip-path/clip-in-mask-userSpaceOnUse.svg [test/expectations] [5] > LayoutTests/platform/chromium/test_expectations.txt:3182: Duplicate expectation. svg/filters/filter-clip.svg [test/expectations] [5] > Total errors found: 4 in 36 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. What does it mean?
Created attachment 70044 [details] Patch
Dirk - sorry, I was trying to get the style checker passing before getting you to review again. I guess I should be clearing the review flag until the patch is ready to go? At any rate, the duplicate expectations are now fixed. In response to your earlier question about pixel tests, there aren't any webkit pixel-by-pixel comparisons, and the lines in test_expectations indicate that the chromium build should temporarily expect these tests to fail. Once this patch lands I can extract the new images from our buildbots and remove those lines from test_expectations as a separate patch. I'll respond to your latest set of change requests now and re-upload.
Created attachment 70053 [details] Patch
Argh, sorry someone *just* got in a bunch of changes to all the test expectations. It'll take me a while to fix this merge conflict, and it's getting late here. I'll do it tomorrow morning, Australian time.
Created attachment 70172 [details] Patch
Anders and I took a look at this and we are a bit confused about why the client needs to know about the viewport at all. Couldn't WebKit handle this itself (maybe if it was put into a special mode where it knew to handle viewport args)?
(In reply to comment #14) > Anders and I took a look at this and we are a bit confused about why the client needs to know about the viewport at all. Couldn't WebKit handle this itself (maybe if it was put into a special mode where it knew to handle viewport args)? Ignore this, wrong bug.
Closing this bug as 69517 fixes the problem.
Comment on attachment 70172 [details] Patch clearing flags