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.
Created attachment 173423 [details] Patch
@dino: please review this. Happy to field any questions about it. Thanks.
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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...
(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?
> 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)?
(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.
> 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.
(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.
Created attachment 173794 [details] Patch
@abarth: addressed your review feedback. Please re-review. Thanks.
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.
Looks great. My comments above are just nits.
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.
Created attachment 174005 [details] Patch for landing
Comment on attachment 174005 [details] Patch for landing Clearing flags on attachment: 174005 Committed r134525: <http://trac.webkit.org/changeset/134525>
All reviewed patches have been landed. Closing bug.