Bug 174452 – Clean-up some things in GraphicsContext3D

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

[Dean Jackson]

Clean-up some things in GraphicsContext3D

[Radar WebKit Bug Importer]

<rdar://problem/33281257>

[Dean Jackson]

Created attachment 315326 [details]
Patch

[Dean Jackson]

Requires 174345 to apply cleanly.

[Simon Fraser (smfr)]

Comment on attachment 315326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315326&action=review

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:-576
> -#if USE(COORDINATED_GRAPHICS_THREADED)
> -        ::glDeleteTextures(1, &m_compositorTexture);
> -#endif

Why can this go away?

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:315
> +    ASSERT(width >= 0 && height >= 0);

Can this assert fire if JS passes bad values from script? If so, don't assert.

[Dean Jackson]

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 315326 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315326&action=review
> 
> > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:-576
> > -#if USE(COORDINATED_GRAPHICS_THREADED)
> > -        ::glDeleteTextures(1, &m_compositorTexture);
> > -#endif
> 
> Why can this go away?

Mentioned in the ChangeLog :)

We don't use COORDINATED_GRAPHICS_THREADED on Apple platforms.

> 
> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:315
> > +    ASSERT(width >= 0 && height >= 0);
> 
> Can this assert fire if JS passes bad values from script? If so, don't
> assert.

I don't really understand why these were int to begin with. You can't have a canvas with negative dimensions. I expect that we catch it in the bindings.

[Dean Jackson]

Committed r219473: <http://trac.webkit.org/changeset/219473>

[Darin Adler]

Comment on attachment 315326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315326&action=review

>>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:315
>>> +    ASSERT(width >= 0 && height >= 0);
>> 
>> Can this assert fire if JS passes bad values from script? If so, don't assert.
> 
> I don't really understand why these were int to begin with. You can't have a canvas with negative dimensions. I expect that we catch it in the bindings.

General WebKit style note: We try not to write a single assertion with &&. Instead we write two assertions on separate lines, so we can tell which one failed.

[Dean Jackson]

Comment on attachment 315326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315326&action=review

>>>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:315
>>>> +    ASSERT(width >= 0 && height >= 0);
>>> 
>>> Can this assert fire if JS passes bad values from script? If so, don't assert.
>> 
>> I don't really understand why these were int to begin with. You can't have a canvas with negative dimensions. I expect that we catch it in the bindings.
> 
> General WebKit style note: We try not to write a single assertion with &&. Instead we write two assertions on separate lines, so we can tell which one failed.

Thanks Darin.