Bug 218720 – Rename ImageBuffer::flushDisplayList to ImageBuffer::submitDisplayList

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Wenson Hsieh]

ImageBuffer's `flushDisplayList` method is currently only responsible for replaying the given display list into the image buffer's drawing context, and doesn't actually guarantee a context flush. A name like `renderDisplayList` seems more fitting.

[Said Abou-Hallawa]

(In reply to Wenson Hsieh from comment #0)
> ImageBuffer's `flushDisplayList` method is currently only responsible for
> replaying the given display list into the image buffer's drawing context,
> and doesn't actually guarantee a context flush. A name like
> `renderDisplayList` seems more fitting.

I think this statement is a little inaccurate. The "flushDisplayList" is responsible for replaying the given display list into the image buffer's "backend context".

Using the "drawing context" is a little vague here because DisplayList::DrawingContext refers to the class that wraps the DisplayList::Recorder.

Regarding the new name "renderDisplayList", I am not sure why it is better than flushDisplayList() especially when we deal with cross process operation and where WebP does not really renders the display list to the backend context.

[Simon Fraser (smfr)]

flush() implies "play this out until the end", and normally flush is used for a function that doesn't take arguments; it's "flush out existing state".

So flush(DisplayList&) is a bit ambiguous. Is there already some internal DisplayList that gets flushed out first?

[Wenson Hsieh]

Created attachment 413618 [details]
Patch

[Tim Horton]

How about "submit"?

[Wenson Hsieh]

(In reply to Tim Horton from comment #4)
> How about "submit"?

"submitDisplayList" sounds fine to me (I think I actually prefer it to "renderDisplayList", since "submit" is a more accurate word to describe what happens in RemoteImageBufferProxy).

[Said Abou-Hallawa]

For me, 'flush' means "When you empty something, you flush it." This means you just get rid of it and you do not have to know where it will end up.

And yes all, we only flush the internal DisplayList of the ImageBuffer to its remote backend. So we should only be flushing the ImageBuffer's existence state.

I think the problem is flushDisplayList() is used in both RemoteImageBufferProxy and RemoteImageBuffer. In RemoteImageBufferProxy, it is used to send the DisplayList to GPUP. But in RemoteImageBuffer, it is used to replay, apply or render the DisplayList into the backend.

My suggestion is:

Remove the virtual function ImageBuffer::flushDisplayList()
Remove RemoteImageBuffer::flushDisplayList() and inline it in RemoteRenderingBackend::applyDisplayList()
Make RemoteImageBufferProxy::flushDisplayList() private none virtual.

[Wenson Hsieh]

(In reply to Said Abou-Hallawa from comment #6)
> For me, 'flush' means "When you empty something, you flush it." This means
> you just get rid of it and you do not have to know where it will end up.
> 
> And yes all, we only flush the internal DisplayList of the ImageBuffer to
> its remote backend. So we should only be flushing the ImageBuffer's
> existence state.
> 
> I think the problem is flushDisplayList() is used in both
> RemoteImageBufferProxy and RemoteImageBuffer. In RemoteImageBufferProxy, it
> is used to send the DisplayList to GPUP. But in RemoteImageBuffer, it is
> used to replay, apply or render the DisplayList into the backend.
> 
> My suggestion is:
> 
> Remove the virtual function ImageBuffer::flushDisplayList()
> Remove RemoteImageBuffer::flushDisplayList() and inline it in
> RemoteRenderingBackend::applyDisplayList()
> Make RemoteImageBufferProxy::flushDisplayList() private none virtual.

(We chatted about this on Slack, and decided to use the name "submitDisplayList" for now to deal with the confusion around flushing, and then remove `ImageBuffer::flushDisplayList` later, which will require some additional refactoring around ConcreteImageBuffer)

[Wenson Hsieh]

Created attachment 413625 [details]
Patch

[Wenson Hsieh]

Created attachment 413627 [details]
Patch

[EWS]

Committed r269606: <https://trac.webkit.org/changeset/269606>

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