[webkit-reviews] review granted: [Bug 215599] [WebGL2] Pass context-lost-restored.html test : [Attachment 406895] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Kenneth Russell <kbr at google.com>'s
request for review:
Bug 215599: [WebGL2] Pass context-lost-restored.html test
https://bugs.webkit.org/show_bug.cgi?id=215599

Attachment 406895: Patch

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




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

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

> Source/WebCore/html/canvas/WebGLExtension.cpp:41
> +WebGLExtension::WebGLExtension(WebGLRenderingContextBase* context)
>      : m_context(context)
>  {
>  }

So I guess this is where Darin is saying it should be a &, but leave m_context
as a *.

> Source/WebCore/html/canvas/WebGLExtension.h:79
> +    // Lose this extension. Passing true means force loss of the
> +    // extension. Some extensions like WEBGL_lose_context are not
> +    // normally lost when the context is lost, but must be lost when
> +    // destroying their WebGLRenderingContextBase.
> +    virtual void lose(bool);
> +    bool isLost() { return !m_context; }

I guess an alternative would be for the extension to hold a WeakPtr to
WebGLRenderingContextBase, which would avoid having to call lose(). I'm not
sure that's better, and I guess you'd still need a way to force it on
WebGLLoseContext.

Could we use a more descriptive name for the parameter to lose()?

We already have WebGLRenderingContextBase::LostContextMode. Can we use it?
Would it be clear that RealLostContext is a forced one? Maybe we could rename
LostContextMode...

enum class LoseContextMode {
  NotForced,
  Forced
};

virtual void loseContext(LoseContextMode == NotForced);

I also think loseContext over lose, since that's what it is actually doing.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1031
> +	   loseExtensions(true);

So if it was a WeakPtr this is where it would automatically disappear.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:6097
> +    loseExtensions(mode == RealLostContext);

If we use LostContextMode we can just pass this mode along.