Bug 126935 – Add support for managing WebGL load policy

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

[Roger Fong]

Add support for handling WebGL load policy.
This includes adding a letting the UI process know when we are attempting to create a webgl context and providing API to query for the WebGL load policy associated with a host.

[Roger Fong]

Created attachment 221084 [details]
Patch

[Build Bot]

Comment on attachment 221084 [details]
Patch

Attachment 221084 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4710223524659200

[Build Bot]

Comment on attachment 221084 [details]
Patch

Attachment 221084 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5356839575224320

[Roger Fong]

<rdar://problem/15790448>

[Roger Fong]

Created attachment 221092 [details]
Patch

[Build Bot]

Comment on attachment 221092 [details]
Patch

Attachment 221092 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6281278001250304

[Roger Fong]

Created attachment 221094 [details]
Patch

[Jon Lee]

Comment on attachment 221094 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:235
> +                        return 0;

nullptr.

> Source/WebCore/html/HTMLCanvasElement.cpp:238
> +                        return 0;

ditto.

[Brent Fulgham]

Comment on attachment 221094 [details]
Patch

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

This looks great! My only suggestion would be to make as many of these 'const' as possible.

> Source/WebCore/loader/FrameLoaderClient.h:334
> +        virtual WebGLLoadPolicy webGLPolicyForHost(const String&) { return WebGLAsk; }

Could this be const?

> Source/WebKit2/UIProcess/WebLoaderClient.h:108
> +    void webGLLoadPolicy(WebPageProxy*, WebCore::WebGLLoadPolicy&, const String&);

Const?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2684
> +void WebPageProxy::webGLContextCreated(const String& pageURLString)

Const!

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2689
> +void WebPageProxy::webGLPolicyForHost(const String& host, uint32_t& loadPolicy)

Could probably be const.

> Source/WebKit2/UIProcess/WebUIClient.cpp:211
> +void WebUIClient::webGLContextCreated(WebPageProxy* page, const String& originatingURL)

Const

> Source/WebKit2/UIProcess/API/C/WKAPICast.h:476
> +inline WebCore::WebGLLoadPolicy toWebGLLoadPolicy(WKWebGLLoadPolicy webGLLoadPolicy)

Const.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1382
> +WebCore::WebGLLoadPolicy WebFrameLoaderClient::webGLPolicyForHost(const String& host)

Const as well.

[Build Bot]

Comment on attachment 221094 [details]
Patch

Attachment 221094 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5550322348982272

[Build Bot]

Comment on attachment 221094 [details]
Patch

Attachment 221094 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6036037650350080

[Roger Fong]

