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

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted 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 392942: v4

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




--- Comment #29 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 392942
  --> https://bugs.webkit.org/attachment.cgi?id=392942
v4

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

This patch looks good.

> Source/WebCore/platform/graphics/InlinePathData.h:48
> +    float start { 0 };

This is ok as-is. No change is needed. A better name would be startAngle and
endAngle for these fields with a comment that these are in radians or degrees
or whatever unit because:

1. Makes it a tiny bit more clear what these are.
2. It breaks symmetry with LineData's identically named,  but different purpose
fields. <-- this is good because it matches intuition: different concepts
should be given different names.

> Source/WebCore/platform/graphics/InlinePathData.h:51
> +    bool hasOffset { false };

This is ok as-is. No change is needed. The optimal solution is to use
Optional<FloatPoint> for the offset because:

1. It is the modern C++ way
2. Can remove this bool (the Optional has one)
3. Because of (1) more likely a person will remember to check if offset is not
nullopt over a separate bool due to API conveniences. This means less error
prone to use this data.

> Source/WebCore/platform/graphics/InlinePathData.h:74
> +    return { data };

This is ok as-is. No change is needed. The braces are not needed.

> Source/WebCore/platform/graphics/InlinePathData.h:92
> +    return { WTFMove(data) };

This is ok as-is. No change is needed. The optimal solution is to just return
data because:

1. Takes advantage of RVO

> Source/WebCore/platform/graphics/InlinePathData.h:130
> +    return { WTFMove(data) };

Ditto.

> Source/WebCore/platform/graphics/Path.cpp:253
> +	   m_inlineData = ArcData { hasAnyInlineData() ?
WTF::get<MoveData>(m_inlineData).location : FloatPoint(), point, radius,
startAngle, endAngle, anticlockwise, hasAnyInlineData() };

This is ok-is. No change is needed. The optimal solution would be to initialize
each struct member individually then move the struct into the the instance
variable because:

1. The names of the fields are self describing so no need to ever have to
consider argument order if in the future this struct is expanded.
2. Removes the need to EXPLICITLY initialize location if hasAnyInlineDate() is
false.
3. There are many arguments to pass, which increases the chance of passing them
in the wrong order in the future if the struct is expanded or layout changed.

> Source/WebCore/platform/graphics/Path.h:322
> +	   return { WTFMove(path) };

This is ok as-is. No change needed. The optimal  solution is to just return
path for the same reasons outlined earlier in this patch.

> Source/WebCore/platform/graphics/Path.h:386
> +    return { WTFMove(path) };

Ditto.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:1231
> +    context.strokePath(WTFMove(m_path));

Is it OK to move this? If so, why?

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:672
> +    HRESULT hr =
GraphicsContext::systemFactory()->CreateGeometryGroup(D2D1_FILL_MODE_WINDING,
nullptr, 0, &m_path);

This is ok as-is. No change needed. A better solution is to either:
1. Add inline comment to describe 0

Or

2. Define local with value 0 and pass it.

Either one because it makes the purpose of the 0 clear.