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

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has canceled 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 392651: Patch

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




--- Comment #8 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 392651
  --> https://bugs.webkit.org/attachment.cgi?id=392651
Patch

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

>> Source/WebCore/ChangeLog:63
>> +	    the NotInline state and no longer attempt to store the path
mutations as inline data on Path.
> 
> This state machine is hard to follow especially the difference between
Cleared and NotInline is very subtle. I have the following suggestion. If Path
class has the following data
> 
> PlatformPathStorageType m_path;
> Optional<InlinePathData> m_inlineData;
> 
> Then we can have four states for these two members:
> 
> m_path	  m_inlineData		  Meaning
> 
> null		  null			  Initial state
> null		  not null		  Path is inline but is not applied to
m_path
> not null	  null			  Path can't be inline
> not null	  not null		  Path is inline and it is applied to
m_path

Okay, I’ll give this a try.

>> Source/WebCore/platform/graphics/Path.h:155
>> +#endif
> 
> I don't think it's worth keeping this inline for the non-USE(CG) case. Seems
cleaner to make them both out of line.

����

>> Source/WebCore/platform/graphics/Path.h:259
>> +	};
> 
> I think these structures should also include methods for encoding, decoding
and applying the data to m_path

����

>>> Source/WebCore/platform/graphics/Path.h:285
>>> +	 InlineDataType m_inlineDataType { InlineDataType::Cleared };
>> 
>> I think this should be part of InlinePathData
>> 
>> struct InlinePathData {
>>     InlineDataType inlineDataType;
>>     MoveData move;
>>     LineData line;
>>     ArcData arc;
>> };
> 
> I mean
> 
> struct InlinePathData {
>     InlineDataType inlineDataType;
>     union {
>	  MoveData move;
>	  LineData line;
>	  ArcData arc;
>     };
> };

This sounds good! I’ll change it to this:

struct InlinePathData {
    InlineDataType type;
    union {
	MoveData move;
	LineData line;
	ArcData arc;
    };
};

(so that I can just use `m_inlineData.type` instead of
`m_inlineData.inlineDataType`).

>> Source/WebCore/platform/graphics/Path.h:296
>> +	encoder << static_cast<uint8_t>(m_inlineDataType);
> 
> Need to use the enum encoder.

����

>> Source/WebCore/platform/graphics/Path.h:357
>> +	path.m_inlineDataType =
static_cast<InlineDataType>(inlineDataTypeValue);
> 
> No, you have to use the enum decoder.

����

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:68
>> +static FloatRect rectContainingPoints(FloatPoint point1, FloatPoint point2)
> 
> I would expect we have this elsewhere. if not, please put in GeometryUtils.h

I couldn’t find it elsewhere, so I made a new helper on GeometryUtils

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:351
>> +	if (m_inlineDataType <= InlineDataType::Move) {
> 
> There's an ordering dependency between the values of InlineDataType which is
not apparent from its declaration. You should add a comment that makes that
dependency super clear, or not rely on comparisons here.

Good call; I’ll change this to compare directly.