Bug 231404 – Split DisplayList::Recorder into an abstract base class and a concrete implementation

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

More work towards streaming display lists.

This refactoring will allow us to introduce a WebKit2-specific subclass of DisplayList::Recorder in a subsequent patch, which is similar to the existing DisplayList::Recorder object, but immediately sends its display list items to the GPU Process through the IPC stream connection instead of appending them to an instance of WebCore::DisplayList.

[Wenson Hsieh]

Created attachment 440612 [details]
For EWS

[Wenson Hsieh]

Created attachment 440614 [details]
Fix GTK/WPE builds

[Wenson Hsieh]

Created attachment 440615 [details]
Fix GTK/WPE builds

[Wenson Hsieh]

Created attachment 440616 [details]
Fix GTK/WPE builds

[Wenson Hsieh]

Created attachment 440618 [details]
Fix GTK/WPE builds

[Wenson Hsieh]

Created attachment 440631 [details]
Fix CMake build

[Myles C. Maxfield]

Comment on attachment 440631 [details]
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?

[Wenson Hsieh]

Comment on attachment 440631 [details]
Fix CMake build

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

Thanks for the review!

>> Source/WebCore/ChangeLog:10
>> +        DisplayList::RecorderImpl. See below for more details.
> 
> Other places we use Concrete. Not sure which is better.

So I chatted with Tim (proposing either ConcreteRecorder and RecorderImpl), and we decided on RecorderImpl. Overall, we seem to use FooImpl a lot more than ConcreteFoo (I think ConcreteImageBuffer is actually the only instance?)

>> Source/WebCore/ChangeLog:44
>> +        Recorder::ContextState) when modifying graphics context state.
> 
> Cool. Enabling this code sharing was my biggest concern.

Indeed! This refactoring allows us to avoid a lot of the duplicate code that was in my hacky WIP.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:33
> +#include "DisplayListRecorderImpl.h"

Looks like this change is also unnecessary, so I'll change this back too.

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

It's not — good catch! Reverted this bit.

>> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:-66
>> -        LOG(DisplayLists, "Recorded display list:\n%s", m_displayList.description().data());
> 
> Who is using m_isNested now? Can it be deleted?

Good catch, it looks like this is actually *just* used inside `RecorderImpl::~RecorderImpl()` for a single logging statement. I'll just move this into the RecorderImpl, and remove the protected getter.

>> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:401
>> +    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?

Yeah, this is a bit unfortunate :/

I wasn't sure how to avoid this though, since we no longer templatize item recording methods, since they're now virtual. I'll add a FIXME in `appendStateChangeItemIfNecessary()`, and continue to think of ways to improve this.

>> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:140
>> +    virtual void recordResourceUse(ImageBuffer&) = 0;
> 
> I would have renamed these in a follow-up patch, to keep this one smaller.

Hm..I'm not sure how this would work though, since these are new virtual methods (not renamed versions of existing methods).

>> 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?

Oh, I moved `canDrawImageBuffer` into the protected section of the class, but forgot to move this comment. I'll move the FIXME to match the `canDrawImageBuffer` again.

[Wenson Hsieh]

Created attachment 440666 [details]
For EWS

[EWS]

Committed r283848 (242727@main): <https://commits.webkit.org/242727@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440666 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84047977>

[Darin Adler]

Comment on attachment 440631 [details]
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.
> 
> So I chatted with Tim (proposing either ConcreteRecorder and RecorderImpl), and we decided on RecorderImpl. Overall, we seem to use FooImpl a lot more than ConcreteFoo (I think ConcreteImageBuffer is actually the only instance?)

Sad to hear it. I personally like Concrete so much better than Impl!

But without our traditions … our lives would be as shaky as a programmer on the roof.