| Summary: | SVG: Remove "create" methods and use port-specific "add" counterparts | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||||||||||
| Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | abarth, darin, eric, jeffschiller, krit, ossy, webkit.review.bot, zimmermann | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | All | ||||||||||||||||||
| OS: | All | ||||||||||||||||||
| Bug Depends on: | 46051 | ||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Andreas Kling
2010-09-19 09:25:53 PDT
Created attachment 68030 [details]
Proposed patch (not to be landed before 46051 is resolved)
Note that this patch will require re-baselining of many tests, so I won't be landing it until 46051 is resolved.
Comment on attachment 68030 [details]
Proposed patch (not to be landed before 46051 is resolved)
r=me, as soon as the DRT bug is resolved first.
Created attachment 70238 [details]
Proposed patch v2
New patch as discussed on IRC.
I've removed all the static Path::create* methods and replaced their usage with Path::add*
Comment on attachment 70238 [details] Proposed patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=70238&action=review Good first try, looking forward to the next version. > WebCore/platform/graphics/Path.cpp:121 > if (width <= 0.0f || height <= 0.0f) Please use if (width <= 0 || height <= 0). If there are more cases of ".f" postfixes in this file, please kill them. > WebCore/platform/graphics/Path.cpp:133 > if (dy > height * 0.5f) > dy = height * 0.5f; s/0.5f/0.5/ > WebCore/platform/graphics/Path.cpp:140 > + addBezierCurveTo(FloatPoint(x + width - dx * (1 - QUARTER), y), FloatPoint(x + width, y + dy * (1 - QUARTER)), FloatPoint(x + width, y + dy)); QUARTER has an evil name, please rename it to something sensible, and use the "g" prefix. > WebCore/platform/graphics/Path.cpp:164 > if (width <= 0.0 || height <= 0.0) if (width <= 0 || height <= 0) > WebCore/platform/graphics/Path.cpp:204 > +#if 0 No #if 0 code please remove it alltogether, or is there a difference between addRect and addRectangle? > WebCore/platform/graphics/Path.h:147 > + void addRoundedRectangle(const FloatRect&, const FloatSize& roundingRadii); Can you move them below the existing add methods? > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:706 > + for (unsigned i = 0; i < rectCount; i++) { While you're at it s/i++/++i/ > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1282 > + { Why do you wrap this in braces? > WebCore/rendering/RenderBoxModelObject.cpp:1756 > + Path path; Declare the Path path; outside of hasBorderRadius and... > WebCore/rendering/RenderBoxModelObject.cpp:1767 > + Path path; ... reuse it hear (with a path.clear() before)... > WebCore/rendering/RenderBoxModelObject.cpp:1776 > + Path path; ...here... > WebCore/rendering/RenderBoxModelObject.cpp:1780 > + Path path; ...and here... > WebCore/svg/SVGCircleElement.cpp:131 > + path.addEllipse(FloatRect(cx().value(this) - radius, I wouldn't wrap the lines here. > WebCore/svg/SVGEllipseElement.cpp:135 > + float radiusX = rx().value(this); Micro optimization: immediately check if (radiusX <= 0) return;. No need to call ry().value() in that case. > WebCore/svg/SVGEllipseElement.cpp:141 > + path.addEllipse(FloatRect(cx().value(this) - radiusX, I wouldn't wrap the lines here. > WebCore/svg/SVGRectElement.cpp:154 > FloatRect rect(x().value(this), y().value(this), width().value(this), height().value(this)); > > + if (rect.width() <= 0 || rect.height() <= 0) This is flawed, please use: float widthValue = width().value(this); if (widthValue <= 0) return; float heightValue = height().value(this); if (heightValue <= 0) return; float xValue = x().value(this); float yValue = y().value(this); > WebCore/svg/SVGRectElement.cpp:160 > float _rx = hasRx ? rx().value(this) : ry().value(this); if (hasRx || hasRy) { float rxValue = rx().value(this); float ryValue = ry().value(this); if (!hasRx) rxValue = ryValue; else if (!hasRy) ryValue = rxValue; .... } This should be sightly faster. > WebCore/svg/SVGStyledTransformableElement.cpp:110 > Path SVGStyledTransformableElement::toClipPath() const toClipPath should also take a Path& parameter. > WebCore/svg/SVGStyledTransformableElement.h:50 > + virtual void toPathData(Path&) const {} Please use { } instead of {}. Created attachment 70355 [details]
Proposed patch v4
Updated patch addressing all of Niko's comments.
Attachment 70355 [details] did not build on mac: Build output: http://queues.webkit.org/results/4360008 Comment on attachment 70355 [details]
Proposed patch v4
Broke MathML, new patch coming..
Created attachment 70370 [details]
Proposed patch v5
Removed Path::create* usage in MathML code.
Also removed the relevant symbols from WebCore.order
Attachment 70370 [details] did not build on mac: Build output: http://queues.webkit.org/results/4365003 (In reply to comment #9) > Attachment 70370 [details] did not build on mac: > Build output: http://queues.webkit.org/results/4365003 Don't know how to make sense of this log, and it builds fine on a Mac here. Comment on attachment 70370 [details] Proposed patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=70370&action=review Almost there, still giving r- as it's not perfect yet. > WebCore/ChangeLog:5 > + SVG: Leverage platform's Path::addEllipse() in Path::createEllipse() The bug title should be changed, you're modifying more than addEllipse. > WebCore/platform/graphics/Path.cpp:80 > + static const float rad2deg = 180.0 / piFloat; Ok, this is unrelated, but please remove thie rad2deg static and use the one in wtf/MathExtras.h > WebCore/platform/graphics/Path.cpp:128 > + if (dx > width * 0.5) I think it makes sense to declare: float halfWidth = width / 2; if (dx > halfWidth) dx = halfWidth; As width * 0.5 is used several times below., Same for height * 0.5 Just a little optimization. > WebCore/platform/graphics/Path.cpp:141 > + addBezierCurveTo(FloatPoint(x + width - dx * (1 - gCircleControlPoint), y), FloatPoint(x + width, y + dy * (1 - gCircleControlPoint)), FloatPoint(x + width, y + dy)); If we're always using 1 - gCircleControlPoint, that should go into the static directly. so it read FloatPoint(x + width - dx * gCircle... > WebCore/platform/graphics/Path.cpp:161 > +void Path::addRoundedRectangle(const FloatRect& rectangle, const FloatSize& topLeftRadius, const FloatSize& topRightRadius, const FloatSize& bottomLeftRadius, const FloatSize& bottomRightRadius) For consistency, this should be renamed addRoundedRect. Or rename addRect to addRectangle. > WebCore/platform/graphics/Path.cpp:183 > + addLineTo(FloatPoint(x + width, y + height - bottomRightRadius.height())); I'd add a newline before this addLineTo line, and split the upper bezier curveTo in two lines, addBezierCurveTo(FloatPoint(x + width, ..., y), FloatPoint(x + width, ....), (aligning the FloatPoints > WebCore/platform/graphics/Path.cpp:185 > + addLineTo(FloatPoint(x + bottomLeftRadius.width(), y + height)); Ditto. > WebCore/platform/graphics/Path.cpp:187 > + addLineTo(FloatPoint(x, y + topLeftRadius.height())); Ditto. > WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:243 > + { Why this extra brace? > WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:707 > + Path path; How about declaring this before the loop, and using if (i > 0) path.clear() here ? > WebCore/rendering/RenderBoxModelObject.cpp:1766 > + path.clear(); You only need the clear, if (hasBorderRadius), avoids an unncessary call. > WebCore/rendering/svg/SVGInlineTextBox.cpp:496 > + if (fragment.width > 0 && thickness > 0) I'd change this: if (fragment.width <= 0 || thickness <= 0) return; And move the float y computation etc. below this check. Early exit if there's nothing to do. > WebCore/svg/SVGAnimateMotionElement.cpp:118 > + return path; Evil Path copying.... but not something you should change now. Created attachment 70431 [details]
Proposed patch v6
Updated patch, addressing all of Niko's comments.
Attachment 70431 [details] did not build on mac: Build output: http://queues.webkit.org/results/4328023 Comment on attachment 70431 [details] Proposed patch v6 View in context: https://bugs.webkit.org/attachment.cgi?id=70431&action=review I'm not sure if we should add an ASSERT to all toClipPath as well as toPathData if the path is empty. (not sure how platforms implemented it, but they should check the number of segments internally). Maybe we can trust the programmer to never call this functions with a none empty path ;-) These are just some notes. > WebCore/platform/graphics/Path.cpp:112 > +void Path::addRoundedRect(const FloatRect& rectangle, const FloatSize& roundingRadii) You should use rect instead of rectangle for the variable, at least if you change the function name. > WebCore/platform/graphics/Path.cpp:117 > float x = rectangle.x(); > float y = rectangle.y(); > float width = rectangle.width(); > float height = rectangle.height(); Why not FloatRect rect(rectangle) so maybe leave the name above. > WebCore/platform/graphics/Path.cpp:119 > float rx = roundingRadii.width(); > float ry = roundingRadii.height(); Can't we use FloatSize radius here? If we copy all data, we might just use void Path::addRoundedRect(FloatRect rect, FloatSize roundingRadii) instead of void Path::addRoundedRect(const FloatRect& rectangle, const FloatSize& roundingRadii). What do you think? > WebCore/platform/graphics/Path.cpp:126 > + float dx = rx; > + float dy = ry; > + float halfWidth = width / 2; > + float halfHeight = height / 2; Maybe we can use FloatSize for the 4 values as well. *** Bug 44375 has been marked as a duplicate of this bug. *** Created attachment 70451 [details]
Proposed patch v7
Addressed Dirk's comments. Didn't add the ASSERTs though, since Path::isEmpty() is very slow on some platforms (CG, ?) and this would slow down debug builds.
(In reply to comment #16) > Didn't add the ASSERTs though, since Path::isEmpty() is very slow on some platforms (CG, ?) and this would slow down debug builds. Normally we include assertions even if they would slow down debug builds. Is the slowdown so dramatic it causes a major problem? Comment on attachment 70451 [details] Proposed patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=70451&action=review > WebCore/ChangeLog:9 > + Circles and ellipses will be significantly faster on platforms that > + implement Path::addEllipse() in a sane fashion. Can you mention, that you avoid copying paths? > WebCore/platform/graphics/Path.cpp:118 > + float dx = roundingRadii.width(); > + float dy = roundingRadii.height(); Why not FloatSize radius= roundingRadii; ? > WebCore/platform/graphics/Path.cpp:120 > + float halfWidth = rect.width() / 2; > + float halfHeight = rect.height() / 2; You could use FloatSize here as well. But it is also ok to use two floats. (In reply to comment #17) > (In reply to comment #16) > > Didn't add the ASSERTs though, since Path::isEmpty() is very slow on some platforms (CG, ?) and this would slow down debug builds. > > Normally we include assertions even if they would slow down debug builds. Is the slowdown so dramatic it causes a major problem? We tried to eliminate most calls to Path::isEmpty() in the past, as at least on CG CGPathIsEmpty is quite slow, at least it showed up on shark profiles quite often in the past. But as it's an assertion, that's not present in release builds, maybe it's not so dramatic after all. (In reply to comment #19) > We tried to eliminate most calls to Path::isEmpty() in the past, as at least on CG CGPathIsEmpty is quite slow, at least it showed up on shark profiles quite often in the past. Note that if CGPathIsEmpty was showing up as a bottleneck in a way that’s difficult to avoid, we could possibly change the augment CG implementation of Path to cache additional state so that in most cases a call to CGPathIsEmpty would not be necessary. Attachment 70451 [details] did not build on mac: Build output: http://queues.webkit.org/results/4368010 (In reply to comment #20) > (In reply to comment #19) > > We tried to eliminate most calls to Path::isEmpty() in the past, as at least on CG CGPathIsEmpty is quite slow, at least it showed up on shark profiles quite often in the past. > > Note that if CGPathIsEmpty was showing up as a bottleneck in a way that’s difficult to avoid, we could possibly change the augment CG implementation of Path to cache additional state so that in most cases a call to CGPathIsEmpty would not be necessary. That's right, a simple boolean would be sufficient. Let's add the assertions and see if it's still a problem, if so, we'll just cache a "bool m_isEmpty" variable in PathCg. Comment on attachment 70451 [details] Proposed patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=70451&action=review Please add the path isEmpty assertion in toPathData, an let's find out why it doesn't build on mac... > WebCore/platform/graphics/Path.cpp:132 > + moveTo(FloatPoint(rect.x() + dx, rect.y())); Please cache rect.x() / y() / width() / height() in local variables. > WebCore/platform/graphics/Path.cpp:162 > + if (rect.width() < topLeftRadius.width() + topRightRadius.width() Please cache rect.x() / y() / width() / height() in local variables. (In reply to comment #23) > (From update of attachment 70451 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=70451&action=review > > Please add the path isEmpty assertion in toPathData, an let's find out why it doesn't build on mac... > > > WebCore/platform/graphics/Path.cpp:132 > > + moveTo(FloatPoint(rect.x() + dx, rect.y())); > > Please cache rect.x() / y() / width() / height() in local variables. > > > WebCore/platform/graphics/Path.cpp:162 > > + if (rect.width() < topLeftRadius.width() + topRightRadius.width() > > Please cache rect.x() / y() / width() / height() in local variables. I take those back, as rect.x() etc. are inline function calls. Created attachment 70460 [details]
Proposed patch v8
Added the ASSERTs.
Didn't cache x(), y() etc in locals though since they're inline functions (as discussed on IRC.)
Comment on attachment 70460 [details] Proposed patch v8 View in context: https://bugs.webkit.org/attachment.cgi?id=70460&action=review > WebCore/platform/graphics/Path.cpp:117 > + FloatSize radius = roundingRadii; Use FloatSize radius(roundingRadii); > WebCore/svg/SVGStyledTransformableElement.cpp:118 > + ASSERT(path.isEmpty()); > + > + toPathData(path); You're calling ASSERT(path.isEmpty()); in toPathData, no need to assert twice. Please change this before landing. Great patch! r=me. Committed r69517: <http://trac.webkit.org/changeset/69517> Attachment 70460 [details] did not build on mac: Build output: http://queues.webkit.org/results/4275026 *** Bug 30994 has been marked as a duplicate of this bug. *** http://trac.webkit.org/changeset/69517 might have broken Qt Linux Release (In reply to comment #30) > http://trac.webkit.org/changeset/69517 might have broken Qt Linux Release Qt specifix expected file updated in http://trac.webkit.org/changeset/69565 |