Merge r180643 - Horizontal and vertical lines are clipped completely …

…if clip-path is included in the tag but the referenced element is defined later. https://bugs.webkit.org/show_bug.cgi?id=141776. Reviewed by Dean Jackson. Source/WebCore: Tests: svg/clip-path/clip-path-line-use-before-defined-expected.svg svg/clip-path/clip-path-line-use-before-defined.svg * rendering/svg/RenderSVGResourceClipper.cpp: (WebCore::RenderSVGResourceClipper::applyClippingToContext): Ensure the renderer is added to m_clipper if it does not exist. The same renderer might have been added to m_clipper in resourceBoundingBox(). (WebCore::RenderSVGResourceClipper::addRendererToClipper): Add the renderer to m_clipper if it does not exist. Return the associated ClipperData. (WebCore::RenderSVGResourceClipper::resourceBoundingBox): If the clipper is referenced before it is defined, add the renderer to m_clipper. While doing the layout() for the clipper, we can check if m_clipper has values or not. If it does have, we are going to mark the clipper for client invalidation which is done by the SVG root. * rendering/svg/RenderSVGResourceClipper.h: * rendering/svg/RenderSVGResourceContainer.h: (WebCore::RenderSVGResourceContainer::selfNeedsClientInvalidation): Define a new function selfNeedsClientInvalidation() which controls marking the clipper for client invalidation. In RenderSVGResourceClipper, override it so it checks m_clipper to force clients validation even if it the first time we do layout for this clipper. * rendering/svg/RenderSVGResourceContainer.cpp: (WebCore::RenderSVGResourceContainer::layout): Call the virtual function selfNeedsClientInvalidation() to check whether we need to mark the clipper for client invalidation. * svg/SVGElement.cpp: Delete unneeded header file. LayoutTests: New test cases for SVG lines which are clipped to a <clipPath>. The <clipPath> is referenced before it is defined. * svg/clip-path/clip-path-line-use-before-defined-expected.svg: Added. * svg/clip-path/clip-path-line-use-before-defined.svg: Added.
WebKit · Mar 1, 2015 · cb51fa6 · cb51fa6
1 parent 8f6278a
commit cb51fa6
Show file tree

Hide file tree

Showing 9 changed files with 211 additions and 33 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,27 @@
+2015-02-26  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Cleanup RenderSVGResourceClipper class.
+        https://bugs.webkit.org/show_bug.cgi?id=142032.
+
+        Reviewed by Darin Adler.
+
+        * svg/clip-path/clip-path-line-use-before-defined-expected.svg:
+        * svg/clip-path/clip-path-line-use-before-defined.svg: Simplify the test
+        and make separate drawings for different cases.
+
+2015-02-25  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Horizontal and vertical lines are clipped completely if clip-path is included in the tag but the referenced element is defined later.
+        https://bugs.webkit.org/show_bug.cgi?id=141776.
+
+        Reviewed by Dean Jackson.
+
+        New test cases for SVG lines which are clipped to a <clipPath>. The <clipPath>
+        is referenced before it is defined.
+
+        * svg/clip-path/clip-path-line-use-before-defined-expected.svg: Added.
+        * svg/clip-path/clip-path-line-use-before-defined.svg: Added.
+
 2015-02-25  Joanmarie Diggs  <jdiggs@igalia.com>
 
         AX: Implement support for ARIA 1.1 'searchbox' role

