[webkit-reviews] review granted: [Bug 104733] Make WebGLRenderingContext inherit from ActiveDOMObject : [Attachment 179321] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Brandon Jones
<bajones at chromium.org>'s request for review:
Bug 104733: Make WebGLRenderingContext inherit from ActiveDOMObject
https://bugs.webkit.org/show_bug.cgi?id=104733

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=179321&action=review


> Source/WebCore/ChangeLog:18
> +	   (WebCore):

Remove this useless line.

> Source/WebCore/ChangeLog:22
> +	   (WebCore::WebGLRenderingContext::WebGLRenderingContext):
> +	   (WebCore::WebGLRenderingContext::~WebGLRenderingContext):
> +	   (WebCore::WebGLRenderingContext::destroyGraphicsContext3D):
> +	   (WebCore::WebGLRenderingContext::stop):

DarinA always encourages me to write per-function comments, especially when
they are easy like this, and I appreciate it. Let me pay it forward!

> Source/WebCore/ChangeLog:24
> +	   (WebGLRenderingContext):

And this useless line.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4597
> +	   // Will this cause anything else to panic and die?

You should probably preface this with FIXME. It's a fairly scary statement.

> Source/WebCore/html/canvas/WebGLRenderingContext.h:321
> +    // Document notification

Maybe be clear that this block of functions comes from ActiveDOMObject? I did a
quick search through WebCore and sometimes people do this, other times not.