Test: imported/w3c/web-platform-tests/html/canvas/offscreen/drawing-images-to-the-canvas/2d.drawImage.zerosource.html Recently, this test was updated in WPT[1] to avoid raising IndexSizeError in drawImage for empty sources, as the spec at [2] does not specify it should be raised in this case. In WebKit, this change was pulled in r269598 and the test has been flaky raising this exception as a console message (the exception is swallowed by the test helper code). [1] https://github.com/web-platform-tests/wpt/issues/2835 [2] https://html.spec.whatwg.org/multipage/canvas.html#check-the-usability-of-the-image-argument
Created attachment 414413 [details] Tentative patch Moving the check for empty source to the start of drawImage(ImageBitmap). Tested locally with fast/canvas and imported/w3c/web-platform-tests/html/canvas without regressions.
Created attachment 414539 [details] Updated patch Although the WPT issue links the 'check usability of the image' section, the correct check this test is about is whether one of the dimensions of the src rect is zero.
Comment on attachment 414539 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=414539&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1668 > + if (!srcRect.width() != !srcRect.height()) Shouldn't this be || instead of != ?
Comment on attachment 414539 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=414539&action=review >> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1668 >> + if (!srcRect.width() != !srcRect.height()) > > Shouldn't this be || instead of != ? Or even better, srcRect.isEmpty()
(In reply to Noam Rosenthal from comment #4) > Comment on attachment 414539 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=414539&action=review > > >> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1668 > >> + if (!srcRect.width() != !srcRect.height()) > > > > Shouldn't this be || instead of != ? > > Or even better, srcRect.isEmpty() I thought this on initial read, but I think it's correct - it's checking if only one of the parameters is zero. If both are zero, you want the exception, which is checked in the next line (though it superfluously still has both dimensions in the check, so makes this read a bit confusingly to me). Maybe this would read less confusingly as if (srcRect.isEmpty()) return (srcRect.width() || srcRect.height()) ? { } : { IndexSizeError };
Comment on attachment 414539 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=414539&action=review >>>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1668 >>>> + if (!srcRect.width() != !srcRect.height()) >>> >>> Shouldn't this be || instead of != ? >> >> Or even better, srcRect.isEmpty() > > I thought this on initial read, but I think it's correct - it's checking if only one of the parameters is zero. If both are zero, you want the exception, which is checked in the next line (though it superfluously still has both dimensions in the check, so makes this read a bit confusingly to me). > > Maybe this would read less confusingly as > > if (srcRect.isEmpty()) > return (srcRect.width() || srcRect.height()) ? { } : { IndexSizeError }; Yes, it's confusing. I would suggest if (srcRect.size().isZero()) return { IndexSizeError }; if (srcRect.isEmpty()) return { };
(In reply to Noam Rosenthal from comment #6) > Yes, it's confusing. I would suggest > > if (srcRect.size().isZero()) > return { IndexSizeError }; > > if (srcRect.isEmpty()) > return { }; That's much nicer, will have to make a note of that for the future...
Comment on attachment 414539 [details] Updated patch r- based on feedback
Comment on attachment 414539 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=414539&action=review > Source/WebCore/ChangeLog:9 > + Can you put a link to a hash in the spec? I couldn't find this... >>>>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1668 >>>>> + if (!srcRect.width() != !srcRect.height()) >>>> >>>> Shouldn't this be || instead of != ? >>> >>> Or even better, srcRect.isEmpty() >> >> I thought this on initial read, but I think it's correct - it's checking if only one of the parameters is zero. If both are zero, you want the exception, which is checked in the next line (though it superfluously still has both dimensions in the check, so makes this read a bit confusingly to me). >> >> Maybe this would read less confusingly as >> >> if (srcRect.isEmpty()) >> return (srcRect.width() || srcRect.height()) ? { } : { IndexSizeError }; > > Yes, it's confusing. I would suggest > > if (srcRect.size().isZero()) > return { IndexSizeError }; > > if (srcRect.isEmpty()) > return { }; btw, where in the standard does it say that if both are zero there should be an exception? The patch and the Changelog should be more explicit about this.
Comment on attachment 414539 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=414539&action=review >> Source/WebCore/ChangeLog:9 >> + > > Can you put a link to a hash in the spec? I couldn't find this... Sure. https://html.spec.whatwg.org/multipage/canvas.html#drawing-images I'll add it to the changelog. >>>>>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1668 >>>>>> + if (!srcRect.width() != !srcRect.height()) >>>>> >>>>> Shouldn't this be || instead of != ? >>>> >>>> Or even better, srcRect.isEmpty() >>> >>> I thought this on initial read, but I think it's correct - it's checking if only one of the parameters is zero. If both are zero, you want the exception, which is checked in the next line (though it superfluously still has both dimensions in the check, so makes this read a bit confusingly to me). >>> >>> Maybe this would read less confusingly as >>> >>> if (srcRect.isEmpty()) >>> return (srcRect.width() || srcRect.height()) ? { } : { IndexSizeError }; >> >> Yes, it's confusing. I would suggest >> >> if (srcRect.size().isZero()) >> return { IndexSizeError }; >> >> if (srcRect.isEmpty()) >> return { }; > > btw, where in the standard does it say that if both are zero there should be an exception? The patch and the Changelog should be more explicit about this. Yeah, this is another issue. At first I thought it was related to the "check usability of image", but IndexSizeError is not mentioned there, only InvalidStateError. IndexSizeError is mentioned in the canvas page [1] only in the following contexts: * arcTo(), arc(), ellipse() * addColorStop(), createRadialGradient() * createImageData(), ImageData(), getImageData() * convertToBlob() Testing a version that only checks if srcRect is empty and returns {}, I could not find any consistent regressions and even the number of flakies (Usually 10-15 upstream) is slightly less with the single check. I'll submit an updated version. [1] https://html.spec.whatwg.org/multipage/canvas.html
Created attachment 414649 [details] Patch without exception Updated patch removing the exception. Also updated another old createBitmap test that explicitly checked for IndexSizeError.
Comment on attachment 414649 [details] Patch without exception Much better. LGTM!
Committed r270126: <https://trac.webkit.org/changeset/270126> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414649 [details].
<rdar://problem/71637171>