Bug 237460

Created attachment 453808 [details] Patch

Comment on attachment 453808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453808&action=review > Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:393 > +#define BACKING(backing) backing FWIW, to me it would appear that the USE(COORDINATED_GRAPHICS) and !USE(COORDINATED_GRAPHICS) do not share that much in common. Maybe it would make senes to just have two different functions or at least two different function contents, and just write the code linearly. The BACKING macro and invoking it with sometimes undefined variable name looks very confusing.

Comment on attachment 453808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453808&action=review >> Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:393 >> +#define BACKING(backing) backing > > FWIW, to me it would appear that the USE(COORDINATED_GRAPHICS) and !USE(COORDINATED_GRAPHICS) do not share that much in common. > Maybe it would make senes to just have two different functions or at least two different function contents, and just write the code linearly. > The BACKING macro and invoking it with sometimes undefined variable name looks very confusing. Thank you for the comment. But, I don't think they are doing different things. !USE(COORDINATED_GRAPHICS) resizes two textures while USE(COORDINATED_GRAPHICS) resizes three textures or sets backings. And, they are sharing other code, for example initializing and finalizing m_compositorTexture.

Comment on attachment 453808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453808&action=review > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:554 > -#if USE(COORDINATED_GRAPHICS) > +#if !PLATFORM(COCOA) What if you use `USE(TEXTURE_MAPPER) || USE(COORDINATED_GRAPHICS)` along with guarding the extra swaps, like you've done below? >>> Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:393 >>> +#define BACKING(backing) backing >> >> FWIW, to me it would appear that the USE(COORDINATED_GRAPHICS) and !USE(COORDINATED_GRAPHICS) do not share that much in common. >> Maybe it would make senes to just have two different functions or at least two different function contents, and just write the code linearly. >> The BACKING macro and invoking it with sometimes undefined variable name looks very confusing. > > Thank you for the comment. > But, I don't think they are doing different things. > !USE(COORDINATED_GRAPHICS) resizes two textures while USE(COORDINATED_GRAPHICS) resizes three textures or sets backings. > And, they are sharing other code, for example initializing and finalizing m_compositorTexture. It's hard to grasp what's going on here, and it's going to change soon for USE(COORDINATED_GRAPHICS) anyway. Better duplicate the code for now in something like the following block: ``` #if USE(COORDINATED_GRAPHICS) ... #elif USE(TEXTURE_MAPPER) ... #endif ```

Comment on attachment 453808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453808&action=review >> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:554 >> +#if !PLATFORM(COCOA) > > What if you use `USE(TEXTURE_MAPPER) || USE(COORDINATED_GRAPHICS)` along with guarding the extra swaps, like you've done below? Got it. I will revise. But, if I change here, I have to change the macro for m_compositorTexture definition in GraphicsContextGLANGLE.h for the consistency. >>>> Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:393 >>>> +#define BACKING(backing) backing >>> >>> FWIW, to me it would appear that the USE(COORDINATED_GRAPHICS) and !USE(COORDINATED_GRAPHICS) do not share that much in common. >>> Maybe it would make senes to just have two different functions or at least two different function contents, and just write the code linearly. >>> The BACKING macro and invoking it with sometimes undefined variable name looks very confusing. >> >> Thank you for the comment. >> But, I don't think they are doing different things. >> !USE(COORDINATED_GRAPHICS) resizes two textures while USE(COORDINATED_GRAPHICS) resizes three textures or sets backings. >> And, they are sharing other code, for example initializing and finalizing m_compositorTexture. > > It's hard to grasp what's going on here, and it's going to change soon for USE(COORDINATED_GRAPHICS) anyway. > > Better duplicate the code for now in something like the following block: > ``` > #if USE(COORDINATED_GRAPHICS) > ... > #elif USE(TEXTURE_MAPPER) > ... > #endif > ``` Got it. I will revise with duplicating approach. But, I thiknk USE(TEXTURE_MAPPER)) isn't needed here.

Created attachment 454020 [details] Patch

Comment on attachment 454020 [details] Patch Awesome, thanks.

Comment on attachment 454020 [details] Patch Thank you for the review.

Committed r290980 (248158@main): <https://commits.webkit.org/248158@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 454020 [details].

<rdar://problem/89954884>