Bug 96381

Created attachment 165258 [details] patch

Attachment 165258 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/css3/masking/clip-path-referen..." exit_code: 1 Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2431: Extra space between return and CSSPrimitiveValue [whitespace/declaration] [3] Source/WebCore/rendering/RenderLayer.cpp:85: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 165258 [details] patch Attachment 165258 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13953971

Comment on attachment 165258 [details] patch Attachment 165258 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13987001 New failing tests: fast/masking/parsing-clip-path-iri.html

Comment on attachment 165258 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=165258&action=review Please use phrases on FIXME's. There is a build warning that you never run into a code path in styleresolver. > LayoutTests/css3/masking/clip-path-reference.html:21 > +<svg height="0"> > + <clipPath id="c1" clipPathUnits="objectBoundingBox"> Please add a test with userSpaceOnUse as well. Note that you need to specify a with and height for the SVG element then: <svg height="200" width="200"> <clipPath id="c1" clipPathUnits="userSpaceOnUse"> <rect x="20" y="20" width="160" height="160"/> </clipPath> > Source/WebCore/css/StyleBuilder.cpp:-1690 > -template <ClipPathOperation* (RenderStyle::*getterFunction)() const, void (RenderStyle::*setterFunction)(PassRefPtr<ClipPathOperation>), ClipPathOperation* (*initialFunction)()> > -class ApplyPropertyClipPath { > -public: I was told to use this template, since it is the new way to use StyleBuilder. Like discussed offline, the code from StyleResolver::createPathOperation should be moved here. > Source/WebCore/css/StyleBuilder.cpp:-2055 > - setPropertyHandler(CSSPropertyWebkitClipPath, ApplyPropertyClipPath<&RenderStyle::clipPath, &RenderStyle::setClipPath, &RenderStyle::initialClipPath>::createHandler()); Needs to be added again. > Source/WebCore/css/StyleResolver.cpp:5171 > + // FIXME: needs handling for load pending SVG documents (similar to what CSS Filters) Add the reference to the bug report https://bugs.webkit.org/show_bug.cgi?id=90405. > Source/WebCore/rendering/RenderLayer.cpp:3149 > + // FIXME: doesn't work if we have an external reference to a SVG document or if the SVG element is after the HTML one The external referencing doesn't work in SVG either. We have a bug report about that. The one with SVG after HTML should reference to the bug report: https://bugs.webkit.org/show_bug.cgi?id=90405 > Source/WebCore/rendering/RenderLayer.cpp:3151 > + > + One empty line s enough. > Source/WebCore/rendering/RenderLayer.cpp:3155 > + RenderSVGResourceClipper* clipper = static_cast<RenderSVGResourceClipper*>(clipPath->renderer()); > + if (clipper) If you don't have more checks, move the initialisation into the if function: if (RenderSVGResourceClipper* clipper = static_cast<RenderSVGResourceClipper*>(clipPath->renderer())) > Source/WebCore/rendering/RenderLayer.cpp:3156 > + clipper->applyClippingToContext(renderer(), calculateLayerBounds(this, rootLayer, 0), paintDirtyRect, transparencyLayerContext); should be context, not transparencyLayerContext. > Source/WebCore/rendering/svg/RenderSVGResourceClipper.h:58 > + // clipPath can be clipped too, but don't have a boundingBox or repaintRect. So we can't call > + // applyResource directly and use the rects from the object, since they are empty for RenderSVGResources > + // This function is public because we need to apply clipping on HTML element > + bool applyClippingToContext(RenderObject*, const FloatRect&, const FloatRect&, GraphicsContext*); They should have a bounding box. Seems to be a bug.

Forgot to say: This is really awesome code! Great work! :)

Created attachment 165340 [details] Updated patch

Created attachment 165342 [details] removed extra include

Comment on attachment 165342 [details] removed extra include Attachment 165342 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13985655 New failing tests: fast/masking/parsing-clip-path-iri.html

Comment on attachment 165342 [details] removed extra include Attachment 165342 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13989641 New failing tests: fast/masking/parsing-clip-path-iri.html css3/masking/clip-path-reference-userSpaceOnUse.html

Comment on attachment 165342 [details] removed extra include Attachment 165342 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13996664 New failing tests: fast/masking/parsing-clip-path-iri.html css3/masking/clip-path-reference-userSpaceOnUse.html

Do you have the time to investigate in the test failure?

Comment on attachment 165342 [details] removed extra include Looks sane to me. I'll leave the final review for DIrk though as he published comments earlier.

Created attachment 167456 [details] Patch Just updated the style issues and added on ref test for chromium to TextExpectations. Seems to be a Chromium specific bug. Opened a separate bug report.

Dan asked to make one more change on IRC: cssUrlValue => cssURLValue Will do that before landing.

Committed r130592: <http://trac.webkit.org/changeset/130592>

Comment on attachment 167456 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167456&action=review > Source/WebCore/rendering/RenderLayer.cpp:3150 > + static_cast<RenderSVGResourceClipper*>(clipPath->renderer())->applyClippingToContext(renderer(), calculateLayerBounds(this, rootLayer, 0), paintDirtyRect, context); Why doesn't this code set hasClipPath to true? Don't we need to restore the context at the end of the function? In fact, applyClippingToContext() clips the context without saving state, so I can't see how this is correct.

(In reply to comment #17) > (From update of attachment 167456 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167456&action=review > > > Source/WebCore/rendering/RenderLayer.cpp:3150 > > + static_cast<RenderSVGResourceClipper*>(clipPath->renderer())->applyClippingToContext(renderer(), calculateLayerBounds(this, rootLayer, 0), paintDirtyRect, context); > > Why doesn't this code set hasClipPath to true? Don't we need to restore the context at the end of the function? In fact, applyClippingToContext() clips the context without saving state, so I can't see how this is correct. I'll check.

(In reply to comment #17) > (From update of attachment 167456 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167456&action=review > > > Source/WebCore/rendering/RenderLayer.cpp:3150 > > + static_cast<RenderSVGResourceClipper*>(clipPath->renderer())->applyClippingToContext(renderer(), calculateLayerBounds(this, rootLayer, 0), paintDirtyRect, context); > > Why doesn't this code set hasClipPath to true? Don't we need to restore the context at the end of the function? In fact, applyClippingToContext() clips the context without saving state, so I can't see how this is correct. Looking at the source, we indeed do not restore the context on SVG clipping. I was more worried if we call save without restore, but this isn't the case. The most important part is that we do not have a security issue here and we haven't. I can not say if the painted result is correct under all circumstances. I guess it just needs testing. However, saving and restoring for SVG clipping shouldn't cause rendering issues either. If you have a test where the rendering fails, we can add save and restore guards.