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.
Created attachment 208178 [details] Vertex attribute binding validation method
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.
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
(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.
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)
Created attachment 208519 [details] Vertex attribute binding validation method update after review
(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.
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.
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
Comment on attachment 208519 [details] Vertex attribute binding validation method Another +1 to Christophe's review. Just minor changes needed before r+
Created attachment 208605 [details] Vertex attribute binding validation method
Christophe, thank you very much for review. Changes applied.
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.
Dean Jackson, could you do a final review for this patch? Thank you.
(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.
Comment on attachment 208605 [details] Vertex attribute binding validation method Clearing flags on attachment: 208605 Committed r154166: <http://trac.webkit.org/changeset/154166>
All reviewed patches have been landed. Closing bug.