[webkit-reviews] review denied: [Bug 48516] GraphicsContext: Remove "current path" and have strokePath, fillPath and clipPath take a Path argument : [Attachment 74492] Proposed patch v3

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Andreas Kling <kling at webkit.org>'s
request for review:
Bug 48516: GraphicsContext: Remove "current path" and have strokePath, fillPath
and clipPath take a Path argument
https://bugs.webkit.org/show_bug.cgi?id=48516

Attachment 74492: Proposed patch v3
https://bugs.webkit.org/attachment.cgi?id=74492&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
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.