<rdar://problem/70891766>
Created attachment 412791 [details] Patch
Comment on attachment 412791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412791&action=review > Source/WebCore/ChangeLog:8 > + > + No new tests (OOPS!). This needs some explanation for the change. Remove the "No new tests" line. > Source/WebCore/html/HTMLCanvasElement.cpp:373 > + if (m_context) > + addObserver(document()); I don't think you should do this unconditionally. You should only do this when GPU Process is active. In fact, you should only do this when GPU Process is active, and drawing has occurred since the last flush. > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:310 > + bool needsPreparationForDisplay() const final { return true; } This can't be unconditional. You should only return true when using the GPU backend. Ideally it would only return true if there is new drawing.
Created attachment 412939 [details] Patch
(In reply to Simon Fraser (smfr) from comment #2) > Comment on attachment 412791 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=412791&action=review > > > Source/WebCore/ChangeLog:8 > > + > > + No new tests (OOPS!). > > This needs some explanation for the change. Remove the "No new tests" line. > > > Source/WebCore/html/HTMLCanvasElement.cpp:373 > > + if (m_context) > > + addObserver(document()); > > I don't think you should do this unconditionally. You should only do this > when GPU Process is active. In fact, you should only do this when GPU > Process is active, and drawing has occurred since the last flush. > > > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:310 > > + bool needsPreparationForDisplay() const final { return true; } > > This can't be unconditional. You should only return true when using the GPU > backend. Ideally it would only return true if there is new drawing. We only flush from flushDrawingContextAndCommit() if there're new items in display list after the last flush. Should that be enough?
Comment on attachment 412939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412939&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:375 > +#if ENABLE(GPU_PROCESS) > + if (m_context) > + addObserver(document()); > +#endif It seems inherently wrong for WebCore to have to care about the GPU Process, so this #ifdef should really never be used in WebCore.
Comment on attachment 412939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412939&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:375 > +#if ENABLE(GPU_PROCESS) > + if (m_context) > + addObserver(document()); > +#endif This needs to be a runtime check not a compile-time check, and should only happen when we make a remote image buffer. > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2051 > + if (buffer && buffer->renderingResourceIdentifier()) I don't think checking for a renderingResourceIdentifier is the right approach. Maybe add ImageBuffer::flushContextAsync() and just call it always, then specialize the remote image buffer to flush the display list etc.
Created attachment 412996 [details] Patch
Comment on attachment 412996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412996&action=review Almost there! Let's do one more round. > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:310 > + bool needsPreparationForDisplay() const final { return true; } This needs to also consult the buffer's shouldEagerlyFlush(), otherwise you've changed the behavior at Document::canvasChanged(). > Source/WebCore/platform/graphics/ImageBuffer.h:71 > + virtual bool shouldEagerlyFlush() { return false; } I'm trying to think of a better name for this. How about prefersPreparationForDisplay().
Created attachment 413079 [details] Patch
Okay. Changed the method name. I think this should be close to what we're looking for.
Comment on attachment 413079 [details] Patch Let's wait for the Windows bot.
Looks like it failed on first run and passed on retry.
Is it concerning? (In reply to Rini Patel from comment #12) > Looks like it failed on first run and passed on retry.
(In reply to Rini Patel from comment #13) > Is it concerning? > (In reply to Rini Patel from comment #12) > > Looks like it failed on first run and passed on retry. The Windows test failure (fast/visual-viewport/resize-event-fired-window-resized.html) looks unrelated — we're probably okay.
Committed r269331: <https://trac.webkit.org/changeset/269331> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413079 [details].
<rdar://problem/71008643>