REGRESSION (r91125): Polyline tool in google docs is broken
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Jan 2012 19:18:18 +0000 (19:18 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Jan 2012 19:18:18 +0000 (19:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=65796

Patch by Stephen Chenney <schenney@chromium.org> on 2012-01-26
Reviewed by Nikolas Zimmermann.

It turns out that the CG problem is a design decision. The bounding code
returns CGRectNull for cases where a bound is ill-defined, rather than the
empty bound as expected.

I'm also removing the workaround for isEmpty to get correct zero length paths.
It is no longer necessary.

Tested by existing layout tests.

* platform/graphics/cg/PathCG.cpp: Removed path empty and path bound testing classes.
(WebCore::Path::boundingRect): Added check for CGRectNull
(WebCore::Path::fastBoundingRect): Added check for CGRectNull
(WebCore::Path::strokeBoundingRect): Added check for CGRectNull
(WebCore::Path::isEmpty): Reverted to former behavior, just using CGPathIsEmpty.
(WebCore::Path::hasCurrentPoint): Reverted to former behavior, using isEmpty.
(WebCore::Path::transform): Reverted to former behavior, using isEmpty.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@106016 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/cg/PathCG.cpp

index e4927df..2e63e3b 100644 (file)
@@ -1,3 +1,27 @@
+2012-01-26  Stephen Chenney  <schenney@chromium.org>
+
+        REGRESSION (r91125): Polyline tool in google docs is broken
+        https://bugs.webkit.org/show_bug.cgi?id=65796
+
+        Reviewed by Nikolas Zimmermann.
+
+        It turns out that the CG problem is a design decision. The bounding code
+        returns CGRectNull for cases where a bound is ill-defined, rather than the
+        empty bound as expected.
+
+        I'm also removing the workaround for isEmpty to get correct zero length paths.
+        It is no longer necessary.
+
+        Tested by existing layout tests.
+
+        * platform/graphics/cg/PathCG.cpp: Removed path empty and path bound testing classes.
+        (WebCore::Path::boundingRect): Added check for CGRectNull
+        (WebCore::Path::fastBoundingRect): Added check for CGRectNull
+        (WebCore::Path::strokeBoundingRect): Added check for CGRectNull
+        (WebCore::Path::isEmpty): Reverted to former behavior, just using CGPathIsEmpty.
+        (WebCore::Path::hasCurrentPoint): Reverted to former behavior, using isEmpty.
+        (WebCore::Path::transform): Reverted to former behavior, using isEmpty.
+
 2012-01-26  Andreas Kling  <awesomekling@apple.com>
 
         Refactor application of additional style attributes for table elements.
index 93faf80..01f7ffe 100644 (file)
 
 namespace WebCore {
 
-// A class to provide an isEmpty test that considers a one-element path with only a MoveTo element
-// to be empty. This behavior is consistent with other platforms in WebKit, and is needed to prevent
-// incorrect (according to the spec) linecap stroking for zero length paths in SVG.
-class PathIsEmptyOrSingleMoveTester {
-public:
-    PathIsEmptyOrSingleMoveTester() : m_moveCount(0) { }
-
-    bool isEmpty() const
-    {
-        return m_moveCount <= 1;
-    }
-
-    static void testPathElement(void* info, const CGPathElement* element)
-    {
-        PathIsEmptyOrSingleMoveTester* tester = static_cast<PathIsEmptyOrSingleMoveTester*>(info);
-        if (element->type == kCGPathElementMoveToPoint)
-            ++tester->m_moveCount;
-        else {
-            // Any non move element implies a non-empty path; set the count to 2 to force
-            // isEmpty to return false.
-            tester->m_moveCount = 2;
-        }
-    }
-
-private:
-    // Any non-move-to element, or more than one move-to element, will make the count >= 2.
-    unsigned m_moveCount;
-};
-
-// Paths with only move-to elements do not draw under any circumstances, so their bound should
-// be empty. Currently, CoreGraphics returns non-empty bounds for such paths. Radar 10450621
-// tracks this. This class reports paths that have only move-to elements, allowing the
-// bounding box code to work around the CoreGraphics problem.
-class PathHasOnlyMoveToTester {
-public:
-    PathHasOnlyMoveToTester() : m_hasSeenOnlyMoveTo(true) { }
-
-    bool hasOnlyMoveTo() const
-    {
-        return m_hasSeenOnlyMoveTo;
-    }
-
-    static void testPathElement(void* info, const CGPathElement* element)
-    {
-        PathHasOnlyMoveToTester* tester = static_cast<PathHasOnlyMoveToTester*>(info);
-        if (tester->m_hasSeenOnlyMoveTo && element->type != kCGPathElementMoveToPoint)
-            tester->m_hasSeenOnlyMoveTo = false;
-    }
-
-private:
-    bool m_hasSeenOnlyMoveTo;
-};
-
 static size_t putBytesNowhere(void*, const void*, size_t count)
 {
     return count;
@@ -218,32 +165,20 @@ FloatRect Path::boundingRect() const
 {
     // CGPathGetBoundingBox includes the path's control points, CGPathGetPathBoundingBox
     // does not, but only exists on 10.6 and above.
-    // A bug in CoreGraphics leads to an incorrect bound on paths containing only move-to elements
-    // with a linecap style that is non-butt. All paths with only move-to elements (regardless of
-    // linecap) are effectively empty for bounding purposes and here we make it so.
-    PathHasOnlyMoveToTester tester;
-    CGPathApply(m_path, &tester, PathHasOnlyMoveToTester::testPathElement);
-    if (tester.hasOnlyMoveTo())
-        return FloatRect(0, 0, 0, 0);
 
+    CGRect bound = CGRectZero;
 #if !defined(BUILDING_ON_LEOPARD)
-    return CGPathGetPathBoundingBox(m_path);
+    bound = CGPathGetPathBoundingBox(m_path);
 #else
-    return CGPathGetBoundingBox(m_path);
+    bound = CGPathGetBoundingBox(m_path);
 #endif
+    return CGRectIsNull(bound) ? CGRectZero : bound;
 }
 
 FloatRect Path::fastBoundingRect() const
 {
-    // A bug in CoreGraphics leads to an incorrect bound on paths containing only move-to elements
-    // with a linecap style that is non-butt. All paths with only move-to elements (regardless of
-    // linecap) are effectively empty for bounding purposes and here we make it so.
-    PathHasOnlyMoveToTester tester;
-    CGPathApply(m_path, &tester, PathHasOnlyMoveToTester::testPathElement);
-    if (tester.hasOnlyMoveTo())
-        return FloatRect(0, 0, 0, 0);
-
-    return CGPathGetBoundingBox(m_path);
+    CGRect bound = CGPathGetBoundingBox(m_path);
+    return CGRectIsNull(bound) ? CGRectZero : bound;
 }
 
 FloatRect Path::strokeBoundingRect(StrokeStyleApplier* applier) const
@@ -263,7 +198,7 @@ FloatRect Path::strokeBoundingRect(StrokeStyleApplier* applier) const
     CGRect box = CGContextIsPathEmpty(context) ? CGRectZero : CGContextGetPathBoundingBox(context);
     CGContextRestoreGState(context);
 
-    return box;
+    return CGRectIsNull(box) ? CGRectZero : box;
 }
 
 void Path::moveTo(const FloatPoint& point)
@@ -321,18 +256,12 @@ void Path::clear()
 
 bool Path::isEmpty() const
 {
-    // The SVG rendering code that uses this method relies on paths with a single move-to
-    // element, and nothing else, as being empty. Until that code is refactored to avoid
-    // the dependence on isEmpty, we match the behavior of other platforms.
-    // When the SVG code is refactored, we could use CGPathIsEmpty(m_path);
-    PathIsEmptyOrSingleMoveTester tester;
-    CGPathApply(m_path, &tester, PathIsEmptyOrSingleMoveTester::testPathElement);
-    return tester.isEmpty();
+    return CGPathIsEmpty(m_path);
 }
 
 bool Path::hasCurrentPoint() const
 {
-    return !CGPathIsEmpty(m_path);
+    return !isEmpty();
 }
     
 FloatPoint Path::currentPoint() const 
@@ -386,7 +315,7 @@ void Path::apply(void* info, PathApplierFunction function) const
 
 void Path::transform(const AffineTransform& transform)
 {
-    if (transform.isIdentity() || CGPathIsEmpty(m_path))
+    if (transform.isIdentity() || isEmpty())
         return;
 
     CGMutablePathRef path = CGPathCreateMutable();