The drawing context will be the context of a display list. The ImageBuffer operations will ensure the recorded displaylist is replayed back before getting the pixels of the ImageBufferBackend.
Created attachment 389493 [details] Patch
Created attachment 389506 [details] Patch for review
Created attachment 391136 [details] Patch
Created attachment 391168 [details] Patch
Comment on attachment 391168 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391168&action=review > Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:49 > + const DisplayList* replayDisplayList() const { return m_replayDisplayList.get(); } "replay" sounds like a verb, but it's just a getter
Comment on attachment 391168 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391168&action=review >> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:49 >> + const DisplayList* replayDisplayList() const { return m_replayDisplayList.get(); } > > "replay" sounds like a verb, but it's just a getter Should I change it to replayedDisplayList()? The reason I chose this name is the caller is HTMLCanvasElement::replayDisplayListAsText() which is called from Internals::replayDisplayListForElement(). So I think if I am going to change the name of this function, I have to change all the callers with the prefix replayDisplayList to start with replayedDisplayList.
Comment on attachment 391168 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391168&action=review >>> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:49 >>> + const DisplayList* replayDisplayList() const { return m_replayDisplayList.get(); } >> >> "replay" sounds like a verb, but it's just a getter > > Should I change it to replayedDisplayList()? The reason I chose this name is the caller is HTMLCanvasElement::replayDisplayListAsText() which is called from Internals::replayDisplayListForElement(). So I think if I am going to change the name of this function, I have to change all the callers with the prefix replayDisplayList to start with replayedDisplayList. replayedDisplayList is fine. > Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:69 > + void replayDisplayList(GraphicsContext& destContext) > + { > + if (!m_displayList.itemCount()) > + return; > + > + Replayer replayer(destContext, m_displayList); > + if (m_tracksDisplayListReplay) > + m_replayDisplayList = replayer.replay({ }, m_tracksDisplayListReplay); > + else > + replayer.replay(); > + m_displayList.clear(); > + } > + Add a .cpp file; I don't think this should be inline. > Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:35 > +class ImageBuffer : public ConcreteImageBuffer<BackendType> , public DrawingContext { Extra space after > Should DrawingContext be added via composition rather than inheritance? > Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:58 > + ~ImageBuffer() > + { > + flushDrawingContext(); > + } Hmm, why flush? No-one can use the buffer after this. If flush triggers any callbacks on |this| we could have a bad time. > Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:60 > + DrawingContext* drawingContext() override { return this; } Weird. > Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:67 > + GraphicsContext& context() const override > + { > + return DrawingContext::context(); > + } > + > + void flushDrawingContext() override override -> final?
Comment on attachment 391168 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391168&action=review >> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:35 >> +class ImageBuffer : public ConcreteImageBuffer<BackendType> , public DrawingContext { > > Extra space after > > > Should DrawingContext be added via composition rather than inheritance? It can but this will make this class just a bridge to the DrawingContext public methods. I will try doing it. >> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:58 >> + } > > Hmm, why flush? No-one can use the buffer after this. If flush triggers any callbacks on |this| we could have a bad time. I hit an assertion in the destructor of GraphicsContext because ASSERT(m_stack.isEmpty()). This call fixed this assertion. I will try to find a way to flush the GraphicsContext without having to flush the DrawingContext.
Created attachment 391298 [details] Patch
Comment on attachment 391168 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391168&action=review >>>> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:49 >>>> + const DisplayList* replayDisplayList() const { return m_replayDisplayList.get(); } >>> >>> "replay" sounds like a verb, but it's just a getter >> >> Should I change it to replayedDisplayList()? The reason I chose this name is the caller is HTMLCanvasElement::replayDisplayListAsText() which is called from Internals::replayDisplayListForElement(). So I think if I am going to change the name of this function, I have to change all the callers with the prefix replayDisplayList to start with replayedDisplayList. > > replayedDisplayList is fine. Done. >> Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:69 >> + > > Add a .cpp file; I don't think this should be inline. Added. >>> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:35 >>> +class ImageBuffer : public ConcreteImageBuffer<BackendType> , public DrawingContext { >> >> Extra space after > >> >> Should DrawingContext be added via composition rather than inheritance? > > It can but this will make this class just a bridge to the DrawingContext public methods. I will try doing it. Done. >>> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:58 >>> + } >> >> Hmm, why flush? No-one can use the buffer after this. If flush triggers any callbacks on |this| we could have a bad time. > > I hit an assertion in the destructor of GraphicsContext because ASSERT(m_stack.isEmpty()). This call fixed this assertion. I will try to find a way to flush the GraphicsContext without having to flush the DrawingContext. I think calling flushDrawingContext() from the destructor is the right sequence. The destructor of HTMLCanvasElement calls setImageBuffer(nullptr) which nullifies m_contextStateSaver. The destructor of GraphicsContextStateSaver calls m_context.restore(). With the DisplayList::ImageBuffer, GraphicsContext::restore() will add a Restore element to the DisplayList. When setImageBuffer() nullifies the m_imageBuffer, DisplayList::ImageBuffer::~ImageBuffer() replays back the Restore item to the ImageBufferBackend which is equivalent to the case of the none DisplayList case. >> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:60 >> + DrawingContext* drawingContext() override { return this; } > > Weird. Fixed by having a member of type DrawingContext. >> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:67 >> + void flushDrawingContext() override > > override -> final? No because this method is going to be overridden by RemoteImageBuffer.
Comment on attachment 391298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391298&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:845 > + if (buffer() && buffer()->drawingContext() && buffer()->drawingContext()->replayedDisplayList()) > + return buffer()->drawingContext()->replayedDisplayList()->asText(flags); Can we if (!buffer()) return and use a buffer ref for the rest of the function? > Source/WebCore/platform/graphics/displaylists/DisplayListDrawingContext.h:28 > +#include "AffineTransform.h" Not needed?
Created attachment 391406 [details] Patch
The commit-queue encountered the following flaky tests while processing attachment 391406 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.
Comment on attachment 391406 [details] Patch Clearing flags on attachment: 391406 Committed r257156: <https://trac.webkit.org/changeset/257156>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59678215>
*** Bug 204803 has been marked as a duplicate of this bug. ***