ImageBitmapRenderingContext: transferFromImageBitmap should just be transfer #3167
Labels
Comments
hubot
pushed a commit
to WebKit/WebKit-http
that referenced
this issue
Oct 30, 2017
https://bugs.webkit.org/show_bug.cgi?id=178992 <rdar://problem/34147157> Reviewed by Sam Weinig. LayoutTests/imported/w3c: Update the expected results now more of the interface has been added. * web-platform-tests/imagebitmap-renderingcontext/bitmaprenderer-as-imagesource-expected.txt: * web-platform-tests/imagebitmap-renderingcontext/context-creation-with-alpha-expected.txt: * web-platform-tests/imagebitmap-renderingcontext/tranferFromImageBitmap-null-expected.txt: * web-platform-tests/imagebitmap-renderingcontext/transferFromImageBitmap-detached-expected.txt: Source/WebCore: Implement the "bitmaprenderer" context for HTMLCanvasElement. Use as much of the existing 2d context code as possible, which means that we are painting the ImageBitmap into the backing store. This isn't optimal, but it is good enough to get going while we move buffer ownership from the canvas object into the rendering contexts. This patch raised a few issues on the HTML specification: whatwg/html#3164 whatwg/html#3165 whatwg/html#3166 whatwg/html#3167 Update existing Web Platform Test results. * html/HTMLCanvasElement.cpp: (WebCore::HTMLCanvasElement::createContext2d): Drive-by comment fix. (WebCore::HTMLCanvasElement::createContextWebGL): Ditto. (WebCore::HTMLCanvasElement::createContextWebGPU): Ditto. (WebCore::HTMLCanvasElement::createContextBitmapRenderer): We now require a layer/backing store for a bitmap context. (WebCore::HTMLCanvasElement::paintsIntoCanvasBuffer const): Add bitmap-renderer to the type of context that uses the canvas's backing store. (WebCore::HTMLCanvasElement::createImageBuffer const): Comment fix. (WebCore::HTMLCanvasElement::setImageBuffer const): Change parameter to a &&. (WebCore::HTMLCanvasElement::setImageBufferAndMarkDirty): New function to set the backing store efficiently, as well as make it look like we need to repaint the entire canvas. (WebCore::HTMLCanvasElement::drawingContext const): The buffer doesn't actually need to provide a GraphicsContext if we're a bitmap renderer. * html/HTMLCanvasElement.h: * html/ImageBitmap.cpp: (WebCore::ImageBitmap::transferOwnershipAndClose): New helper to give the ownership of the ImageBuffer away, and look like close() was called. * html/ImageBitmap.h: * html/canvas/ImageBitmapRenderingContext.cpp: Implement the algorithm from the HTML specification. (WebCore::ImageBitmapRenderingContext::ImageBitmapRenderingContext): (WebCore::ImageBitmapRenderingContext::isAccelerated const): (WebCore::ImageBitmapRenderingContext::setOutputBitmap): (WebCore::ImageBitmapRenderingContext::transferFromImageBitmap): * html/canvas/ImageBitmapRenderingContext.h: * html/canvas/ImageBitmapRenderingContext.idl: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@224195 268f45cc-cd09-0410-ab3c-d52691b4dbfc
|
Just transfer() is not clear IMHO. It reads like the intent is to transfer the the object it was called on (the rendering context). I think we should at least keep the "From". |
|
Yeah, I forgot to comment on this again, but I realised transferFrom() is the better name. |
|
Does everyone else in @whatwg/canvas agree with this change? Anyone willing to make it? |
|
The reason it's called transferFromImageBitmap is to be symmetric with OffscreenCanvas's transferToImageBitmap. I lean toward leaving it as is but don't object to changing it. |
|
Also Gecko and Blink have already shipped with the current naming. It is possible to backtrack (alias+deprecate), but it's not ideal. |
|
In that case I suggest we leave it as-is. If in the future we support another type we can have a more generic overloaded method. Does that seem reasonable @grorg? |
ryanhaddad
pushed a commit
to WebKit/WebKit
that referenced
this issue
Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=178992 <rdar://problem/34147157> Reviewed by Sam Weinig. LayoutTests/imported/w3c: Update the expected results now more of the interface has been added. * web-platform-tests/imagebitmap-renderingcontext/bitmaprenderer-as-imagesource-expected.txt: * web-platform-tests/imagebitmap-renderingcontext/context-creation-with-alpha-expected.txt: * web-platform-tests/imagebitmap-renderingcontext/tranferFromImageBitmap-null-expected.txt: * web-platform-tests/imagebitmap-renderingcontext/transferFromImageBitmap-detached-expected.txt: Source/WebCore: Implement the "bitmaprenderer" context for HTMLCanvasElement. Use as much of the existing 2d context code as possible, which means that we are painting the ImageBitmap into the backing store. This isn't optimal, but it is good enough to get going while we move buffer ownership from the canvas object into the rendering contexts. This patch raised a few issues on the HTML specification: whatwg/html#3164 whatwg/html#3165 whatwg/html#3166 whatwg/html#3167 Update existing Web Platform Test results. * html/HTMLCanvasElement.cpp: (WebCore::HTMLCanvasElement::createContext2d): Drive-by comment fix. (WebCore::HTMLCanvasElement::createContextWebGL): Ditto. (WebCore::HTMLCanvasElement::createContextWebGPU): Ditto. (WebCore::HTMLCanvasElement::createContextBitmapRenderer): We now require a layer/backing store for a bitmap context. (WebCore::HTMLCanvasElement::paintsIntoCanvasBuffer const): Add bitmap-renderer to the type of context that uses the canvas's backing store. (WebCore::HTMLCanvasElement::createImageBuffer const): Comment fix. (WebCore::HTMLCanvasElement::setImageBuffer const): Change parameter to a &&. (WebCore::HTMLCanvasElement::setImageBufferAndMarkDirty): New function to set the backing store efficiently, as well as make it look like we need to repaint the entire canvas. (WebCore::HTMLCanvasElement::drawingContext const): The buffer doesn't actually need to provide a GraphicsContext if we're a bitmap renderer. * html/HTMLCanvasElement.h: * html/ImageBitmap.cpp: (WebCore::ImageBitmap::transferOwnershipAndClose): New helper to give the ownership of the ImageBuffer away, and look like close() was called. * html/ImageBitmap.h: * html/canvas/ImageBitmapRenderingContext.cpp: Implement the algorithm from the HTML specification. (WebCore::ImageBitmapRenderingContext::ImageBitmapRenderingContext): (WebCore::ImageBitmapRenderingContext::isAccelerated const): (WebCore::ImageBitmapRenderingContext::setOutputBitmap): (WebCore::ImageBitmapRenderingContext::transferFromImageBitmap): * html/canvas/ImageBitmapRenderingContext.h: * html/canvas/ImageBitmapRenderingContext.idl: Canonical link: https://commits.webkit.org/195159@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@224195 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
transferFromImageBitmapis a redundant name. It should just betransfer.Also, the only parameter currently allowed is an
ImageBitmap. Renaming it totransferwould mean that it could theoretically take other objects in the future (as unlikely as that may be).The text was updated successfully, but these errors were encountered: