[webkit-reviews] review denied: [Bug 50364] Throw webglcontextlost and webglcontextrestored events when a WebGL context is lost and restored. : [Attachment 75555] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Alexey Marinichev
<amarinichev at chromium.org>'s request for review:
Bug 50364: Throw webglcontextlost and webglcontextrestored events when a WebGL
context is lost and restored.
https://bugs.webkit.org/show_bug.cgi?id=50364

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

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

Looks good overall. A few minor issues.

> WebCore/html/canvas/WebGLRenderingContext.cpp:97
> +	   startOneShot(1);

Instead of 1, use a const double with a descriptive name like
secondsBetweenRestoreAttempts.

> WebCore/html/canvas/WebGLRenderingContext.cpp:106
> +	       startOneShot(1);

Const double instead of 1.

> WebCore/html/canvas/WebGLRenderingContext.cpp:2125
> +	   m_restoreTimer.startOneShot(1);

Const double instead of 1.

> WebCore/html/canvas/WebGLRenderingContext.h:67
> +class WebGLRenderingContextRestoreTimer : public TimerBase {
> +public:
> +    WebGLRenderingContextRestoreTimer(WebGLRenderingContext* context) :
m_context(context) { }
> +private:
> +    virtual void fired();
> +    WebGLRenderingContext* m_context;
> +};

In WebKit code each class generally goes into its own header file. In this case
I think you ought to be able to forward declare this class as long as you
change the data member to a RefPtr below.

> WebCore/html/canvas/WebGLRenderingContext.h:357
> +    WebGLRenderingContextRestoreTimer m_restoreTimer;

Please try making this a RefPtr<WebGLRenderingContextRestoreTimer> and see
whether that allows the declaration and definition of
WebGLRenderingContextRestoreTimer to move to the .cpp file.