Bug 174452 - Clean-up some things in GraphicsContext3D
Summary: Clean-up some things in GraphicsContext3D
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-12 19:18 PDT by Dean Jackson
Modified: 2017-07-25 13:41 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.11 KB, patch)
2017-07-12 19:25 PDT, Dean Jackson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2017-07-12 19:18:15 PDT
Clean-up some things in GraphicsContext3D
Comment 1 Radar WebKit Bug Importer 2017-07-12 19:18:51 PDT
<rdar://problem/33281257>
Comment 2 Dean Jackson 2017-07-12 19:25:02 PDT
Created attachment 315326 [details]
Patch
Comment 3 Dean Jackson 2017-07-12 19:36:26 PDT
Requires 174345 to apply cleanly.
Comment 4 Simon Fraser (smfr) 2017-07-13 14:15:12 PDT
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.
Comment 5 Dean Jackson 2017-07-13 15:40:36 PDT
(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.
Comment 6 Dean Jackson 2017-07-13 16:00:18 PDT
Committed r219473: <http://trac.webkit.org/changeset/219473>
Comment 7 Darin Adler 2017-07-25 09:28:57 PDT
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.
Comment 8 Dean Jackson 2017-07-25 13:41:16 PDT
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.