Bug 126935 - Add support for managing WebGL load policy
Summary: Add support for managing WebGL load policy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Roger Fong
URL:
Keywords:
Depends on: 127009
Blocks: 127042 127044
  Show dependency treegraph
 
Reported: 2014-01-13 15:15 PST by Roger Fong
Modified: 2014-01-16 12:47 PST (History)
18 users (show)

See Also:


Attachments
Patch (25.27 KB, patch)
2014-01-13 15:46 PST, Roger Fong
no flags Details | Formatted Diff | Diff
Patch (26.42 KB, patch)
2014-01-13 17:28 PST, Roger Fong
no flags Details | Formatted Diff | Diff
Patch (27.34 KB, patch)
2014-01-13 18:05 PST, Roger Fong
no flags Details | Formatted Diff | Diff
Patch (2.31 KB, patch)
2014-01-13 20:27 PST, Roger Fong
no flags Details | Formatted Diff | Diff
Patch (28.24 KB, patch)
2014-01-14 09:48 PST, Roger Fong
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (547.02 KB, application/zip)
2014-01-14 11:32 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (500.92 KB, application/zip)
2014-01-14 12:06 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (530.11 KB, application/zip)
2014-01-14 12:32 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (496.15 KB, application/zip)
2014-01-14 13:06 PST, Build Bot
no flags Details
WebKit2 Patch (20.42 KB, patch)
2014-01-14 18:58 PST, Roger Fong
no flags Details | Formatted Diff | Diff
WebKit2 Patch v2 (32.75 KB, patch)
2014-01-15 10:23 PST, Roger Fong
no flags Details | Formatted Diff | Diff
WebKit2 patch v3 (18.94 KB, patch)
2014-01-15 16:33 PST, Roger Fong
no flags Details | Formatted Diff | Diff
Patch (18.87 KB, patch)
2014-01-16 11:33 PST, Roger Fong
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roger Fong 2014-01-13 15:15:55 PST
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.
Comment 1 Roger Fong 2014-01-13 15:46:24 PST
Created attachment 221084 [details]
Patch
Comment 2 Build Bot 2014-01-13 16:27:01 PST
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
Comment 3 Build Bot 2014-01-13 16:43:28 PST
Comment on attachment 221084 [details]
Patch

Attachment 221084 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5356839575224320
Comment 4 Roger Fong 2014-01-13 16:58:36 PST
<rdar://problem/15790448>
Comment 5 Roger Fong 2014-01-13 17:28:20 PST
Created attachment 221092 [details]
Patch
Comment 6 Build Bot 2014-01-13 17:53:11 PST
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
Comment 7 Roger Fong 2014-01-13 18:05:43 PST
Created attachment 221094 [details]
Patch
Comment 8 Jon Lee 2014-01-13 19:04:15 PST
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.
Comment 9 Brent Fulgham 2014-01-13 19:31:06 PST
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.
Comment 10 Build Bot 2014-01-13 19:48:00 PST
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
Comment 11 Build Bot 2014-01-13 20:10:49 PST
Comment on attachment 221094 [details]
Patch

Attachment 221094 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6036037650350080
Comment 12 Roger Fong 2014-01-13 20:12:51 PST
(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.
Comment 13 Roger Fong 2014-01-13 20:27:30 PST
Created attachment 221101 [details]
Patch
Comment 14 EFL EWS Bot 2014-01-13 20:37:01 PST
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
Comment 15 Build Bot 2014-01-13 21:01:33 PST
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
Comment 16 Brent Fulgham 2014-01-13 21:04:22 PST
(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.
Comment 17 Build Bot 2014-01-13 21:15:20 PST
Comment on attachment 221101 [details]
Patch

Attachment 221101 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6668306094227456
Comment 18 Roger Fong 2014-01-14 09:45:52 PST
(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..
Comment 19 Roger Fong 2014-01-14 09:47:16 PST
(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.
Comment 20 Roger Fong 2014-01-14 09:48:38 PST
Created attachment 221172 [details]
Patch
Comment 21 Build Bot 2014-01-14 11:32:02 PST
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
Comment 22 Build Bot 2014-01-14 11:32:04 PST
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
Comment 23 Roger Fong 2014-01-14 11:53:04 PST
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+?
Comment 24 Build Bot 2014-01-14 12:06:31 PST
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
Comment 25 Build Bot 2014-01-14 12:06:36 PST
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
Comment 26 Build Bot 2014-01-14 12:32:15 PST
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
Comment 27 Build Bot 2014-01-14 12:32:18 PST
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
Comment 28 Brent Fulgham 2014-01-14 12:42:08 PST
Comment on attachment 221172 [details]
Patch

r=me.
Comment 29 Build Bot 2014-01-14 13:06:45 PST
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
Comment 30 Build Bot 2014-01-14 13:06:50 PST
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
Comment 31 Roger Fong 2014-01-14 13:37:57 PST
https://trac.webkit.org/r162000
Comment 32 Sam Weinig 2014-01-14 13:58:11 PST
(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.
Comment 33 WebKit Commit Bot 2014-01-14 14:06:26 PST
Re-opened since this is blocked by bug 127009
Comment 34 Roger Fong 2014-01-14 17:18:38 PST
Reping for review. We'd like to get this in ASAP.
Comment 35 Roger Fong 2014-01-14 18:58:40 PST
Created attachment 221225 [details]
WebKit2 Patch
Comment 36 Przemyslaw Szymanski 2014-01-15 02:58:42 PST
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.
Comment 37 Piotr Grad 2014-01-15 05:07:46 PST
(In reply to comment #35)
> Created an attachment (id=221225) [details]
> Patch

Please rollback 162036 as this patch breaks WebGL on Win, GTK & EFL.
Comment 38 Roger Fong 2014-01-15 09:52:44 PST
(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?
Comment 39 Roger Fong 2014-01-15 09:55:13 PST
(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?
Comment 40 Roger Fong 2014-01-15 09:58:21 PST
(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.
Comment 41 Roger Fong 2014-01-15 10:08:13 PST
In theory, fixed it here: http://trac.webkit.org/changeset/162078 
Let me know if it's still broken
Comment 42 Roger Fong 2014-01-15 10:23:46 PST
Created attachment 221283 [details]
WebKit2 Patch v2
Comment 43 Roger Fong 2014-01-15 16:33:48 PST
Created attachment 221314 [details]
WebKit2 patch v3
Comment 44 Piotr Grad 2014-01-16 00:21:16 PST
(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.
Comment 45 Brent Fulgham 2014-01-16 09:05:25 PST
(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.
Comment 46 Roger Fong 2014-01-16 11:33:41 PST
Created attachment 221395 [details]
Patch
Comment 47 Tim Horton 2014-01-16 11:42:26 PST
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