Summary: | Cocoa: Make WebGLLayer not dependent on GraphicsContextGLOpenGL | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kimmo Kinnunen <kkinnunen> | ||||||||
Component: | WebGL | Assignee: | Kimmo Kinnunen <kkinnunen> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dino, ews-watchlist, graouts, kbr, koivisto, kondapallykalyan, msatbentley, noam, reon90, sam, webkit-bug-importer | ||||||||
Priority: | P1 | Keywords: | InRadar | ||||||||
Version: | Safari Technology Preview | ||||||||||
Hardware: | Mac | ||||||||||
OS: | macOS 10.15 | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=229606 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 217211, 217581, 217213, 217697, 218100, 218177 | ||||||||||
Attachments: |
|
Description
Kimmo Kinnunen
2020-10-02 04:16:34 PDT
Created attachment 411113 [details]
Patch
Comment on attachment 411113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411113&action=review > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:91 > +#if PLATFORM(COCOA) If this is really not universal, please use a more specific define. Please try to keep PLATFORM usage to a minimum. Comment on attachment 411113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411113&action=review >> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:91 >> +#if PLATFORM(COCOA) > > If this is really not universal, please use a more specific define. Please try to keep PLATFORM usage to a minimum. It's universal to COCOA. This patch is about implementation of Cocoa-specific GraphicsContextGLOpenGL. Unfortunately the setup is what it is before this patch, and this patch does not yet address the fundamental problem with the ifdefs. This patch does work towards eventually being able to remove the ifdefs. Created attachment 411127 [details]
Patch
Comment on attachment 411127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411127&action=review > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93 > +#if PLATFORM(COCOA) > + , private WebGLLayerClient > +#endif Looks like you kept this PLATFORM(COCOA). Please you use a more specific #define rather here. Comment on attachment 411127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411127&action=review >> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93 >> +#endif > > Looks like you kept this PLATFORM(COCOA). Please you use a more specific #define rather here. Would you be able to suggest a define that would be more appropriate and still work with other code defined inside PLATFORM(COCOA) ifdefs and be implemented in the file GraphicsContextGLOpenGLCocoa.mm ? (In reply to Kimmo Kinnunen from comment #7) > Comment on attachment 411127 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=411127&action=review > > >> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93 > >> +#endif > > > > Looks like you kept this PLATFORM(COCOA). Please you use a more specific #define rather here. > > Would you be able to suggest a define that would be more appropriate and > still work with other code defined inside PLATFORM(COCOA) ifdefs and be > implemented in the file GraphicsContextGLOpenGLCocoa.mm ? Sure. Maybe USE(CA) in this case? One day maybe we will have a better PlatformLayer abstraction than what we have right now :(. Comment on attachment 411127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411127&action=review >>>> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93 >>>> +#endif >>> >>> Looks like you kept this PLATFORM(COCOA). Please you use a more specific #define rather here. >> >> Would you be able to suggest a define that would be more appropriate and still work with other code defined inside PLATFORM(COCOA) ifdefs and be implemented in the file GraphicsContextGLOpenGLCocoa.mm ? > > Sure. Maybe USE(CA) in this case? One day maybe we will have a better PlatformLayer abstraction than what we have right now :(. But do I rename the file GraphicsContextGLOpenGLCocoa.mm to GraphicsContextGLOpenGLCA.mm ? Do I rename the existing ifdef PLATFORM(COCOA) to PLATFORM(CA) ? None of that existing code will make sense if part of it is CA and part of it is COCOA ? Comment on attachment 411127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411127&action=review >>>>> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93 >>>>> +#endif >>>> >>>> Looks like you kept this PLATFORM(COCOA). Please you use a more specific #define rather here. >>> >>> Would you be able to suggest a define that would be more appropriate and still work with other code defined inside PLATFORM(COCOA) ifdefs and be implemented in the file GraphicsContextGLOpenGLCocoa.mm ? >> >> Sure. Maybe USE(CA) in this case? One day maybe we will have a better PlatformLayer abstraction than what we have right now :(. > > But do I rename the file GraphicsContextGLOpenGLCocoa.mm to GraphicsContextGLOpenGLCA.mm ? Do I rename the existing ifdef PLATFORM(COCOA) to PLATFORM(CA) ? None of that existing code will make sense if part of it is CA and part of it is COCOA ? Up to you to rename existing things. This stuff is already a mess of different conflicting defines. If you really feel strongly about keeping this COCOA, I guess that's ok too. Comment on attachment 411127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411127&action=review >>>>>> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93 >>>>>> +#endif >>>>> >>>>> Looks like you kept this PLATFORM(COCOA). Please you use a more specific #define rather here. >>>> >>>> Would you be able to suggest a define that would be more appropriate and still work with other code defined inside PLATFORM(COCOA) ifdefs and be implemented in the file GraphicsContextGLOpenGLCocoa.mm ? >>> >>> Sure. Maybe USE(CA) in this case? One day maybe we will have a better PlatformLayer abstraction than what we have right now :(. >> >> But do I rename the file GraphicsContextGLOpenGLCocoa.mm to GraphicsContextGLOpenGLCA.mm ? Do I rename the existing ifdef PLATFORM(COCOA) to PLATFORM(CA) ? None of that existing code will make sense if part of it is CA and part of it is COCOA ? > > Up to you to rename existing things. This stuff is already a mess of different conflicting defines. If you really feel strongly about keeping this COCOA, I guess that's ok too. Thanks. I'll keep the COCOA in this patch. Renaming COCOA->CA should be done in a different patch, targeting specifically that goal. Comment on attachment 411127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411127&action=review > Source/WebCore/ChangeLog:18 > + Move the back buffer ownership to the GraphicsContextGLOpenGL. Nice. > Source/WebCore/ChangeLog:21 > + Make the front buffer ownership explicit in WebGLLayer. > + Move the EGL bindings ownerships of all buffers to > + GraphicsContextGLOpenGL. Nicer! > Source/WebCore/platform/graphics/cocoa/WebGLLayer.h:34 > + void* handle { nullptr }; // Client specific metadata handle (such as EGLSurface). Will a WebGLLayer that is getting remote buffers need a handle? If not, it might be ok if this is specific to EGL. > Source/WebCore/platform/graphics/cocoa/WebGLLayer.h:63 > +- (WebCore::WebGLLayerBuffer) recycleBuffer; Nit: We don't put a space between the return type and the name. >>>>>>> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93 >>>>>>> +#endif >>>>>> >>>>>> Looks like you kept this PLATFORM(COCOA). Please you use a more specific #define rather here. >>>>> >>>>> Would you be able to suggest a define that would be more appropriate and still work with other code defined inside PLATFORM(COCOA) ifdefs and be implemented in the file GraphicsContextGLOpenGLCocoa.mm ? >>>> >>>> Sure. Maybe USE(CA) in this case? One day maybe we will have a better PlatformLayer abstraction than what we have right now :(. >>> >>> But do I rename the file GraphicsContextGLOpenGLCocoa.mm to GraphicsContextGLOpenGLCA.mm ? Do I rename the existing ifdef PLATFORM(COCOA) to PLATFORM(CA) ? None of that existing code will make sense if part of it is CA and part of it is COCOA ? >> >> Up to you to rename existing things. This stuff is already a mess of different conflicting defines. If you really feel strongly about keeping this COCOA, I guess that's ok too. > > Thanks. I'll keep the COCOA in this patch. Renaming COCOA->CA should be done in a different patch, targeting specifically that goal. I think it is ok to leave the files named Cocoa. I wonder if we could make WebGLLayerClient an unconditional inheritance though, and have an empty implementation for other ports. Created attachment 411198 [details]
Patch
> I wonder if we could make WebGLLayerClient an unconditional inheritance though, and have an empty implementation for other ports.
I don't think the ifdef is a special. There are multiple as severe existing ifdefs, they don't just stand out because mental adaptation to having ifdefs for member functions but not for base classes.
Adding empty WebGLLayerClient for other ports makes little sense, since it only rises questions of "why do we have WebGLLayerClient when we don't even have a thing called WebGLLayer nor we make this object a client of anything".
The solution to the ifdef problem is to work on the inheritance tree so that the cocoa specific code can have normal final cocoa-specific class where the client inheritance is business as usual. This is a preliminary work to that.
kkinnunen@apple.com does not have committer permissions according to https://svn.webkit.org/repository/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. Rejecting attachment 411198 [details] from commit queue. kkinnunen@apple.com does not have reviewer permissions according to https://svn.webkit.org/repository/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. Rejecting attachment 411198 [details] from commit queue. Committed r268386: <https://trac.webkit.org/changeset/268386> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411198 [details]. *** Bug 218100 has been marked as a duplicate of this bug. *** Great fix for this Kimmo! This fixes real resource leaks that have been identified in Bug 218100 and Bug 217581. Could this please be merged back to the Safari 14 release branch? *** Bug 218100 has been marked as a duplicate of this bug. *** *** Bug 219780 has been marked as a duplicate of this bug. *** Could you answer what version of Safari contains such fix? |