[webkit-reviews] review granted: [Bug 206068] REGRESSION (r254291): [ Catalina wk2 Debug ] Flaky ASSERT on fast/images/animated-image-loop-count.html : [Attachment 387509] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Chris Lord <clord at igalia.com>'s
request for review:
Bug 206068: REGRESSION (r254291): [ Catalina wk2 Debug ] Flaky ASSERT on
fast/images/animated-image-loop-count.html
https://bugs.webkit.org/show_bug.cgi?id=206068

Attachment 387509: Patch

https://bugs.webkit.org/attachment.cgi?id=387509&action=review




--- Comment #6 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 387509
  --> https://bugs.webkit.org/attachment.cgi?id=387509
Patch

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

I think this would work fine but I have a couple of nits.

> Source/WebCore/platform/graphics/ImageSource.cpp:349
> +    decodingQueue().dispatch([protectedThis = makeRef(*this),
protectedDecodingQueue = makeRef(decodingQueue()), protectedFrameRequestQueue =
makeRef(frameRequestQueue()), protectedDecoder = makeRef(*m_decoder), sourceURL
= sourceURL().string().isolatedCopy()] () mutable {

I do not understand why we're capturing the protectedDecodingQueue here, is
does not appear to be needed. WorkQueue::dispatch() already takes care of
ref'ing the WorkQueue object internally.

> Source/WebCore/platform/graphics/ImageSource.cpp:385
> +	   // Ensure destruction happens on creation thread

Missing period at the end of the comment (per coding style).

> Source/WebCore/platform/graphics/ImageSource.cpp:386
> +	   protectedThis->m_runLoop.dispatch([protectedThis =
WTFMove(protectedThis), protectedQueue = WTFMove(protectedDecodingQueue),
protectedDecoder = WTFMove(protectedDecoder)] () mutable { });

I see no good reason to dispatch the protectedQueue here, I don't think it
should even have been captured in the first place.