Bug 143298

Created attachment 249905 [details] Patch

Comment on attachment 249905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249905&action=review This looks almost perfect. Left a few comments though. While the patch looks harmless, I wouldn't land it before the EFL port fixes the last regression. > Source/WebCore/platform/graphics/texmap/BitmapTexture.h:70 > + virtual bool canReuseWith(const IntSize& /* contentsSize */, bool /* hasAlpha */ = 0) { return false; } Can we drop the hasAlpha parameter? It's unused in the current code. > Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:65 > +static bool driverSupportsExternalTextureBGRA(GraphicsContext3D* context) This can just take in a reference to the GraphicsContext3D object. > Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:91 > + if (driverSupportsExternalTextureBGRA(m_context3D.get())) Can we afford caching the return value of driverSupportsExternalTextureBGRA() in a member variable?

Created attachment 249919 [details] Patch

(In reply to comment #2) > Comment on attachment 249905 [details] > Patch > > Source/WebCore/platform/graphics/texmap/BitmapTexture.h:70 > > + virtual bool canReuseWith(const IntSize& /* contentsSize */, bool /* hasAlpha */ = 0) { return false; } > > Can we drop the hasAlpha parameter? It's unused in the current code. > I just removed canReuseWith, this method is not needed anymore. > > Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:65 > > +static bool driverSupportsExternalTextureBGRA(GraphicsContext3D* context) > > This can just take in a reference to the GraphicsContext3D object. > > > Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:91 > > + if (driverSupportsExternalTextureBGRA(m_context3D.get())) > > Can we afford caching the return value of > driverSupportsExternalTextureBGRA() in a member variable? Good catch. we can test it with a m_format whether swizzling is needed or not.

Comment on attachment 249919 [details] Patch Looks good.

Comment on attachment 249919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249919&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperTile.cpp:66 > + m_texture->reset(targetRect.size(), true); Here I think you should either keep the enum or put in a comment, because of this style rule: "Prefer enums to bools on function parameters if callers are likely to be passing constants, since named constants are easier to read at the call site. An exception to this rule is a setter function, where the name of the function already makes clear what the boolean is." The new version is a little harder to understand.

Created attachment 263995 [details] Patch

(In reply to comment #6) > Comment on attachment 249919 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249919&action=review > > > Source/WebCore/platform/graphics/texmap/TextureMapperTile.cpp:66 > > + m_texture->reset(targetRect.size(), true); > > Here I think you should either keep the enum or put in a comment, because of > this style rule: > > "Prefer enums to bools on function parameters if callers are likely to be > passing constants, since named constants are easier to read at the call > site. An exception to this rule is a setter function, where the name of the > function already makes clear what the boolean is." > > The new version is a little harder to understand. Ah, I thought this patch was already landed. :/ Anyway, I agree with your review, thank you so much for quoting the guideline. I learned one more thing today. I reverted changes related with Flags.

Comment on attachment 263995 [details] Patch Clearing flags on attachment: 263995 Committed r191541: <http://trac.webkit.org/changeset/191541>