Trying to take empty snapshots of the view should not cause crashes.
Created attachment 431685 [details] Returns a NSError
Comment on attachment 431685 [details] Returns a NSError View in context: https://bugs.webkit.org/attachment.cgi?id=431685&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1178 > + if (CGRectIsEmpty(rectInViewCoordinates) || !snapshotWidth) { > + RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)]() mutable { > + completionHandler(nil, createNSError(WKErrorUnknown).get()); > + }); > + return; > + } Is returning WKErrorUnknown better than returning an empty image? NSImage appears to support an empty image constructor, with the caveat that using the image later might cause you problems: "It is permissible to initialize the image object by passing a size of (0.0, 0.0); however, you must set the size to a non-zero value before using it or an exception will be raised." [ https://developer.apple.com/documentation/appkit/nsimage/1520033-initwithsize?language=objc ] Perhaps those problems will be clearer than WKErrorUnknown.
(In reply to Geoffrey Garen from comment #2) > Comment on attachment 431685 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=431685&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1178 > > + if (CGRectIsEmpty(rectInViewCoordinates) || !snapshotWidth) { > > + RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)]() mutable { > > + completionHandler(nil, createNSError(WKErrorUnknown).get()); > > + }); > > + return; > > + } > > Is returning WKErrorUnknown better than returning an empty image? NSImage > appears to support an empty image constructor, with the caveat that using > the image later might cause you problems: > > "It is permissible to initialize the image object by passing a size of (0.0, > 0.0); however, you must set the size to a non-zero value before using it or > an exception will be raised." [ > https://developer.apple.com/documentation/appkit/nsimage/1520033- > initwithsize?language=objc ] > > Perhaps those problems will be clearer than WKErrorUnknown. If you prefer, I can certainly do that. I tried to maintain pre-existing behavior in release builds (we would hit the assertion in debug).
(In reply to Chris Dumez from comment #3) > (In reply to Geoffrey Garen from comment #2) > > Comment on attachment 431685 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=431685&action=review > > > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1178 > > > + if (CGRectIsEmpty(rectInViewCoordinates) || !snapshotWidth) { > > > + RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)]() mutable { > > > + completionHandler(nil, createNSError(WKErrorUnknown).get()); > > > + }); > > > + return; > > > + } > > > > Is returning WKErrorUnknown better than returning an empty image? NSImage > > appears to support an empty image constructor, with the caveat that using > > the image later might cause you problems: > > > > "It is permissible to initialize the image object by passing a size of (0.0, > > 0.0); however, you must set the size to a non-zero value before using it or > > an exception will be raised." [ > > https://developer.apple.com/documentation/appkit/nsimage/1520033- > > initwithsize?language=objc ] > > > > Perhaps those problems will be clearer than WKErrorUnknown. > > If you prefer, I can certainly do that. I tried to maintain pre-existing > behavior in release builds (we would hit the assertion in debug). I am also unsure how to create an empty UIImage. I don't see a similar API as for NSImage.
Found in the context of rdar://79424132.
Created attachment 431692 [details] Returns an empty image
Comment on attachment 431692 [details] Returns an empty image View in context: https://bugs.webkit.org/attachment.cgi?id=431692&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1180 > + completionHandler(image.get(), nil); I'm a little confused. The image param is nullable, shouldn't we just do that? (as a consumer of many cocoa APIs, that seems less surprising to me than this)
(In reply to Tim Horton from comment #7) > Comment on attachment 431692 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=431692&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1180 > > + completionHandler(image.get(), nil); > > I'm a little confused. The image param is nullable, shouldn't we just do > that? (as a consumer of many cocoa APIs, that seems less surprising to me > than this) So your preference would be a nil image AND a nil NSError?
(In reply to Chris Dumez from comment #8) > (In reply to Tim Horton from comment #7) > > Comment on attachment 431692 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=431692&action=review > > > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1180 > > > + completionHandler(image.get(), nil); > > > > I'm a little confused. The image param is nullable, shouldn't we just do > > that? (as a consumer of many cocoa APIs, that seems less surprising to me > > than this) > > So your preference would be a nil image AND a nil NSError? Our documentation says "The completionHandler is passed the image of the viewport contents or an error." My first patch was passing an error, my second one is passing an empty image. I think either would be fine but we need to agree on something and fix the crashes on the bots :)
(In reply to Chris Dumez from comment #9) > (In reply to Chris Dumez from comment #8) > > (In reply to Tim Horton from comment #7) > > > Comment on attachment 431692 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=431692&action=review > > > > > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1180 > > > > + completionHandler(image.get(), nil); > > > > > > I'm a little confused. The image param is nullable, shouldn't we just do > > > that? (as a consumer of many cocoa APIs, that seems less surprising to me > > > than this) > > > > So your preference would be a nil image AND a nil NSError? > > Our documentation says "The completionHandler is passed the image of the > viewport contents or an error." > > My first patch was passing an error, my second one is passing an empty > image. I think either would be fine but we need to agree on something and > fix the crashes on the bots :) Ahaa, fair :) I think either is definitely fine.
I guess.. you did ask for an empty image, and you got an empty image, so I will r+ that one.
Committed r279006 (238931@main): <https://commits.webkit.org/238931@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431692 [details].
<rdar://problem/79468888>
> I guess.. you did ask for an empty image, and you got an empty image FWIW, that was my thinking too.