diff --git a/LayoutTests/svg/clip-path/clip-path-line-use-before-defined-expected.svg b/LayoutTests/svg/clip-path/clip-path-line-use-before-defined-expected.svg
diff --git a/LayoutTests/svg/clip-path/clip-path-line-use-before-defined.svg b/LayoutTests/svg/clip-path/clip-path-line-use-before-defined.svg
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,65 @@
+2015-02-26  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Cleanup RenderSVGResourceClipper class.
+        https://bugs.webkit.org/show_bug.cgi?id=142032.
+
+        Reviewed by Darin Adler.
+
+        This is a follow up for r180643: <http://trac.webkit.org/changeset/180643>.
+        It includes cleanup for RenderSVGResourceClipper class.
+
+        * rendering/svg/RenderSVGResourceClipper.cpp:
+        (WebCore::RenderSVGResourceClipper::applyClippingToContext):
+        (WebCore::RenderSVGResourceClipper::drawContentIntoMaskImage):
+        * rendering/svg/RenderSVGResourceClipper.h: Change ClipperData to be a
+        typedef instead of a class and rename it to ClipperMaskImage. The purpose
+        of having it a class even though it includes only one member was because
+        we wanted it to be WTF_MAKE_FAST_ALLOCATED. We do not need to allocate it
+        as a separate object on the heap anymore.
+
+        (WebCore::RenderSVGResourceClipper::addRendererToClipper): Instead of doing
+        double hash table lookups by calling HashMap::contains() and then HashMap::get(),
+        we can use HashMap::add() instead.
+
+2015-02-25  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Horizontal and vertical lines are clipped completely if clip-path is included in the tag but the referenced element is defined later.
+        https://bugs.webkit.org/show_bug.cgi?id=141776.
+
+        Reviewed by Dean Jackson.
+
+        Tests: svg/clip-path/clip-path-line-use-before-defined-expected.svg
+               svg/clip-path/clip-path-line-use-before-defined.svg
+
+        * rendering/svg/RenderSVGResourceClipper.cpp:
+        (WebCore::RenderSVGResourceClipper::applyClippingToContext): Ensure the renderer
+        is added to m_clipper if it does not exist. The same renderer might have been 
+        added to m_clipper in resourceBoundingBox().
+
+        (WebCore::RenderSVGResourceClipper::addRendererToClipper): Add the renderer to
+        m_clipper if it does not exist. Return the associated ClipperData.
+
+        (WebCore::RenderSVGResourceClipper::resourceBoundingBox): If the clipper is
+        referenced before it is defined, add the renderer to m_clipper. While doing the
+        layout() for the clipper, we can check if m_clipper has values or not. If it does
+        have, we are going to mark the clipper for client invalidation which is done by
+        the SVG root.
+
+        * rendering/svg/RenderSVGResourceClipper.h:
+        * rendering/svg/RenderSVGResourceContainer.h:
+        (WebCore::RenderSVGResourceContainer::selfNeedsClientInvalidation): Define a 
+        new function selfNeedsClientInvalidation() which controls marking the clipper 
+        for client invalidation. In RenderSVGResourceClipper, override it so it checks
+        m_clipper to force clients validation even if it the first time we do layout
+        for this clipper.
+
+        * rendering/svg/RenderSVGResourceContainer.cpp:
+        (WebCore::RenderSVGResourceContainer::layout): Call the virtual function
+        selfNeedsClientInvalidation() to check whether we need to mark the clipper for
+        client invalidation.
+
+        * svg/SVGElement.cpp: Delete unneeded header file.
+
 2015-02-25  Beth Dakin  <bdakin@apple.com>
 
         REGRESSION (r180018 ): Holding a rubber-band in place can get stuck

