RESOLVED FIXED 203146
Put OffscreenCanvas behind a build flag
https://bugs.webkit.org/show_bug.cgi?id=203146
Summary Put OffscreenCanvas behind a build flag
Chris Lord
Reported 2019-10-18 02:21:42 PDT
Currently OffscreenCanvas is enabled when ImageBitmap is enabled, and both are controlled by the experimental-features flag. OffscreenCanvas is going to require some pretty hairy changes with regards to threaded behaviour though, so it makes sense to separate these two. I also suggest that OffscreenCanvas should be enabled when running tests, preferably on a platform that runs on EWS.
Attachments
Patch (80.52 KB, patch)
2019-10-18 05:23 PDT, Chris Lord
no flags
Patch (81.10 KB, patch)
2019-10-18 05:52 PDT, Chris Lord
no flags
Patch (81.00 KB, patch)
2019-10-18 06:00 PDT, Chris Lord
no flags
Archive of layout-test-results from ews211 for win-future (13.71 MB, application/zip)
2019-10-18 07:45 PDT, EWS Watchlist
no flags
Patch (1.23 MB, patch)
2019-10-23 02:56 PDT, Chris Lord
no flags
Patch (1.23 MB, patch)
2019-10-23 03:46 PDT, Chris Lord
no flags
Patch (1.24 MB, patch)
2019-10-23 07:58 PDT, Chris Lord
no flags
Patch (2.32 MB, patch)
2019-10-24 02:35 PDT, Chris Lord
no flags
Patch (2.32 MB, patch)
2019-10-24 03:32 PDT, Chris Lord
no flags
Patch (1.27 MB, patch)
2019-10-24 06:44 PDT, Chris Lord
no flags
Patch (1.31 MB, patch)
2019-10-25 02:32 PDT, Chris Lord
no flags
Chris Lord
Comment 1 2019-10-18 05:23:23 PDT
Zan Dobersek
Comment 2 2019-10-18 05:46:24 PDT
Comment on attachment 381295 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381295&action=review > Source/WebCore/html/OffscreenCanvas.cpp:150 > } > +#endif Nit: I think normally we do an extra empty line before #endif, to match the whitespace around the matching #if.
Chris Lord
Comment 3 2019-10-18 05:52:36 PDT
Chris Lord
Comment 4 2019-10-18 06:00:12 PDT
EWS Watchlist
Comment 5 2019-10-18 07:45:06 PDT
Comment on attachment 381297 [details] Patch Attachment 381297 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13147527 New failing tests: http/wpt/offscreen-canvas/offscreencanvas.constructor.html fast/canvas/offscreen-enabled.html http/wpt/offscreen-canvas/transferToImageBitmap-empty.html
EWS Watchlist
Comment 6 2019-10-18 07:45:08 PDT
Created attachment 381300 [details] Archive of layout-test-results from ews211 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews211 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Chris Lord
Comment 7 2019-10-18 07:53:25 PDT
(In reply to Build Bot from comment #5) > Comment on attachment 381297 [details] > Patch > > Attachment 381297 [details] did not pass win-ews (win): > Output: https://webkit-queues.webkit.org/results/13147527 > > New failing tests: > http/wpt/offscreen-canvas/offscreencanvas.constructor.html > fast/canvas/offscreen-enabled.html > http/wpt/offscreen-canvas/transferToImageBitmap-empty.html Nice catch buildbot.
Zan Dobersek
Comment 8 2019-10-21 06:29:25 PDT
It might be simpler to just skip OffscreenCanvas tests on ports that won't have the feature enabled from the get-go. That way we avoid running tests that don't test much, especially if they're constructed in a way where the lack of exposed API leads to timeouts. Otherwise, the patch is fine.
Chris Lord
Comment 9 2019-10-23 02:56:08 PDT
Chris Lord
Comment 10 2019-10-23 03:46:58 PDT
Chris Lord
Comment 11 2019-10-23 05:19:36 PDT
Comment on attachment 381674 [details] Patch Not there yet...
Chris Lord
Comment 12 2019-10-23 07:58:26 PDT
Chris Lord
Comment 13 2019-10-23 09:32:49 PDT
I'm a bit stumped on this, if anyone could enlighten me as to why OffscreenCanvas still seems to be enabled on mac-wk2, I'd greatly appreciate it!
youenn fablet
Comment 14 2019-10-23 09:36:12 PDT
I believe WTR is enabling all experimental features. If you do not want that, you can probably override it in WTR code. Or not mark your runtime flag as experimental.
Chris Lord
Comment 15 2019-10-24 02:35:34 PDT
Chris Lord
Comment 16 2019-10-24 03:32:48 PDT
Chris Lord
Comment 17 2019-10-24 06:44:05 PDT
Ryosuke Niwa
Comment 18 2019-10-24 23:58:59 PDT
Comment on attachment 381803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381803&action=review > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:311 > +ENABLE_OFFSCREEN_CANVAS = ENABLE_OFFSCREEN_CANVAS; I don't think we want to enable this by default on Apple's WebKit ports for now.
Chris Lord
Comment 19 2019-10-25 00:39:17 PDT
(In reply to Ryosuke Niwa from comment #18) > Comment on attachment 381803 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=381803&action=review > > > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:311 > > +ENABLE_OFFSCREEN_CANVAS = ENABLE_OFFSCREEN_CANVAS; > > I don't think we want to enable this by default on Apple's WebKit ports for > now. It shouldn't be - the build option defaults to off except on Gtk and WPE and the runtime option defaults to off and is internal. This is contrary to before this patch, where it was being built on all platforms and enabled with experimental features.
Ryosuke Niwa
Comment 20 2019-10-25 00:49:13 PDT
(In reply to Chris Lord from comment #19) > (In reply to Ryosuke Niwa from comment #18) > > Comment on attachment 381803 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=381803&action=review > > > > > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:311 > > > +ENABLE_OFFSCREEN_CANVAS = ENABLE_OFFSCREEN_CANVAS; > > > > I don't think we want to enable this by default on Apple's WebKit ports for > > now. > > It shouldn't be - the build option defaults to off except on Gtk and WPE and > the runtime option defaults to off and is internal. This is contrary to > before this patch, where it was being built on all platforms and enabled > with experimental features. I don't understand. This line clearly enables ENABLE_OFFSCREEN_CANVAS. It needs to look like this for the feature to be compiled out: ENABLE_OFFSCREEN_CANVAS = ;
Ryosuke Niwa
Comment 21 2019-10-25 00:51:49 PDT
(In reply to Chris Lord from comment #13) > I'm a bit stumped on this, if anyone could enlighten me as to why > OffscreenCanvas still seems to be enabled on mac-wk2, I'd greatly appreciate > it! This alone indicates that things aren't compiled out. If the feature is compiled out, then there is no way for WTR to be enabling thees features. WTR doesn't randomly compile & enable things that are disabled at compilation time.
Chris Lord
Comment 22 2019-10-25 02:25:07 PDT
(In reply to Ryosuke Niwa from comment #20) > (In reply to Chris Lord from comment #19) > I don't understand. This line clearly enables ENABLE_OFFSCREEN_CANVAS. It > needs to look like this for the feature to be compiled out: > ENABLE_OFFSCREEN_CANVAS = ; You're right, I misunderstood how this worked - will update in the next submission.
Chris Lord
Comment 23 2019-10-25 02:32:25 PDT
WebKit Commit Bot
Comment 24 2019-10-26 00:13:08 PDT
Comment on attachment 381898 [details] Patch Clearing flags on attachment: 381898 Committed r251630: <https://trac.webkit.org/changeset/251630>
WebKit Commit Bot
Comment 25 2019-10-26 00:13:11 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 26 2019-10-26 00:14:19 PDT
Note You need to log in before you can comment on or make changes to this bug.