Bug 101826 – Notify embedder of lost contexts and allow overriding of WebGL support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

If WebGL content causes the context to be lost per the GL_ARB_robustness extension, the embedder should be notified, and to have the option to veto creation of new WebGL contexts and restoration of existing ones.

The FrameLoaderClient already contains the hooks to request permissions of the embedder, so this is the natural place to put the new hooks.

[Kenneth Russell]

Created attachment 173423 [details]
Patch

[Kenneth Russell]

@dino: please review this. Happy to field any questions about it. Thanks.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 173423 [details]
Patch

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

> Source/WebKit/chromium/public/WebPermissionClient.h:112
> +    // Notifies the client that a WebGL context was lost on this page with the given reason (one of
> +    // the GL_ARB_robustness status codes; see Extensions3D.h in WebCore/platform/graphics).
> +    virtual void didLoseWebGLContext(WebFrame*, int) { }

Why is this call routed through the WebPermissionClient?  I would have expected it to come through the WebFrameClient...

[Kenneth Russell]

(In reply to comment #4)
> (From update of attachment 173423 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173423&action=review
> 
> > Source/WebKit/chromium/public/WebPermissionClient.h:112
> > +    // Notifies the client that a WebGL context was lost on this page with the given reason (one of
> > +    // the GL_ARB_robustness status codes; see Extensions3D.h in WebCore/platform/graphics).
> > +    virtual void didLoseWebGLContext(WebFrame*, int) { }
> 
> Why is this call routed through the WebPermissionClient?  I would have expected it to come through the WebFrameClient...

There seemed to be parallels with other methods on WebPermissionClient like didNotAllowPlugins and didNotAllowScript, but WebFrameClient looks like a good place for it too. Should I move it and regenerate the patch?

[Adam Barth]

> There seemed to be parallels with other methods on WebPermissionClient like didNotAllowPlugins and didNotAllowScript, but WebFrameClient looks like a good place for it too.

Those are related to permissions (e.g., we didn't allow script to run for some reason).

> Should I move it and regenerate the patch?

Is didLoseWebGLContext related to permissions, or is it a more general notification that we lost the WebGL context (for any of a number of reasons)?

[Kenneth Russell]

(In reply to comment #6)
> > There seemed to be parallels with other methods on WebPermissionClient like didNotAllowPlugins and didNotAllowScript, but WebFrameClient looks like a good place for it too.
> 
> Those are related to permissions (e.g., we didn't allow script to run for some reason).
> 
> > Should I move it and regenerate the patch?
> 
> Is didLoseWebGLContext related to permissions, or is it a more general notification that we lost the WebGL context (for any of a number of reasons)?

The calls are related; the intent is that the embedder will change its answer to allowWebGL (possibly even on other frames) if it receives a didLoseWebGLContext notification.

[Adam Barth]

> The calls are related; the intent is that the embedder will change its answer to allowWebGL (possibly even on other frames) if it receives a didLoseWebGLContext notification.

If it doesn't cause too much trouble, I'd prefer to move the callback to another client.  WebKit shouldn't really know that the calls are related.  There are many things that might cause the embedder to change its answer to allowWebGL.  That's up to the embedder.

[Kenneth Russell]

(In reply to comment #8)
> > The calls are related; the intent is that the embedder will change its answer to allowWebGL (possibly even on other frames) if it receives a didLoseWebGLContext notification.
> 
> If it doesn't cause too much trouble, I'd prefer to move the callback to another client.  WebKit shouldn't really know that the calls are related.  There are many things that might cause the embedder to change its answer to allowWebGL.  That's up to the embedder.

OK, no problem. I'll move the didLoseWebGLContext notification to WebFrameClient.

[Kenneth Russell]

Created attachment 173794 [details]
Patch

[Kenneth Russell]

@abarth: addressed your review feedback. Please re-review. Thanks.

[Adam Barth]

Comment on attachment 173794 [details]
Patch

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.

[Adam Barth]

Looks great.  My comments above are just nits.

[Kenneth Russell]

Comment on attachment 173794 [details]
Patch

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

>> 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.

Thanks; I didn't know how these were linked up. I'll update the code.

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:410
>> +    // return null.
> 
> This comment just says what the code does, not why.

Will improve the comment.

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4474
>> +        if (document) {
> 
> How can document be null here?

We've seen situations (probably during window closing) where some of the linkage between these objects is broken. Those may have just been bugs in WebGL's context restoration code, but I'd prefer to program defensively here.

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4476
>> +            if (frame)
> 
> You can combine these two lines.

OK.

>> 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.

Will adjust.

[Kenneth Russell]

Created attachment 174005 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 174005 [details]
Patch for landing

Clearing flags on attachment: 174005

Committed r134525: <http://trac.webkit.org/changeset/134525>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.