diff --git a/Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp b/Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp
@@ -130,27 +130,22 @@ bool RenderSVGResourceClipper::pathOnlyClipping(GraphicsContext* context, const
 bool RenderSVGResourceClipper::applyClippingToContext(RenderElement& renderer, const FloatRect& objectBoundingBox,
                                                       const FloatRect& repaintRect, GraphicsContext* context)
 {
-    bool missingClipperData = !m_clipper.contains(&renderer);
-    if (missingClipperData)
-        m_clipper.set(&renderer, std::make_unique<ClipperData>());
+    ClipperMaskImage& clipperMaskImage = addRendererToClipper(renderer);
+    bool shouldCreateClipperMaskImage = !clipperMaskImage;
 
-    bool shouldCreateClipData = false;
     AffineTransform animatedLocalTransform = clipPathElement().animatedLocalTransform();
-    ClipperData* clipperData = m_clipper.get(&renderer);
-    if (!clipperData->clipMaskImage) {
-        if (pathOnlyClipping(context, animatedLocalTransform, objectBoundingBox))
-            return true;
-        shouldCreateClipData = true;
-    }
+
+    if (shouldCreateClipperMaskImage && pathOnlyClipping(context, animatedLocalTransform, objectBoundingBox))
+        return true;
 
     AffineTransform absoluteTransform;
     SVGRenderingContext::calculateTransformationToOutermostCoordinateSystem(renderer, absoluteTransform);
 
-    if (shouldCreateClipData && !repaintRect.isEmpty()) {
-        if (!SVGRenderingContext::createImageBuffer(repaintRect, absoluteTransform, clipperData->clipMaskImage, ColorSpaceDeviceRGB, Unaccelerated))
+    if (shouldCreateClipperMaskImage && !repaintRect.isEmpty()) {
+        if (!SVGRenderingContext::createImageBuffer(repaintRect, absoluteTransform, clipperMaskImage, ColorSpaceDeviceRGB, Unaccelerated))
             return false;
 
-        GraphicsContext* maskContext = clipperData->clipMaskImage->context();
+        GraphicsContext* maskContext = clipperMaskImage->context();
         ASSERT(maskContext);
 
         maskContext->concatCTM(animatedLocalTransform);
@@ -165,28 +160,27 @@ bool RenderSVGResourceClipper::applyClippingToContext(RenderElement& renderer, c
             if (!clipper->applyClippingToContext(*this, objectBoundingBox, repaintRect, maskContext))
                 return false;
 
-            succeeded = drawContentIntoMaskImage(clipperData, objectBoundingBox);
+            succeeded = drawContentIntoMaskImage(clipperMaskImage, objectBoundingBox);
             // The context restore applies the clipping on non-CG platforms.
         } else
-            succeeded = drawContentIntoMaskImage(clipperData, objectBoundingBox);
+            succeeded = drawContentIntoMaskImage(clipperMaskImage, objectBoundingBox);
 
         if (!succeeded)
-            clipperData->clipMaskImage.reset();
+            clipperMaskImage.reset();
     }
 
-    if (!clipperData->clipMaskImage)
+    if (!clipperMaskImage)
         return false;
 
-    SVGRenderingContext::clipToImageBuffer(context, absoluteTransform, repaintRect, clipperData->clipMaskImage, missingClipperData);
+    SVGRenderingContext::clipToImageBuffer(context, absoluteTransform, repaintRect, clipperMaskImage, shouldCreateClipperMaskImage);
     return true;
 }
 
-bool RenderSVGResourceClipper::drawContentIntoMaskImage(ClipperData* clipperData, const FloatRect& objectBoundingBox)
+bool RenderSVGResourceClipper::drawContentIntoMaskImage(const ClipperMaskImage& clipperMaskImage, const FloatRect& objectBoundingBox)
 {
-    ASSERT(clipperData);
-    ASSERT(clipperData->clipMaskImage);
+    ASSERT(clipperMaskImage);
 
-    GraphicsContext* maskContext = clipperData->clipMaskImage->context();
+    GraphicsContext* maskContext = clipperMaskImage->context();
     ASSERT(maskContext);
 
     AffineTransform maskContentTransformation;
@@ -237,7 +231,7 @@ bool RenderSVGResourceClipper::drawContentIntoMaskImage(ClipperData* clipperData
         // In the case of a <use> element, we obtained its renderere above, to retrieve its clipRule.
         // We have to pass the <use> renderer itself to renderSubtreeToImageBuffer() to apply it's x/y/transform/etc. values when rendering.
         // So if isUseElement is true, refetch the childNode->renderer(), as renderer got overriden above.
-        SVGRenderingContext::renderSubtreeToImageBuffer(clipperData->clipMaskImage.get(), isUseElement ? *child.renderer() : *renderer, maskContentTransformation);
+        SVGRenderingContext::renderSubtreeToImageBuffer(clipperMaskImage.get(), isUseElement ? *child.renderer() : *renderer, maskContentTransformation);
     }
 
     view().frameView().setPaintBehavior(oldBehavior);
@@ -261,6 +255,11 @@ void RenderSVGResourceClipper::calculateClipContentRepaintRect()
     m_clipBoundaries = clipPathElement().animatedLocalTransform().mapRect(m_clipBoundaries);
 }
 
+ClipperMaskImage& RenderSVGResourceClipper::addRendererToClipper(const RenderObject& object)
+{
+    return m_clipper.add(&object, ClipperMaskImage()).iterator->value;
+}
+
 bool RenderSVGResourceClipper::hitTestClipContent(const FloatRect& objectBoundingBox, const FloatPoint& nodeAtPoint)
 {
     FloatPoint point = nodeAtPoint;
@@ -294,8 +293,10 @@ bool RenderSVGResourceClipper::hitTestClipContent(const FloatRect& objectBoundin
 FloatRect RenderSVGResourceClipper::resourceBoundingBox(const RenderObject& object)
 {
     // Resource was not layouted yet. Give back the boundingBox of the object.
-    if (selfNeedsLayout())
+    if (selfNeedsLayout()) {
+        addRendererToClipper(object);
         return object.objectBoundingBox();
+    }
 
     if (m_clipBoundaries.isEmpty())
         calculateClipContentRepaintRect();

diff --git a/Source/WebCore/rendering/svg/RenderSVGResourceClipper.h b/Source/WebCore/rendering/svg/RenderSVGResourceClipper.h
@@ -31,11 +31,7 @@
 
 namespace WebCore {
 
-struct ClipperData {
-    WTF_MAKE_FAST_ALLOCATED;
-public:
-    std::unique_ptr<ImageBuffer> clipMaskImage;
-};
+typedef std::unique_ptr<ImageBuffer> ClipperMaskImage;
 
 class RenderSVGResourceClipper final : public RenderSVGResourceContainer {
 public:
@@ -60,17 +56,21 @@ class RenderSVGResourceClipper final : public RenderSVGResourceContainer {
 
     SVGUnitTypes::SVGUnitType clipPathUnits() const { return clipPathElement().clipPathUnits(); }
 
+protected:
+    virtual bool selfNeedsClientInvalidation() const override { return (everHadLayout() || m_clipper.size()) && selfNeedsLayout(); }
+
 private:
     void element() const = delete;
 
     virtual const char* renderName() const override { return "RenderSVGResourceClipper"; }
 
     bool pathOnlyClipping(GraphicsContext*, const AffineTransform&, const FloatRect&);
-    bool drawContentIntoMaskImage(ClipperData*, const FloatRect& objectBoundingBox);
+    bool drawContentIntoMaskImage(const ClipperMaskImage&, const FloatRect& objectBoundingBox);
     void calculateClipContentRepaintRect();
+    ClipperMaskImage& addRendererToClipper(const RenderObject&);
 
     FloatRect m_clipBoundaries;
-    HashMap<RenderObject*, std::unique_ptr<ClipperData>> m_clipper;
+    HashMap<const RenderObject*, ClipperMaskImage> m_clipper;
 };
 
 }

diff --git a/Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp b/Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp
@@ -52,7 +52,7 @@ void RenderSVGResourceContainer::layout()
 {
     StackStats::LayoutCheckPoint layoutCheckPoint;
     // Invalidate all resources if our layout changed.
-    if (everHadLayout() && selfNeedsLayout())
+    if (selfNeedsClientInvalidation())
         RenderSVGRoot::addResourceForClientInvalidation(this);
 
     RenderSVGHiddenContainer::layout();

diff --git a/Source/WebCore/rendering/svg/RenderSVGResourceContainer.h b/Source/WebCore/rendering/svg/RenderSVGResourceContainer.h
@@ -55,6 +55,8 @@ class RenderSVGResourceContainer : public RenderSVGHiddenContainer,
     };
 
     // Used from the invalidateClient/invalidateClients methods from classes, inheriting from us.
+    virtual bool selfNeedsClientInvalidation() const { return everHadLayout() && selfNeedsLayout(); }
+
     void markAllClientsForInvalidation(InvalidationMode);
     void markAllClientLayersForInvalidation();
     void markClientForInvalidation(RenderObject&, InvalidationMode);

diff --git a/Source/WebCore/svg/SVGElement.cpp b/Source/WebCore/svg/SVGElement.cpp
@@ -39,7 +39,6 @@
 #include "HTMLParserIdioms.h"
 #include "RenderObject.h"
 #include "RenderSVGResource.h"
-#include "RenderSVGResourceClipper.h"
 #include "RenderSVGResourceFilter.h"
 #include "RenderSVGResourceMasker.h"
 #include "SVGCursorElement.h"