[webkit-reviews] review denied: [Bug 83513] WebGLRenderingContext should defer caching program info : [Attachment 136535] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Zhenyao Mo <zmo at google.com>'s
request for review:
Bug 83513: WebGLRenderingContext should defer caching program info
https://bugs.webkit.org/show_bug.cgi?id=83513

Attachment 136535: Patch
https://bugs.webkit.org/attachment.cgi?id=136535&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=136535&action=review


Looks OK in general but r- for one API simplification. One question as well
below. Also, was this tested with the test case I vaguely recall from the
Chromium bug report to demonstrate the performance improvement?

> Source/WebCore/html/canvas/WebGLProgram.h:64
> +    void invalidateCachedInfo() { m_infoValid = false; }

It would be better to make this private and call it from the implementation of
increaseLinkCount(). This would make the API simpler and reduce the possibility
of errors.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3135
> +	       if (program->getLinkStatus())

Why make this change? We know that the program can't link successfully, and
avoiding this call will avoid a bunch of caching work that we want to defer as
long as possible.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3143
> +    program->invalidateCachedInfo();

Calling invalidateCachedInfo() as a side-effect of increaseLinkCount() would
allow this line to be deleted.