Summary: | Create a new ImageBuffer type for drawing on a DisplayList | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||
Component: | Canvas | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | alecflett, annulen, beidson, cdumez, commit-queue, dino, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, graouts, gyuyoung.kim, hta, jer.noble, jsbell, kondapallykalyan, noam, pdr, philipj, ryuan.choi, schenney, sergio, simon.fraser, thorton, tommyw, webkit-bug-importer, wenson_hsieh | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 207048 | ||||||||||||||||
Bug Blocks: | 204955, 207134 | ||||||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2020-02-03 00:11:04 PST
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. *** Bug 204803 has been marked as a duplicate of this bug. *** |