Bug 119515 – [WebGL] Vertex attribute binding validation method

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Przemyslaw Szymanski]

validateVertexAttributes method can has a little code refactoring. We can move some calculations to a separate methods.
This also can be usable in other parts of the code. So less code and easy to reuse now.

[Przemyslaw Szymanski]

Created attachment 208178 [details]
Vertex attribute binding validation method

[Kalyan]

Comment on attachment 208178 [details]
Vertex attribute binding validation method

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

> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.h:62
> +        inline bool validateBinding() const { return !enabled || (enabled && isBound()); }

Second check should be enough here.

[Przemyslaw Szymanski]

I can't agreed. Two reasons:
1) I tested only the second check and it was not work well
2) Vertex attribute can be disabled (and then check should be true, it is ok) or enabled but bound. It can't be enabled and not bound.

So there must be two checks

[Kalyan]

(In reply to comment #3)
> I can't agreed. Two reasons:
> 1) I tested only the second check and it was not work well
> 2) Vertex attribute can be disabled (and then check should be true, it is ok) or enabled but bound. It can't be enabled and not bound.
> 
> So there must be two checks

sorry, I missed that you moved the enabled check too.

[zalan]

Comment on attachment 208178 [details]
Vertex attribute binding validation method

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

>> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.h:62
>> +        inline bool validateBinding() const { return !enabled || (enabled && isBound()); }
> 
> Second check should be enough here.

Do you need the second enabled? (if enabled is false, the second part of the condition does not get evaluated, while if enabled is true, then it doesnt bring much value to the second part of the condition)

[Przemyslaw Szymanski]

Created attachment 208519 [details]
Vertex attribute binding validation method

update after review

[Przemyslaw Szymanski]

(In reply to comment #5)
> (From update of attachment 208178 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208178&action=review
> 
> >> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.h:62
> >> +        inline bool validateBinding() const { return !enabled || (enabled && isBound()); }
> > 
> > Second check should be enough here.
> 
> Do you need the second enabled? (if enabled is false, the second part of the condition does not get evaluated, while if enabled is true, then it doesnt bring much value to the second part of the condition)

Good point. Thank you. I updated this code. Review again please.

[Chris Dumez]

Comment on attachment 208519 [details]
Vertex attribute binding validation method

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

> Source/WebCore/ChangeLog:6
> +        This patch refactors WebGLRenderingContext code by move a vertex

"by moving"

> Source/WebCore/ChangeLog:7
> +        attribute binding validation to the separate method. It is now

"to a separate"

> Source/WebCore/ChangeLog:9
> +        code is more readable and more clear now. 

"more readable and more clear": pretty much means the same. Probably one is enough.

> Source/WebCore/ChangeLog:13
> +        No new tests. Covered by existing tests.

There is no behavior change, right? If so, it is a good idea to say so in the Changelog.

> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.h:61
> +        inline bool isBound() const { return bufferBinding && bufferBinding->object(); }

inline is redundant here.

> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.h:62
> +        inline bool validateBinding() const { return !enabled || isBound(); }

Ditto.

[Chris Dumez]

Comment on attachment 208519 [details]
Vertex attribute binding validation method

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

>> Source/WebCore/ChangeLog:6
>> +        This patch refactors WebGLRenderingContext code by move a vertex
> 
> "by moving"

"by moving the" actually

[Dean Jackson]

Comment on attachment 208519 [details]
Vertex attribute binding validation method

Another +1 to Christophe's review. Just minor changes needed before r+

[Przemyslaw Szymanski]

Created attachment 208605 [details]
Vertex attribute binding validation method

[Przemyslaw Szymanski]

Christophe, thank you very much for review. Changes applied.

[Chris Dumez]

Comment on attachment 208605 [details]
Vertex attribute binding validation method

Looks like a nice refactoring. r=me but please let Dean take a final look before landing as he was reviewing this as well.

[Przemyslaw Szymanski]

Dean Jackson, could you do a final review for this patch? Thank you.

[Chris Dumez]

(In reply to comment #14)
> Dean Jackson, could you do a final review for this patch? Thank you.

Dean seems busy. Feel free to land the patch.

[WebKit Commit Bot]

Comment on attachment 208605 [details]
Vertex attribute binding validation method

Clearing flags on attachment: 208605

Committed r154166: <http://trac.webkit.org/changeset/154166>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.