(In reply to comment #9)

I'm assuming you mean the methods themselves should be const'ified if they're returning something that isn't a pointer (and hence won't be modified) right?

All the parameters are about as const'ified as I can make em already. A lot of them are references that definitely shouldn't be const.

[Roger Fong]

Created attachment 221101 [details]
Patch

[EFL EWS Bot]

Comment on attachment 221101 [details]
Patch

Attachment 221101 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/4586082461024256

[Build Bot]

Comment on attachment 221101 [details]
Patch

Attachment 221101 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6522478666973184

[Brent Fulgham]

(In reply to comment #12)
> (In reply to comment #9)
> 
> I'm assuming you mean the methods themselves should be const'ified if they're returning something that isn't a pointer (and hence won't be modified) right?

Right. These methods don't seem to modify any internal state, so it would be best to mark them const to maximize the methods where you can call your new interface.

[Build Bot]

Comment on attachment 221101 [details]
Patch

Attachment 221101 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6668306094227456

[Roger Fong]

(In reply to comment #17)
> (From update of attachment 221101 [details])
> Attachment 221101 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/6668306094227456

My local run of build-webkit tells me that everything built swimmingly...not sure what's going on here..

[Roger Fong]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 221101 [details] [details])
> > Attachment 221101 [details] [details] did not pass mac-ews (mac):
> > Output: http://webkit-queues.appspot.com/results/6668306094227456
> 
> My local run of build-webkit tells me that everything built swimmingly...not sure what's going on here..

And now EWS is telling me to get rid of the things that it just told me to add in.
Will reland the exact same patch again, because I don't trust EWS.

[Roger Fong]

Created attachment 221172 [details]
Patch

[Build Bot]

Comment on attachment 221172 [details]
Patch

Attachment 221172 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5621195886034944

New failing tests:
http/tests/canvas/webgl/origin-clean-conformance.html
http/tests/webgl/1.0.2/texSubImage2DHTML.html
http/tests/security/webgl-remote-read-remote-image-allowed.html
http/tests/security/webgl-remote-read-remote-image-allowed-with-credentials.html
http/tests/webgl/1.0.2/texImage2DHTML.html
http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html

[Build Bot]

Created attachment 221184 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Roger Fong]

It is unsurprising that these tests are now failing seeing as the we won't immediately load because a webglcontext has now been created and those tests are being run from a server.

I will skip these tests in a followup patch and will make the correct fix for them in the near future.
All previous review comments have been addressed, can I get an r+?

[Build Bot]

Comment on attachment 221172 [details]
Patch

Attachment 221172 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5490028792774656

New failing tests:
http/tests/canvas/webgl/origin-clean-conformance.html
http/tests/webgl/1.0.2/texSubImage2DHTML.html
http/tests/security/webgl-remote-read-remote-image-allowed.html
http/tests/security/webgl-remote-read-remote-image-allowed-with-credentials.html
http/tests/webgl/1.0.2/texImage2DHTML.html
http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html

[Build Bot]

Created attachment 221186 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221172 [details]
Patch

Attachment 221172 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5138185071886336

New failing tests:
http/tests/canvas/webgl/origin-clean-conformance.html
http/tests/webgl/1.0.2/texSubImage2DHTML.html
http/tests/security/webgl-remote-read-remote-image-allowed.html
http/tests/security/webgl-remote-read-remote-image-allowed-with-credentials.html
http/tests/webgl/1.0.2/texImage2DHTML.html
http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html

[Build Bot]

Created attachment 221188 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Brent Fulgham]

Comment on attachment 221172 [details]
Patch

r=me.

[Build Bot]

Comment on attachment 221172 [details]
Patch

Attachment 221172 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6404822467084288

New failing tests:
http/tests/canvas/webgl/origin-clean-conformance.html
http/tests/webgl/1.0.2/texSubImage2DHTML.html
http/tests/security/webgl-remote-read-remote-image-allowed.html
http/tests/security/webgl-remote-read-remote-image-allowed-with-credentials.html
http/tests/webgl/1.0.2/texImage2DHTML.html
http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html

[Build Bot]

Created attachment 221193 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Roger Fong]

https://trac.webkit.org/r162000

[Sam Weinig]

(In reply to comment #28)
> (From update of attachment 221172 [details])
> r=me.

This needs review from a WebKit2 maintainer.  Please roll this out until that happens.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 127009

[Roger Fong]

Reping for review. We'd like to get this in ASAP.

[Roger Fong]

Created attachment 221225 [details]
WebKit2 Patch

[Przemyslaw Szymanski]

Hello. I'm not an expert but with this patch MiniBrowser with debug build and EFL port does not support WebGL. Try to run Khronos conformance tests:
https://www.khronos.org/registry/webgl/sdk/tests/webgl-conformance-tests.html?version=1.0.3

or this sites
http://get.webgl.org/
https://www.khronos.org/registry/webgl/sdk/demos/webkit/SpiritBox.html

This page fails to create webgl context because this code is called:
if (policy == WebGLAsk) {
   page->chrome().client().webGLContextCreated(this);
   return nullptr;
}

In my opinion we have a bug now.

[Piotr Grad]

(In reply to comment #35)
> Created an attachment (id=221225) [details]
> Patch

Please rollback 162036 as this patch breaks WebGL on Win, GTK & EFL.

[Roger Fong]

(In reply to comment #37)
> (In reply to comment #35)
> > Created an attachment (id=221225) [details] [details]
> > Patch
> 
> Please rollback 162036 as this patch breaks WebGL on Win, GTK & EFL.

I'm seeing all green on Win. Where do you see the failure?

[Roger Fong]

(In reply to comment #38)
> (In reply to comment #37)
> > (In reply to comment #35)
> > > Created an attachment (id=221225) [details] [details] [details]
> > > Patch
> > 
> > Please rollback 162036 as this patch breaks WebGL on Win, GTK & EFL.
> 
> I'm seeing all green on Win. Where do you see the failure?

Oh you must mean WinCairo. What kind of failures are you seeing?

[Roger Fong]

(In reply to comment #36)
> Hello. I'm not an expert but with this patch MiniBrowser with debug build and EFL port does not support WebGL. Try to run Khronos conformance tests:
> https://www.khronos.org/registry/webgl/sdk/tests/webgl-conformance-tests.html?version=1.0.3
> 
> or this sites
> http://get.webgl.org/
> https://www.khronos.org/registry/webgl/sdk/demos/webkit/SpiritBox.html
> 
> This page fails to create webgl context because this code is called:
> if (policy == WebGLAsk) {
>    page->chrome().client().webGLContextCreated(this);
>    return nullptr;
> }
> 
> In my opinion we have a bug now.

Ah you're right. I'll need to add some sort of check to disable this check when the client doesn't want it.
I'll comment it out for now and put a fix me.

[Roger Fong]

In theory, fixed it here: http://trac.webkit.org/changeset/162078 
Let me know if it's still broken

[Roger Fong]

Created attachment 221283 [details]
WebKit2 Patch v2

[Roger Fong]

Created attachment 221314 [details]
WebKit2 patch v3

[Piotr Grad]

(In reply to comment #39)
> (In reply to comment #38)
> > (In reply to comment #37)
> > > (In reply to comment #35)
> > > > Created an attachment (id=221225) [details] [details] [details] [details]
> > > > Patch
> > > 
> > > Please rollback 162036 as this patch breaks WebGL on Win, GTK & EFL.
> > 
> > I'm seeing all green on Win. Where do you see the failure?
> 
> Oh you must mean WinCairo. What kind of failures are you seeing?

I didn't mean build break, but that WebGL was not working. Don't know if its working now.

[Brent Fulgham]

(In reply to comment #44)
> > > > Please rollback 162036 as this patch breaks WebGL on Win, GTK & EFL.
> > > 
> > > I'm seeing all green on Win. Where do you see the failure?
> > 
> > Oh you must mean WinCairo. What kind of failures are you seeing?
> 
> I didn't mean build break, but that WebGL was not working. Don't know if its working now.

This is now working for me.

I think he was referring to the WebGL load policy defaulting to 'off', with no way to activate it. For example, yesterday's OS X nightly couldn't be used for WebGL content because it rejected every WebGL site.

That behavior was fixed by your changes yesterday.

[Roger Fong]

Created attachment 221395 [details]
Patch

[Tim Horton]

Comment on attachment 221395 [details]
Patch

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

> Source/WebKit2/UIProcess/WebLoaderClient.cpp:337
> +WebCore::WebGLLoadPolicy WebLoaderClient::webGLLoadPolicy(WebPageProxy* page, const String& site) const

s/site/url/, I think?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2683
> +void WebPageProxy::webGLPolicyForSite(const String& site, uint32_t& loadPolicy)

s/Site/URL/, I think?

> Source/WebKit2/UIProcess/WebPageProxy.h:930
> +    void webGLPolicyForSite(const String& site, uint32_t& loadPolicy);

ditto

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:37
> +    WebGLPolicyForSite(String host) -> (uint32_t loadPolicy)

ditto

> Source/WebKit2/UIProcess/API/C/WKAPICast.h:488
> +    return WebCore::WebGLAsk;

your default in other places seems to be Allow; why Ask here?

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1342
> +WebCore::WebGLLoadPolicy WebFrameLoaderClient::webGLPolicyForSite(const String& site) const

ditto

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.h:190
> +    virtual WebCore::WebGLLoadPolicy webGLPolicyForSite(const String&) const OVERRIDE;

ditto

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:603
> +WebCore::WebGLLoadPolicy WebPage::webGLPolicyForSite(WebFrame* frame, const String& url)

ditto (and here hilariously the argument is url already)

> Source/WebKit2/WebProcess/WebPage/WebPage.h:293
> +    WebCore::WebGLLoadPolicy webGLPolicyForSite(WebFrame*, const String&);

ditto

[Roger Fong]

Committed: http://trac.webkit.org/changeset/162141
All patches and related patches:

http://trac.webkit.org/changeset/162141
http://trac.webkit.org/changeset/162099
http://trac.webkit.org/changeset/162078
http://trac.webkit.org/changeset/162036
http://trac.webkit.org/changeset/162002