Summary: | Allow different back-ends for ImageBuffer | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||||||||
Component: | Canvas | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | alecflett, annulen, beidson, cdumez, commit-queue, dino, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, graouts, gyuyoung.kim, hta, jer.noble, jsbell, kondapallykalyan, noam, pdr, philipj, ryuan.choi, schenney, sergio, thorton, tommyw, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=217384 https://bugs.webkit.org/show_bug.cgi?id=217603 |
||||||||||||||||||||||
Bug Depends on: | 206621 | ||||||||||||||||||||||
Bug Blocks: | 204955, 207109 | ||||||||||||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2020-01-31 08:59:48 PST
Created attachment 389356 [details]
Patch
Created attachment 389357 [details]
Patch
Created attachment 389366 [details]
Patch
Created attachment 389414 [details]
Patch
Created attachment 389449 [details]
Patch for review
Comment on attachment 389449 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=389449&action=review I think this is fine, except the name. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:834 > - 2DDE1D081F574D0E00D1A365 /* JSVRStageParameters.h in Headers */ = {isa = PBXBuildFile; fileRef = 2DDE1CF61F574BFF00D1A365 /* JSVRStageParameters.h */; }; > + 2DDE1CE31F574AE500D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; }; This all doesn't look good > Source/WebCore/platform/graphics/ContextualImageBuffer.h:33 > +class ContextualImageBuffer : public ImageBuffer { I'm not sure I follow the "Contextual" name (but maybe you can explain) (also, I'm not sure why it exists? Why isn't this just all IN ImageBuffer?) (reading more... is it so you can hide BackendType from ImageBuffer clients? why not just `using` it away?) > Source/WebCore/platform/graphics/ContextualImageBuffer.h:145 > + const_cast<ContextualImageBuffer&>(*this).flushContext(); Maybe we should just bite the bullet and de-const getImageData if it's a lie? Created attachment 391061 [details]
Patch
Created attachment 391063 [details]
Patch
Comment on attachment 389449 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=389449&action=review >> Source/WebCore/WebCore.xcodeproj/project.pbxproj:834 >> + 2DDE1CE31F574AE500D1A365 /* (null) in Headers */ = {isa = PBXBuildFile; }; > > This all doesn't look good This was fixed in the latest patch. >> Source/WebCore/platform/graphics/ContextualImageBuffer.h:33 >> +class ContextualImageBuffer : public ImageBuffer { > > I'm not sure I follow the "Contextual" name (but maybe you can explain) > > (also, I'm not sure why it exists? Why isn't this just all IN ImageBuffer?) > > (reading more... is it so you can hide BackendType from ImageBuffer clients? why not just `using` it away?) Contextual here means an ImageBuffer which can provide a GraphicsContext. ContextualImageBuffer is a template class which is derived from ImageBuffer. All the callers can still keep a. pointer to an ImageBuffer which represents the interface of all the ContextualImageBuffers. It is also a bridge design pattern. As you said it hides the backend type from the callers. The creator can choose the backend type but caller which receives an std::unique_ptr<ImageBuffer> does not have to know any details about this backend. Other names: BackendedImageBuffer, ImageBufferWithBackend, PhysicalImageBuffer, ConcreteImageBuffer, ImageBufferImpl, ImageBufferBridge.. >> Source/WebCore/platform/graphics/ContextualImageBuffer.h:145 >> + const_cast<ContextualImageBuffer&>(*this).flushContext(); > > Maybe we should just bite the bullet and de-const getImageData if it's a lie? I think it is okay to keep the function const. flushContext() is an internal function which does nothing expect in the case of IOSurface backed ended ImageBuffer and the remote ImageBuffer. Created attachment 391110 [details]
Patch
Created attachment 391114 [details]
Patch
Comment on attachment 391114 [details] Patch Clearing flags on attachment: 391114 Committed r256892: <https://trac.webkit.org/changeset/256892> All reviewed patches have been landed. Closing bug. |