[webkit-reviews] review granted: [Bug 231404] Split DisplayList::Recorder into an abstract base class and a concrete implementation : [Attachment 440631] Fix CMake build

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 231404: Split DisplayList::Recorder into an abstract base class and a
concrete implementation
https://bugs.webkit.org/show_bug.cgi?id=231404

Attachment 440631: Fix CMake build

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




--- Comment #7 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 440631
  --> https://bugs.webkit.org/attachment.cgi?id=440631
Fix CMake build

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

> Source/WebCore/ChangeLog:10
> +	   DisplayList::RecorderImpl. See below for more details.

Other places we use Concrete. Not sure which is better.

> Source/WebCore/ChangeLog:44
> +	   data or drawing simple paths. The base class also manages updating
and maintaining the state stack (a list of
> +	   Recorder::ContextState) when modifying graphics context state.

Cool. Enabling this code sharing was my biggest concern.

> Source/WebCore/platform/graphics/cocoa/GraphicsContextCocoa.mm:29
> +#import "DisplayListRecorderImpl.h"

Is this necessary?

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:-66
> -    if (!m_isNested)
> -	   LOG(DisplayLists, "Recorded display list:\n%s",
m_displayList.description().data());

Who is using m_isNested now? Can it be deleted?

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:401
> -    append<StrokeEllipse>(rect);
> +    appendStateChangeItemIfNecessary();
> +    recordStrokeEllipse(rect);

It seems unfortunate that the appendStateChange logic had to be moved from one
centralized place into all its callers, duplicating the call. Is there a better
way? Can we at least do some code sharing?

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:140
> +    virtual void recordResourceUse(NativeImage&) = 0;
> +    virtual void recordResourceUse(Font&) = 0;
> +    virtual void recordResourceUse(ImageBuffer&) = 0;

I would have renamed these in a follow-up patch, to keep this one smaller.

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:189
>      // FIXME: Maybe remove this?

Looks like you're removing this. Perhaps the comment can be removed too?