Bug 213906

Created attachment 403413 [details]
Patch

Created attachment 403419 [details]
rebaseline layout tests and fix non-mac build

Comment on attachment 403419 [details]
rebaseline layout tests and fix non-mac build

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

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1836
> +    WebGLTransformFeedback* toBeBound = feedbackObject ? feedbackObject : m_defaultTransformFeedback.get();

auto

> Source/WebCore/html/canvas/WebGL2RenderingContext.h:249
> +    GCGLuint getMaxTransformFeedbackSeparateAttribs();

const?

> Source/WebCore/html/canvas/WebGLProgram.h:77
> +    int getRequiredTransformFeedbackBufferCount()

const

> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:93
> +    for (auto boundBuffer : m_boundIndexedTransformFeedbackBuffers) {

This will do unnecessary reference count churn. Should just use auto&.

But also, Vector has a find function that I think we can use here.

> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:105
> +    for (unsigned i = 0; i < m_boundIndexedTransformFeedbackBuffers.size(); i++) {
> +        if (m_boundIndexedTransformFeedbackBuffers[i] == buffer)
> +            m_boundIndexedTransformFeedbackBuffers[i] = nullptr;
> +    }

range-based for loop would be better:

    for (auto& boundBuffer : m_boundIndexedTransformFeedbackBuffers) {
        if (boundBuffer == &buffer)
            boundBuffer = nullptr;
    }

> Source/WebCore/html/canvas/WebGLTransformFeedback.h:58
> +    WebGLProgram* getProgram() const { return m_program.get(); }

WebKit coding style says we don’t name these functions "get".

> Source/WebCore/html/canvas/WebGLTransformFeedback.h:59
> +    void setProgram(WebGLProgram*);

Can this take a reference instead of a pointer?

> Source/WebCore/html/canvas/WebGLTransformFeedback.h:62
> +    bool usesBuffer(WebGLBuffer*);
> +    void unbindBuffer(WebGLBuffer*);

Can these take a reference instead of a pointer?

Comment on attachment 403419 [details]
rebaseline layout tests and fix non-mac build

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

>> Source/WebCore/html/canvas/WebGLProgram.h:77
>> +    int getRequiredTransformFeedbackBufferCount()
> 
> const

This function isn't const, since cacheInfoIfNeeded() is not, but Darin's comment about not calling getters "get" applies here

> Source/WebCore/html/canvas/WebGLTransformFeedback.h:70
> +    bool m_active = false;

Style nit: class member initializer defaults should use { } notation.

Comment on attachment 403419 [details]
rebaseline layout tests and fix non-mac build

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

Great work James figuring out all of the dependent fixes needed!

LGTM modulo Darin's and Justin's other feedback.

To fix the build failure on gtk, it might be necessary to change WebGLTransformFeedback.idl to use:
  Conditional=WEBGL2,
  EnabledAtRuntime=WebGL2,
but not 100% sure. Some of the other WebGL 2.0-specific types (WebGLSampler, for example) only use "#if ENABLE(WEBGL)" guards in their headers, which seems wrong.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1788
> +    if (isContextLostOrPending() || !feedbackObject || feedbackObject->isDeleted() || !validateWebGLObject("deleteTransformFeedback", feedbackObject))

The isDeleted() check differs slightly from Chromium's validation - this version looks more correct. Is there a way to test the difference, and if so could you upstream a WebGL conformance test covering it?

Created attachment 403605 [details]
address review feedback and fix non-webgl2 builds

Comment on attachment 403419 [details]
rebaseline layout tests and fix non-mac build

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

>> Source/WebCore/html/canvas/WebGL2RenderingContext.h:249
>> +    GCGLuint getMaxTransformFeedbackSeparateAttribs();
> 
> const?

And name should be maxTransformFeedbackSeparateAttribs()

>> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:93
>> +    for (auto boundBuffer : m_boundIndexedTransformFeedbackBuffers) {
> 
> This will do unnecessary reference count churn. Should just use auto&.
> 
> But also, Vector has a find function that I think we can use here.

I think you can just call m_boundIndexedTransformFeedbackBuffers.contains(buffer)

Comment on attachment 403605 [details]
address review feedback and fix non-webgl2 builds

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

> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:64
> +bool WebGLTransformFeedback::setBoundIndexedTransformFeedbackBuffer(GCGLuint index, WebGLBuffer* buffer)

You don't check the return value of this. Should it return void?

> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:78
> +bool WebGLTransformFeedback::getBoundIndexedTransformFeedbackBuffer(GCGLuint index, WebGLBuffer** outBuffer)
> +{
> +    if (index > m_boundIndexedTransformFeedbackBuffers.size())
> +        return false;
> +    *outBuffer = m_boundIndexedTransformFeedbackBuffers[index].get();
> +    return true;
> +}

Not necessary right now, but this could return an Optional instead.

> Source/WebCore/html/canvas/WebGLTransformFeedback.h:51
> +    bool getBoundIndexedTransformFeedbackBuffer(GCGLuint index, WebGLBuffer** outBuffer);

Nit: Don't use "get"

Created attachment 403622 [details]
more review feedback

Comment on attachment 403419 [details]
rebaseline layout tests and fix non-mac build

Thanks for all the feedback! It should all be addressed now.

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

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1788
>> +    if (isContextLostOrPending() || !feedbackObject || feedbackObject->isDeleted() || !validateWebGLObject("deleteTransformFeedback", feedbackObject))
> 
> The isDeleted() check differs slightly from Chromium's validation - this version looks more correct. Is there a way to test the difference, and if so could you upstream a WebGL conformance test covering it?

These checks happen in the DeleteObject function in Chromium's implementation, but I don't think the behavior is different.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1836
>> +    WebGLTransformFeedback* toBeBound = feedbackObject ? feedbackObject : m_defaultTransformFeedback.get();
> 
> auto

Done.

>>> Source/WebCore/html/canvas/WebGL2RenderingContext.h:249
>>> +    GCGLuint getMaxTransformFeedbackSeparateAttribs();
>> 
>> const?
> 
> And name should be maxTransformFeedbackSeparateAttribs()

Done.

>>> Source/WebCore/html/canvas/WebGLProgram.h:77
>>> +    int getRequiredTransformFeedbackBufferCount()
>> 
>> const
> 
> This function isn't const, since cacheInfoIfNeeded() is not, but Darin's comment about not calling getters "get" applies here

Removed "get".

>>> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:93
>>> +    for (auto boundBuffer : m_boundIndexedTransformFeedbackBuffers) {
>> 
>> This will do unnecessary reference count churn. Should just use auto&.
>> 
>> But also, Vector has a find function that I think we can use here.
> 
> I think you can just call m_boundIndexedTransformFeedbackBuffers.contains(buffer)

This function was actually not used, so I deleted it.

>> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:105
>> +    }
> 
> range-based for loop would be better:
> 
>     for (auto& boundBuffer : m_boundIndexedTransformFeedbackBuffers) {
>         if (boundBuffer == &buffer)
>             boundBuffer = nullptr;
>     }

Done.

>> Source/WebCore/html/canvas/WebGLTransformFeedback.h:58
>> +    WebGLProgram* getProgram() const { return m_program.get(); }
> 
> WebKit coding style says we don’t name these functions "get".

Done.

>> Source/WebCore/html/canvas/WebGLTransformFeedback.h:59
>> +    void setProgram(WebGLProgram*);
> 
> Can this take a reference instead of a pointer?

Done.

>> Source/WebCore/html/canvas/WebGLTransformFeedback.h:62
>> +    void unbindBuffer(WebGLBuffer*);
> 
> Can these take a reference instead of a pointer?

Done.

>> Source/WebCore/html/canvas/WebGLTransformFeedback.h:70
>> +    bool m_active = false;
> 
> Style nit: class member initializer defaults should use { } notation.

Done.

Comment on attachment 403605 [details]
address review feedback and fix non-webgl2 builds

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

>> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:64
>> +bool WebGLTransformFeedback::setBoundIndexedTransformFeedbackBuffer(GCGLuint index, WebGLBuffer* buffer)
> 
> You don't check the return value of this. Should it return void?

Done.

>> Source/WebCore/html/canvas/WebGLTransformFeedback.h:51
>> +    bool getBoundIndexedTransformFeedbackBuffer(GCGLuint index, WebGLBuffer** outBuffer);
> 
> Nit: Don't use "get"

The style guide says to use get for getters that use out parameters.

Committed r263999: <https://trac.webkit.org/changeset/263999>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403622 [details].

<rdar://problem/65154525>