[webkit-reviews] review denied: [Bug 208464] Lazily generate CGPaths for some simple types of paths, such as arcs and lines : [Attachment 392930] v3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 208464: Lazily generate CGPaths for some simple types of paths, such as
arcs and lines
https://bugs.webkit.org/show_bug.cgi?id=208464

Attachment 392930: v3

https://bugs.webkit.org/attachment.cgi?id=392930&action=review




--- Comment #24 from Darin Adler <darin at apple.com> ---
Comment on attachment 392930
  --> https://bugs.webkit.org/attachment.cgi?id=392930
v3

View in context: https://bugs.webkit.org/attachment.cgi?id=392930&action=review

> Source/WebCore/platform/graphics/Path.cpp:209
> +#if ENABLE(INLINE_PATH_DATA)
> +
> +void Path::applyInlinePathDataIfNeeded() const
> +{
> +    if (!m_needsToApplyInlineData)
> +	   return;
> +
> +    m_needsToApplyInlineData = false;
> +
> +    if (!hasAnyInlineData())
> +	   return;
> +
> +    if (!m_path)
> +	   m_path = createPlatformPath();
> +
> +    applyInlinePathData(m_inlineData, m_path.get());
> +}
> +
> +#endif // ENABLE(INLINE_PATH_DATA)

I was wrong to say that this function is platform-independent. It’s not. It
should be deleted.

> Source/WebCore/platform/graphics/Path.h:252
> +    mutable bool m_needsToApplyInlineData { false };

This boolean is redundant. When we have inline data and m_path is null, this is
always true. When we have inline data and m_path is non-null, this is always
false. This function will always return the same thing as this boolean:

    bool needsToApplyInlineData() const { return !m_path && hasAnyInlineData();
}

So please get rid of this boolean and all the code that manages it.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:90
> +RetainPtr<CGMutablePathRef> Path::createPlatformPath()
> +{
> +    return adoptCF(CGPathCreateMutable());
> +}
> +
> +void Path::applyInlinePathData(const InlinePathData& data, CGMutablePathRef
path)
> +{

These functions should be merged into a single createPlatformPath function, a
const function that does not take any arguments or return any values. It should
start like this:

    if (m_path)
	return;
    m_path = adoptCF(CGPathCreateMutable());
    WTF::switchOn(m_inlinePathData,
	[&](Monostate) { }, // Start with an empty path.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:92
> +	   [&](auto) { ASSERT_NOT_REACHED(); },

This seems really peculiar. We want to get a compiler error if there’s any type
of data we fail to handle. But including this "auto" case means we won’t get
that. This should just cover Monostate, I think, not auto.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:263
> +    CGPathAddPath(path.get(), &transformCG, ensurePlatformPath());

This should be platformPath(), not ensurePlatformPath(). And we should
explicitly set m_inlineData = Monostate { } after setting m_path below.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:265
> +    auto path =
adoptCF(CGPathCreateMutableCopyByTransformingPath(ensurePlatformPath(),
&transformCG));

Ditto.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:274
>	   return CGRectZero;

This should return { } rather than CGRectZero. I see no reason to make this
different than the line below.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:295
>	   return CGRectZero;

This should return { } rather than CGRectZero. I see no reason to make this
different than the line below.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:308
> +    CGRect bound = CGPathGetBoundingBox(platformPath());
>      return CGRectIsNull(bound) ? CGRectZero : bound;

Why do we have two different boundingRect functions? It seems that the
boundingRect and fastBoundingRect function bodies are exactly the same.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:340
> +    if (isNull() || hasInlineData<MoveData>()) {
> +	   m_inlineData = MoveData { point };
> +	   m_needsToApplyInlineData = true;
> +	   return;
> +    }

This is the platform-independent code that I would like to share if possible.
If we did that it means that the general case of moveTo needs to be renamed to
something like moveToSlowCase or whatever we want to call the non-inline-data
case.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:351
> +    if (isNull() || hasInlineData<MoveData>()) {
> +	   m_inlineData = LineData { hasAnyInlineData() ?
WTF::get<MoveData>(m_inlineData).location : FloatPoint(), p };
> +	   m_needsToApplyInlineData = true;
> +	   return;
> +    }

Ditto.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:439
> +    if (isNull() || hasInlineData<MoveData>()) {
> +	   m_inlineData = ArcData { hasAnyInlineData() ?
WTF::get<MoveData>(m_inlineData).location : FloatPoint(), p, radius,
startAngle, endAngle, clockwise, hasAnyInlineData() };
> +	   m_needsToApplyInlineData = true;
> +	   return;
> +    }

Ditto. Probably have to share the workaround too.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:499
> +    if (isNull())
> +	   return true;
> +
> +    if (hasAnyInlineData())
> +	   return false;

Would be nice to share this part.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:518
>      if (isNull())
> -	   return FloatPoint();
> -    return CGPathGetCurrentPoint(m_path.get());
> +	   return { };
> +
> +    if (hasInlineData<MoveData>())
> +	   return WTF::get<MoveData>(m_inlineData).location;
> +
> +    if (hasInlineData<LineData>())
> +	   return WTF::get<LineData>(m_inlineData).end;

Would be nice to share this part.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:572
>      if (isNull())
>	   return;
>  
> -    CGPathApply(m_path.get(), (void*)&function, CGPathApplierToPathApplier);
> +    if (hasInlineData<MoveData>()) {
> +	   PathElement element;
> +	   element.type = PathElement::Type::MoveToPoint;
> +	   element.points[0] = WTF::get<MoveData>(m_inlineData).location;
> +	   function(element);
> +	   return;
> +    }
> +
> +    if (hasInlineData<LineData>()) {
> +	   auto& line = WTF::get<LineData>(m_inlineData);
> +	   PathElement element;
> +	   element.type = PathElement::Type::MoveToPoint;
> +	   element.points[0] = line.start;
> +	   function(element);
> +	   element.type = PathElement::Type::AddLineToPoint;
> +	   element.points[0] = line.end;
> +	   function(element);
> +	   return;
> +    }

Would be nice to share this part.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:585
> +    if (hasInlineData<MoveData>())
> +	   return 1;
> +
> +    if (hasInlineData<LineData>())
> +	   return 2;

And this part.

> Source/WebKit/Platform/IPC/ArgumentCoders.cpp:212
> +Optional<Monostate> ArgumentCoder<Monostate>::decode(Decoder& decoder)

Should omit the argument name "decoder" since the value is unused.