[webkit-reviews] review granted: [Bug 101826] Notify embedder of lost contexts and allow overriding of WebGL support : [Attachment 173794] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 12 22:55:35 PST 2012


Adam Barth <abarth at webkit.org> has granted Kenneth Russell <kbr at google.com>'s
request for review:
Bug 101826: Notify embedder of lost contexts and allow overriding of WebGL
support
https://bugs.webkit.org/show_bug.cgi?id=101826

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=173794&action=review


> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:404
> +	   // Unlikely; possibly the window is being closed. Not worth
reporting a spurious error in
> +	   // this situation.

I'd skip this comment.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:407
> +    Settings* settings = document->settings();

It's kind of strange to get the settings from the Document given that you've
already gone though the work of getting the frame.  It's better to get the
settings from the frame: frame->settings().  That's all Document::settings does
anyway.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:410
> +    // If the FrameLoaderClient vetoes creation of a new WebGL context
despite the page settings,
> +    // return null.

This comment just says what the code does, not why.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4474
> +	   Document* document = canvas()->document();
> +	   if (document) {

How can document be null here?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4476
> +	       Frame* frame = document->frame();
> +	       if (frame)

You can combine these two lines.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:5619
> +    if (!frame->loader()->client()->allowWebGL(document->settings() &&
document->settings()->webGLEnabled()))

Same comment about how we should get the settings from the frame here since we
already have the frame pointer.


More information about the webkit-reviews mailing list