[Webkit-unassigned] [Bug 48516] GraphicsContext: Remove "current path" and have strokePath, fillPath and clipPath take a Path argument

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48516


Dirk Schulze <krit at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #74492|review?                     |review-
               Flag|                            |




--- Comment #37 from Dirk Schulze <krit at webkit.org>  2010-11-20 23:36:18 PST ---
(From update of attachment 74492)
View in context: https://bugs.webkit.org/attachment.cgi?id=74492&action=review

> WebCore/platform/graphics/cg/GraphicsContextCG.cpp:399
> +static Path createConvexPolygonPath(size_t numPoints, const FloatPoint* points)

Path& not Path. Don't copy the created path.

> WebCore/platform/graphics/qt/GraphicsContextQt.cpp:491
> +void GraphicsContext::fillPath(const Path& pathToFill)

Is fillPath better? Not sure.

> WebCore/platform/graphics/qt/GraphicsContextQt.cpp:529
> +void GraphicsContext::strokePath(const Path& pathToStroke)

Ditto. strokePath?

> WebCore/platform/graphics/qt/GraphicsContextQt.cpp:755
> +void GraphicsContext::clipPath(const Path& path, WindRule clipRule)

That is inconsistent, you name it pathToFill and pathToStroke above and name the new path just 'path', but here you call it 'path' and the new path 'newPath'. Could you use one schema? E.g. clipPath here?

> WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:423
> +void GraphicsContext::clipPath(const Path& pathToClip, WindRule clipRule)

See above.

> WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:730
> +void GraphicsContext::fillPath(const Path& pathToFill)

Ditto.

> WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:1188
> +void GraphicsContext::strokePath(const Path& pathToStroke)

Ditto.

> WebCore/platform/graphics/wince/GraphicsContextWinCE.cpp:1332
> +    // FIXME: Be smarter about this.
> +    beginPath();
> +    addPath(path);

You use this pattern multiple times, how would you make it smarter? What is wrong with it right now? Just that you use the GC functions instead of platform code? Can you add more details please?

> WebCore/platform/graphics/wince/GraphicsContextWinCE.cpp:1386
> +    // FIXME: Be smarter about this.
> +    beginPath();
> +    addPath(path);
> +

Ditto. And on other places as well.

> WebCore/platform/graphics/wx/GraphicsContextWx.cpp:541
> +    // FIXME: Be smarter about this.
> +    beginPath();
> +    addPath(path);

Ditto. Won't continue this on the bug.

> WebCore/rendering/RenderObject.cpp:958
> -        graphicsContext->addPath(borderPath);
> -        graphicsContext->strokePath();
> +        graphicsContext->strokePath(borderPath);

hm, Is this correct? We added a Path to the existing one before, with your patch you delete the existing one. Is it possible that an old Path still exist? If not, where is the beginPath()?

> WebCore/rendering/svg/RenderSVGPath.cpp:186
> -        context->addPath(m_path);
> +        path = m_path;

this slows down the common case. Please don't copy the path here!

> WebCore/rendering/svg/RenderSVGPath.cpp:189
> +        strokePaintingResource->postApplyResource(this, context, ApplyToStrokeMode, path);

better to add something like nonScalingStroke ? path : m_path

> WebCore/rendering/svg/RenderSVGPath.cpp:194
> +            fallbackResource->postApplyResource(this, context, ApplyToStrokeMode, path);

ditto.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.