Bug 143298 – [TexMap] Clean up BitmapTexture and BitmapTextureGL.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gwang Yoon Hwang]

[TexMap] Clean up BitmapTexture and BitmapTextureGL.

[Gwang Yoon Hwang]

Created attachment 249905 [details]
Patch

[Zan Dobersek]

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?

[Gwang Yoon Hwang]

Created attachment 249919 [details]
Patch

[Gwang Yoon Hwang]

(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.

[Zan Dobersek]

Comment on attachment 249919 [details]
Patch

Looks good.

[Martin Robinson]

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.

[Gwang Yoon Hwang]

Created attachment 263995 [details]
Patch

[Gwang Yoon Hwang]

(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.

[WebKit Commit Bot]

Comment on attachment 263995 [details]
Patch

Clearing flags on attachment: 263995

Committed r191541: <http://trac.webkit.org/changeset/191541>