According to http://www.w3.org/2003/01/REC-SVG11-20030114-errata#stroking-subpaths-zero-length a zero-length path with stroke-linecap="square" should result in a square being rendered.
There doesn't seem to be an easy way to get CG to draw stroke caps in this way, so I assume some preprocessing of the path data will have to be done before handing it off to CG to render.
This is probably well into the "edge case" realm. But a bug all the same. Now if the W3C actually had a test case...
Speaking of test case, Microsoft seems to have put one together: http://samples.msdn.microsoft.com/ietestcenter/svg112/svg_harness.htm?url=./svg112/svg/chapter_11.4.svg
*** Bug 61243 has been marked as a duplicate of this bug. ***
Created attachment 100809 [details] Patch
Comment on attachment 100809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100809&action=review Good catch, some comments: > Source/WebCore/rendering/svg/RenderSVGPath.cpp:173 > + Path* path = &m_path; > + Path tempPath; I don't see the need here for these two. Just leave "Path path" as is. > Source/WebCore/rendering/svg/RenderSVGPath.cpp:180 > + // Spec: any zero length subpath shall not be stroked if the âstroke-linecapâ property has a value of butt s/any/Any/. A link to the paragraph would be even better (plus your text). > Source/WebCore/rendering/svg/RenderSVGPath.cpp:184 > + path = &tempPath; When you leave "Path path" above, that line is obsolete. > Source/WebCore/rendering/svg/RenderSVGPath.cpp:196 > + tempPath = m_path; > + path = &tempPath; > + tempPath.transform(nonScalingStrokeTransform); You can revert this is as well. > Source/WebCore/rendering/svg/RenderSVGPath.cpp:327 > + if (svgStyle->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) { m_fillBoundingBox.isEmpty() ? > Source/WebCore/rendering/svg/RenderSVGPath.cpp:332 > + Path path; > + SVGElement* svgElement = static_cast<SVGElement*>(node()); > + float strokeWidth = svgStyle->strokeWidth().value(svgElement); > + path.addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth)); > + m_strokeAndMarkerBoundingBox = path.boundingRect(); Why do you need a temp rect for this? Just use the already calculatd Rect?
Comment on attachment 100809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100809&action=review > Source/WebCore/rendering/svg/RenderSVGPath.cpp:174 > + int applyMode = ApplyToStrokeMode; I think Rob don't want to change the original path. This is necessary for JS calls like pathLength() and similar calls. But have to look at the SVG code first if this is needed. >> Source/WebCore/rendering/svg/RenderSVGPath.cpp:327 >> + if (svgStyle->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) { > > m_fillBoundingBox.isEmpty() ? Thats not the same, isEmpty is true for values smaller than 0. In our case this is unimportant than the rect never gets negative.
Hi Dirk, (In reply to comment #7) > (From update of attachment 100809 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100809&action=review > > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:174 > > + int applyMode = ApplyToStrokeMode; > > I think Rob don't want to change the original path. This is necessary for JS calls like pathLength() and similar calls. But have to look at the SVG code first if this is needed. Agreed. >>> Source/WebCore/rendering/svg/RenderSVGPath.cpp:327 >>> + if (svgStyle->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) { >> >> m_fillBoundingBox.isEmpty() ? > > Thats not the same, isEmpty is true for values smaller than 0. In our case this is unimportant than the rect never gets negative. Also isEmpty uses ||, where we want && for the width and height is zero condition. Thanks for the reaction Dirk. I hope I left the non-scaling stroke stuff correct and as efficient as before :) Cheers, Rob.
(In reply to comment #7) > (From update of attachment 100809 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100809&action=review > > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:174 > > + int applyMode = ApplyToStrokeMode; > > I think Rob don't want to change the original path. This is necessary for JS calls like pathLength() and similar calls. But have to look at the SVG code first if this is needed. I don't see how that is related? The current code doesn't change the original path at the moment either?
Created attachment 100825 [details] Patch
Hi Niko, (In reply to comment #6) > (From update of attachment 100809 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100809&action=review > > Good catch, some comments: > > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:173 > > + Path* path = &m_path; > > + Path tempPath; > > I don't see the need here for these two. Just leave "Path path" as is. Well I think we want a temp path for either non-scaling or linecap=square code paths, to leave m_path intact. > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:180 > > + // Spec: any zero length subpath shall not be stroked if the âstroke-linecapâ property has a value of butt > > s/any/Any/. A link to the paragraph would be even better (plus your text). Fixed. > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:184 > > + path = &tempPath; > > When you leave "Path path" above, that line is obsolete. See above :) > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:196 > > + tempPath = m_path; > > + path = &tempPath; > > + tempPath.transform(nonScalingStrokeTransform); > > You can revert this is as well. Ditto. > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:327 > > + if (svgStyle->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) { > > m_fillBoundingBox.isEmpty() ? Does not do the same unfortunately :( > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:332 > > + Path path; > > + SVGElement* svgElement = static_cast<SVGElement*>(node()); > > + float strokeWidth = svgStyle->strokeWidth().value(svgElement); > > + path.addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth)); > > + m_strokeAndMarkerBoundingBox = path.boundingRect(); > > Why do you need a temp rect for this? Just use the already calculatd Rect? That is in a different method. So I think we want to keep m_path as it is, changing it at paint time feels wrong. I just now noticed strokeContains is probably wrong for the zero-length path + linecap=square, let me know if I should fix that... Cheers, Rob.
Comment on attachment 100825 [details] Patch Attachment 100825 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9065267 New failing tests: svg/W3C-SVG-1.1-SE/painting-control-04-f.svg
Created attachment 100836 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #11) Hi Rob, > > > > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:173 > > > + Path* path = &m_path; > > > + Path tempPath; > > > > I don't see the need here for these two. Just leave "Path path" as is. > > Well I think we want a temp path for either non-scaling or linecap=square code paths, to leave m_path intact. Okay, but this looks super ugly. I'll think about a better way. > > m_fillBoundingBox.isEmpty() ? > > Does not do the same unfortunately :( Oh yes, you are right. > > > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:332 > > > + Path path; > > > + SVGElement* svgElement = static_cast<SVGElement*>(node()); > > > + float strokeWidth = svgStyle->strokeWidth().value(svgElement); > > > + path.addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth)); > > > + m_strokeAndMarkerBoundingBox = path.boundingRect(); > > > > Why do you need a temp rect for this? Just use the already calculatd Rect? > > That is in a different method. > So I think we want to keep m_path as it is, changing it at paint time feels wrong. I just now noticed strokeContains is probably wrong for the zero-length path + linecap=square, let me know if I should fix that... You misunderstood me. The result of path.boundingRect() should be exactly the same that you've calculated in path.addRect, no? So I'd just use m_strokeAndMarkerBoundingBox = FloatRect(m_fillBo.... Will review the patch now.
Comment on attachment 100825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100825&action=review > Source/WebCore/rendering/svg/RenderSVGPath.cpp:174 > + Path* path = &m_path; > + Path tempPath; > + int applyMode = ApplyToStrokeMode; The information we need to call strokePaingingResource->appyResource/postApplyResource is the Path, and the ApplyToFill/StrokeMode. How about only storing "Path* usePath = 0;" here. And then if (style->svgStyle()->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) setupSquareCapPath(usePath, applyMode); else if (nonScalingStroke) setupNonScalingStrokePath(usePath, applyMode); else { applyMode = ApplyToStrokeMode; usePath = &m_path; } static inline void setupSquareCapPath(Path*& usePath, int& applyMode) { .... } contains your new code static inline void setupNonScalingStrokePath(Path*& usePath, int& applyMode) { .... } contains the old code to handle non-scaling-stroke. That would avoid having to allocate a Path tempPath object even if we don't use it, it also makes the function more readable. What do you think? > Source/WebCore/rendering/svg/RenderSVGPath.cpp:332 > + Path path; > + SVGElement* svgElement = static_cast<SVGElement*>(node()); > + float strokeWidth = svgStyle->strokeWidth().value(svgElement); > + path.addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth)); > + m_strokeAndMarkerBoundingBox = path.boundingRect(); I still think this should be replaced by: m_strokeAndMarkerBoundingBox = FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth); m_repaintBoundingBox = m_strokeAndMarkerBoundingBox; SVGRenderSupport::... return;
Created attachment 100962 [details] Patch
Hi Niko, (In reply to comment #15) > (From update of attachment 100825 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100825&action=review > > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:174 > > + Path* path = &m_path; > > + Path tempPath; > > + int applyMode = ApplyToStrokeMode; > > The information we need to call strokePaingingResource->appyResource/postApplyResource is the Path, and the ApplyToFill/StrokeMode. > How about only storing "Path* usePath = 0;" here. > And then > > if (style->svgStyle()->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) > setupSquareCapPath(usePath, applyMode); > else if (nonScalingStroke) > setupNonScalingStrokePath(usePath, applyMode); > else { > applyMode = ApplyToStrokeMode; > usePath = &m_path; > } > > static inline void setupSquareCapPath(Path*& usePath, int& applyMode) { .... } contains your new code > static inline void setupNonScalingStrokePath(Path*& usePath, int& applyMode) { .... } contains the old code to handle non-scaling-stroke. > > That would avoid having to allocate a Path tempPath object even if we don't use it, it also makes the function more readable. > What do you think? The temp Path bothered me as well. Great idea! I just didnt do setupNonScalingStrokePath since it requires 6 parameters. > > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:332 > > + Path path; > > + SVGElement* svgElement = static_cast<SVGElement*>(node()); > > + float strokeWidth = svgStyle->strokeWidth().value(svgElement); > > + path.addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth)); > > + m_strokeAndMarkerBoundingBox = path.boundingRect(); > > I still think this should be replaced by: > m_strokeAndMarkerBoundingBox = FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth); > m_repaintBoundingBox = m_strokeAndMarkerBoundingBox; > SVGRenderSupport::... > return; Great idea, didnt see that one, fixed. Also note that I fixes strokeContains this time. Now if you do inspect element on such a zero-length path the Inspector shows the correct one. Cheers, Rob.
Comment on attachment 100962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100962&action=review In general this is fine, but has way too much copy/paste code, which needs to be refactored. sorry for another round... > Source/WebCore/rendering/svg/RenderSVGPath.cpp:104 > + if (style()->svgStyle()->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) { That needs to be shared with the other duplicated checks, in a member function like shouldStrokeZeroLengthSubpath.. > Source/WebCore/rendering/svg/RenderSVGPath.cpp:109 > + Path path; > + SVGElement* svgElement = static_cast<SVGElement*>(node()); > + float strokeWidth = style()->svgStyle()->strokeWidth().value(svgElement); > + path.addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth)); > + return path.contains(point, style()->svgStyle()->fillRule()); Erm, this is again a trivial rect, you can implement the "contains" check without the need to use "Path". The fill rule won't matter here. > Source/WebCore/rendering/svg/RenderSVGPath.cpp:162 > +// Spec(11.4): Any zero length subpath shall not be stroked if the âstroke-linecapâ property has a value of butt > +// but shall be stroked if the âstroke-linecapâ property has a value of round or square > +void RenderSVGPath::setupSquareCapPath(Path*& usePath, int& applyMode) I'd move this inline the function. > Source/WebCore/rendering/svg/RenderSVGPath.cpp:167 > + tempPath = Path(); This defeats the idea of a shared Path! This way you'd force eg. CG to create new underlying CGPathRefs. You'd better leave this out and change .... > Source/WebCore/rendering/svg/RenderSVGPath.cpp:171 > + usePath->addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth)); .... to call usePath->clear(); first. That allows the underlying graphics platform to really maintain a shared path. > Source/WebCore/rendering/svg/RenderSVGPath.cpp:204 > + if (style->svgStyle()->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) This logic and the comment should be moved into a member function, as it's more than once. I'd add "bool shouldStrokeZeroLengthPath()". I've just noticed it says _subpath_ not _path_ - what does that mean? sequence of segments? or does it really apply to the whole path only? > Source/WebCore/rendering/svg/RenderSVGPath.cpp:219 > + DEFINE_STATIC_LOCAL(Path, tempPath, ()); > + > SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(node()); > AffineTransform nonScalingStrokeTransform = element->getScreenCTM(SVGLocatable::DisallowStyleUpdate); > if (!nonScalingStrokeTransform.isInvertible()) > return; > > - path = m_path; > - path.transform(nonScalingStrokeTransform); > + tempPath = m_path; > + usePath = &tempPath; > + tempPath.transform(nonScalingStrokeTransform); > > stateSaver.save(); > context->concatCTM(nonScalingStrokeTransform.inverse()); Just make it a member function: bool RenderSVGPath::setupNonScalingStrokePath(Path*& usePath, GraphicsContextStateSaver& stateSaver) { SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(node()); AffineTransform nonScalingStrokeTransform = element->getScreenCTM(SVGLocatable::DisallowStyleUpdate); if (!nonScalingStrokeTransform.isInvertible()) return false; DEFINE_STATIC_LOCAL(Path, tempPath, ()); tempPath = m_path; tempPath.transform(nonScalingStrokeTransform); usePath = &tempPath; stateSaver.save(); stateSaver.context()->concatCTM(nonScalingStrokeTransform.inverse()); return true; } and use (1): else if (nonScalingStroke) { if (!setupNonScalingStrokePath(usePath, stateSaver)) return; } or (2) else if (nonScalingStroke && !setupNonScalingStrokePath(usePath, stateSaver)) return; Though I guess (1) looks nicer. > Source/WebCore/rendering/svg/RenderSVGPath.cpp:350 > + m_strokeAndMarkerBoundingBox = FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth); The logic to create this rectangle needs to be shared as well in a member function. It's needed in multiple places.
Created attachment 100982 [details] Patch
Comment on attachment 100982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100982&action=review Great, r=me. You may want to fix some last things before landing: > Source/WebCore/rendering/svg/RenderSVGPath.cpp:194 > + context->concatCTM(nonScalingStrokeTransform.inverse()); it seems nicer to give the GraphicsContextStateSaver a "GraphicsContext* context" accessor, as it already stores it. You'd save having to hand over GraphicsContext as argument then. I'd do it. > Source/WebCore/rendering/svg/RenderSVGPath.cpp:358 > + const SVGRenderStyle* svgStyle = style()->svgStyle(); This can be reverted to the old position. > Source/WebCore/rendering/svg/RenderSVGPath.cpp:391 > + Not needed?
Comment on attachment 100982 [details] Patch Attachment 100982 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9093272 New failing tests: svg/W3C-SVG-1.1-SE/painting-control-04-f.svg
Created attachment 100987 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Hi Niko, (In reply to comment #18) > (From update of attachment 100962 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100962&action=review > > In general this is fine, but has way too much copy/paste code, which needs to be refactored. sorry for another round... > > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:104 > > + if (style()->svgStyle()->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) { > > That needs to be shared with the other duplicated checks, in a member function like shouldStrokeZeroLengthSubpath.. Fixed. > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:109 > > + Path path; > > + SVGElement* svgElement = static_cast<SVGElement*>(node()); > > + float strokeWidth = style()->svgStyle()->strokeWidth().value(svgElement); > > + path.addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth)); > > + return path.contains(point, style()->svgStyle()->fillRule()); > > Erm, this is again a trivial rect, you can implement the "contains" check without the need to use "Path". The fill rule won't matter here. Right, fixed. > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:162 > > +// Spec(11.4): Any zero length subpath shall not be stroked if the âstroke-linecapâ property has a value of butt > > +// but shall be stroked if the âstroke-linecapâ property has a value of round or square > > +void RenderSVGPath::setupSquareCapPath(Path*& usePath, int& applyMode) > > I'd move this inline the function. Fixed. > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:167 > > + tempPath = Path(); > > This defeats the idea of a shared Path! This way you'd force eg. CG to create new underlying CGPathRefs. > You'd better leave this out and change .... > > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:171 > > + usePath->addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth)); > > .... to call usePath->clear(); first. That allows the underlying graphics platform to really maintain a shared path. I see what you mean, fixed. > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:204 > > + if (style->svgStyle()->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) > > This logic and the comment should be moved into a member function, as it's more than once. I'd add "bool shouldStrokeZeroLengthPath()". Right, fixed. > I've just noticed it says _subpath_ not _path_ - what does that mean? sequence of segments? or does it really apply to the whole path only? Yes, this will unfortunatly not do for subpaths. OTOH I never saw an SVG testing or using that. Added a FIXME as agreed. > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:219 > > + DEFINE_STATIC_LOCAL(Path, tempPath, ()); > > + > > SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(node()); > > AffineTransform nonScalingStrokeTransform = element->getScreenCTM(SVGLocatable::DisallowStyleUpdate); > > if (!nonScalingStrokeTransform.isInvertible()) > > return; > > > > - path = m_path; > > - path.transform(nonScalingStrokeTransform); > > + tempPath = m_path; > > + usePath = &tempPath; > > + tempPath.transform(nonScalingStrokeTransform); > > > > stateSaver.save(); > > context->concatCTM(nonScalingStrokeTransform.inverse()); > > Just make it a member function: > > bool RenderSVGPath::setupNonScalingStrokePath(Path*& usePath, GraphicsContextStateSaver& stateSaver) > { > SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(node()); > AffineTransform nonScalingStrokeTransform = element->getScreenCTM(SVGLocatable::DisallowStyleUpdate); > if (!nonScalingStrokeTransform.isInvertible()) > return false; > DEFINE_STATIC_LOCAL(Path, tempPath, ()); > tempPath = m_path; > tempPath.transform(nonScalingStrokeTransform); > usePath = &tempPath; > stateSaver.save(); > stateSaver.context()->concatCTM(nonScalingStrokeTransform.inverse()); > return true; > } > > and use > (1): > else if (nonScalingStroke) { > if (!setupNonScalingStrokePath(usePath, stateSaver)) > return; > } > > or (2) > else if (nonScalingStroke && !setupNonScalingStrokePath(usePath, stateSaver)) > return; > > Though I guess (1) looks nicer. Agreed, fixed. > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:350 > > + m_strokeAndMarkerBoundingBox = FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth); > > The logic to create this rectangle needs to be shared as well in a member function. It's needed in multiple places. Fixed. Cheers, Rob.
Hi Niko, (In reply to comment #20) > (From update of attachment 100982 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100982&action=review > > Great, r=me. You may want to fix some last things before landing: > > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:194 > > + context->concatCTM(nonScalingStrokeTransform.inverse()); > > it seems nicer to give the GraphicsContextStateSaver a "GraphicsContext* context" accessor, as it already stores it. You'd save having to hand over GraphicsContext as argument then. > I'd do it. Ah, I first read 'i'll do it' :) I"ll try to show the final patch before landing(in a few hours). > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:358 > > + const SVGRenderStyle* svgStyle = style()->svgStyle(); > > This can be reverted to the old position. Fixed. > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:391 > > + > > Not needed? Fixed. Cheers, Rob.
This was fixed in r91125.
(In reply to comment #21) > (From update of attachment 100982 [details]) > Attachment 100982 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/9093272 > > New failing tests: > svg/W3C-SVG-1.1-SE/painting-control-04-f.svg This test failed in the chromium EWS, but it seems nobody did anything about it. Unsurprisingly when this patch landed it made the real chromium testers go red on the exact same test. Why did you land a patch that you knew would make a buildbot go red?
(In reply to comment #26) > (In reply to comment #21) > > (From update of attachment 100982 [details] [details]) > > Attachment 100982 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/9093272 > > > > New failing tests: > > svg/W3C-SVG-1.1-SE/painting-control-04-f.svg > > This test failed in the chromium EWS, but it seems nobody did anything about it. Unsurprisingly when this patch landed it made the real chromium testers go red on the exact same test. Why did you land a patch that you knew would make a buildbot go red? I did assume it's a new test that needs baselines for chromium? This isn't the case?
(In reply to comment #26) > (In reply to comment #21) > > (From update of attachment 100982 [details] [details]) > > Attachment 100982 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/9093272 > > > > New failing tests: > > svg/W3C-SVG-1.1-SE/painting-control-04-f.svg > > This test failed in the chromium EWS, but it seems nobody did anything about it. Unsurprisingly when this patch landed it made the real chromium testers go red on the exact same test. Why did you land a patch that you knew would make a buildbot go red? General question: Do the chromium bots provide pixel results? Can we you the rebaseline tool to upload new results for chromium?
Created attachment 101092 [details] Patch
Hi Niko, (In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #21) > > > (From update of attachment 100982 [details] [details] [details]) > > > Attachment 100982 [details] [details] [details] did not pass chromium-ews (chromium-xvfb): > > > Output: http://queues.webkit.org/results/9093272 > > > > > > New failing tests: > > > svg/W3C-SVG-1.1-SE/painting-control-04-f.svg > > > > This test failed in the chromium EWS, but it seems nobody did anything about it. Unsurprisingly when this patch landed it made the real chromium testers go red on the exact same test. Why did you land a patch that you knew would make a buildbot go red? > > I did assume it's a new test that needs baselines for chromium? This isn't the case? It is a new test that needs a baseline. However, I noticed that the chromium result really was different, the circle was not drawn (stroke-linecap=round)! Maybe James meant that. To fix that we may as well do the rendering ourselves so it will be right regardless of toolkit. The attached patch should fix that, reopening :) Cheers, Rob.
Comment on attachment 101092 [details] Patch Looks great! It doesn't apply on cr-linux atm - can you recreate the patch? I'd like to get confirmation from cr-linux ews that it's correct on Chromium now.
Created attachment 101093 [details] Patch
Hi Niko, (In reply to comment #31) > (From update of attachment 101092 [details]) > Looks great! It doesn't apply on cr-linux atm - can you recreate the patch? I'd like to get confirmation from cr-linux ews that it's correct on Chromium now. Done! So I did an update-webkit && webkit-patch upload, lets hope it works this time... Cheers, Rob.
I just checked on qt, the toolkit doesnt render the round cap either for zero-length, so this should really go in. Cheers, Rob. (In reply to comment #33) > Hi Niko, > > (In reply to comment #31) > > (From update of attachment 101092 [details] [details]) > > Looks great! It doesn't apply on cr-linux atm - can you recreate the patch? I'd like to get confirmation from cr-linux ews that it's correct on Chromium now. > > Done! So I did an update-webkit && webkit-patch upload, lets hope it works this time... > Cheers, > > Rob.
Comment on attachment 101093 [details] Patch r=me, very elegant :-)
Comment on attachment 101093 [details] Patch Clearing flags on attachment: 101093 Committed r91191: <http://trac.webkit.org/changeset/91191>
All reviewed patches have been landed. Closing bug.
(In reply to comment #28) > (In reply to comment #26) > > (In reply to comment #21) > > > (From update of attachment 100982 [details] [details] [details]) > > > Attachment 100982 [details] [details] [details] did not pass chromium-ews (chromium-xvfb): > > > Output: http://queues.webkit.org/results/9093272 > > > > > > New failing tests: > > > svg/W3C-SVG-1.1-SE/painting-control-04-f.svg > > > > This test failed in the chromium EWS, but it seems nobody did anything about it. Unsurprisingly when this patch landed it made the real chromium testers go red on the exact same test. Why did you land a patch that you knew would make a buildbot go red? > > General question: Do the chromium bots provide pixel results? Can we you the rebaseline tool to upload new results for chromium? They do, and the chromium EWS bots also run pixel tests and upload their results. If you'd looked at the attachment layout-test-results from ec2-cr-linux-02, you would see that the circle didn't render at all. It looks like Rob Buis found a better fix for this and landed it (thanks!) Even if a test needs new baselines, it is NOT valid to land a patch that makes a bot go red! Add an appropriate entry to test_expectations.txt if a test will need new baselines. If you aren't sure how to do this, ask.
(In reply to comment #38) > Even if a test needs new baselines, it is NOT valid to land a patch that makes a bot go red! Add an appropriate entry to test_expectations.txt if a test will need new baselines. If you aren't sure how to do this, ask. This is news to me - in the past chromium gardening was left to the chromium team. Over the past years, I never had to touch chromium specific NRWT stuff. Now times are changing, as we unify our test expectation management, so in the future we'll cover chromium as well.
We didn't used to have EWS bots, either, so it was not always possible to predict what would happen when a patch landed. In this case the EWS bot ran, told you that landing the patch would make bots turn red. In that case it's pretty clear that you should take care of the issue before landing (one way or another), not just leave a mess for someone else to clean up.
(In reply to comment #38) > > General question: Do the chromium bots provide pixel results? Can we you the rebaseline tool to upload new results for chromium? > > They do, and the chromium EWS bots also run pixel tests and upload their results. If you'd looked at the attachment layout-test-results from ec2-cr-linux-02, you would see that the circle didn't render at all. It looks like Rob Buis found a better fix for this and landed it (thanks!) Yes he did. Great that it is fixed now. > > Even if a test needs new baselines, it is NOT valid to land a patch that makes a bot go red! Add an appropriate entry to test_expectations.txt if a test will need new baselines. If you aren't sure how to do this, ask. I never said that it is correct to land patches that break builds. I just was curious if it is possible to upload new baselines to the bots with webkit-patch --rebaseline, including pixel results. Nevertheless, if you compare the 'failing' results with a revision before Robs patch, you'll see that Rob definitely did not broke chromium! The patch was a progression for chromium as well. Chromium just needed a bit more love and a followup in this case would be ok as well for me. Great that Rob found a solution to fix the test completely on all ports. Thank you Rob!
Hi Dirk, (In reply to comment #41) > (In reply to comment #38) > > Even if a test needs new baselines, it is NOT valid to land a patch that makes a bot go red! Add an appropriate entry to test_expectations.txt if a test will need new baselines. If you aren't sure how to do this, ask. > I never said that it is correct to land patches that break builds. I just was curious if it is possible to upload new baselines to the bots with webkit-patch --rebaseline, including pixel results. > > Nevertheless, if you compare the 'failing' results with a revision before Robs patch, you'll see that Rob definitely did not broke chromium! The patch was a progression for chromium as well. Chromium just needed a bit more love and a followup in this case would be ok as well for me. Great that Rob found a solution to fix the test completely on all ports. Thank you Rob! Well, that is what they pay me for, right? ;) Cheers, Rob.
This caused bug 65257
This also caused bug 65796