Bug 164749 – [WebGL] Migrate construction functions from pointers to references

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

[Myles C. Maxfield]

[WebGL] Migrate construction functions from pointers to references

[Myles C. Maxfield]

Created attachment 294776 [details]
Patch

[WebKit Commit Bot]

Attachment 294776 [details] did not pass style-queue:


ERROR: Source/WebCore/html/canvas/WebGLSharedObject.cpp:39:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 81 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Created attachment 294814 [details]
Patch

[Myles C. Maxfield]

Created attachment 294816 [details]
Patch

[Myles C. Maxfield]

Created attachment 294819 [details]
Patch

[Myles C. Maxfield]

Created attachment 294844 [details]
Patch

[Dean Jackson]

Comment on attachment 294844 [details]
Patch

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

> Source/WebCore/html/canvas/ANGLEInstancedArrays.cpp:57
> +    Extensions3D& extensions = context.graphicsContext3D()->getExtensions();

Use auto.

Actually, how about removing the local variable altogether?
return context.graphicsContext3D()->getExtensions().supports(...

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:961
> +    if (WebGLDepthTexture::supported(*graphicsContext3D()))

I wonder if graphicsContext3D() can return a ref? Is it ever null? I don't think so.

> Source/WebCore/html/canvas/WebGLCompressedTextureATC.cpp:55
> +    Extensions3D& extensions = context.graphicsContext3D()->getExtensions();

auto or remove local var

> Source/WebCore/html/canvas/WebGLCompressedTexturePVRTC.cpp:57
> +    Extensions3D& extensions = context.graphicsContext3D()->getExtensions();

same here

> Source/WebCore/html/canvas/WebGLCompressedTextureS3TC.cpp:57
> +    Extensions3D& extensions = context.graphicsContext3D()->getExtensions();

auto

> Source/WebCore/html/canvas/WebGLDepthTexture.cpp:52
> +    Extensions3D& extensions = context.getExtensions();

auto

> Source/WebCore/html/canvas/WebGLDrawBuffers.cpp:52
> +    Extensions3D& extensions = context.graphicsContext3D()->getExtensions();
> +    return extensions.supports("GL_EXT_draw_buffers")

auto or local var

> Source/WebCore/html/canvas/WebGLDrawBuffers.cpp:109
> +    bool supportsDepth = (context->getExtensions().supports("GL_CHROMIUM_depth_texture")
> +        || context->getExtensions().supports("GL_OES_depth_texture")
> +        || context->getExtensions().supports("GL_ARB_depth_texture"));

We can remove the CHROMIUM one now.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:215
> +        && WebGLDepthTexture::supported(*graphicsContext3D())) {
>          if (!m_webglDepthTexture) {
> -            m_context->getExtensions()->ensureEnabled("GL_CHROMIUM_depth_texture");
> -            m_webglDepthTexture = std::make_unique<WebGLDepthTexture>(this);
> +            m_context->getExtensions().ensureEnabled("GL_CHROMIUM_depth_texture");
> +            m_webglDepthTexture = std::make_unique<WebGLDepthTexture>(*this);

I wonder if we need this at all.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:579
> +    m_isErrorGeneratedOnOutOfBoundsAccesses = m_context->getExtensions().isEnabled("GL_CHROMIUM_strict_attribs");
> +    m_isResourceSafe = m_context->getExtensions().isEnabled("GL_CHROMIUM_resource_safe");

Hmm... these must have standard names now. Or we don't nee them.

> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.cpp:45
> +    Extensions3D& extensions = context()->graphicsContext3D()->getExtensions();

auto

[Myles C. Maxfield]

Created attachment 294849 [details]
Patch for committing

[Myles C. Maxfield]

Comment on attachment 294844 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:961
>> +    if (WebGLDepthTexture::supported(*graphicsContext3D()))
> 
> I wonder if graphicsContext3D() can return a ref? Is it ever null? I don't think so.

Unfortunately, WebGLRenderingContextBase::destroyGraphicsContext3D() sets it to nullptr.

>> Source/WebCore/html/canvas/WebGLDrawBuffers.cpp:109
>> +        || context->getExtensions().supports("GL_ARB_depth_texture"));
> 
> We can remove the CHROMIUM one now.

I'll do this in a follow-up patch.

[Myles C. Maxfield]

Comment on attachment 294844 [details]
Patch

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

>>> Source/WebCore/html/canvas/WebGLDrawBuffers.cpp:109
>>> +        || context->getExtensions().supports("GL_ARB_depth_texture"));
>> 
>> We can remove the CHROMIUM one now.
> 
> I'll do this in a follow-up patch.

https://bugs.webkit.org/show_bug.cgi?id=164782

[Myles C. Maxfield]

Committed r208740: <http://trac.webkit.org/changeset/208740>

[Csaba Osztrogonác]

(In reply to comment #11)
> Committed r208740: <http://trac.webkit.org/changeset/208740>

It broke the WinCairo build, see build.webkit.org for details.

[Csaba Osztrogonác]

(In reply to comment #12)
> (In reply to comment #11)
> > Committed r208740: <http://trac.webkit.org/changeset/208740>
> 
> It broke the WinCairo build, see build.webkit.org for details.

https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/62839

[Alex Christensen]

http://trac.webkit.org/changeset/208873