[webkit-reviews] review granted: [Bug 126941] [WebGL2] Sampler objects : [Attachment 387999] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Justin Fan <justin_fan at apple.com>'s
request for review:
Bug 126941: [WebGL2] Sampler objects
https://bugs.webkit.org/show_bug.cgi?id=126941

Attachment 387999: Patch

https://bugs.webkit.org/attachment.cgi?id=387999&action=review




--- Comment #4 from Dean Jackson <dino at apple.com> ---
Comment on attachment 387999
  --> https://bugs.webkit.org/attachment.cgi?id=387999
Patch

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

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1182
> +	   for (auto& samplerSlot : m_boundSamplers) {
> +	       if (samplerSlot == sampler)
> +		   samplerSlot = nullptr;

Rather than relying on the fact you got a ref out of the loop, I think doing
this via an index would be more obvious. e.g. 

for (int i = 0; i < m_boundSamplers.length(); i++) {
  if (sampler == m_boundSamplers[i])
    m_boundSamplers[i] = nullptr;
}

.. even though it is more verbose and old-school. But that's just my opinion. I
guess this is the only object that can be bound in multiple places.

I wouldn't be surprised if everyone else disagrees with me here.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1206
> +

Maybe check if it is already bound to that unit and return early?