Bug 203146 - Put OffscreenCanvas behind a build flag
Summary: Put OffscreenCanvas behind a build flag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords: InRadar
Depends on:
Blocks: 183720
  Show dependency treegraph
 
Reported: 2019-10-18 02:21 PDT by Chris Lord
Modified: 2019-10-26 00:14 PDT (History)
32 users (show)

See Also:


Attachments
Patch (80.52 KB, patch)
2019-10-18 05:23 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (81.10 KB, patch)
2019-10-18 05:52 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (81.00 KB, patch)
2019-10-18 06:00 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
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 Details
Patch (1.23 MB, patch)
2019-10-23 02:56 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.23 MB, patch)
2019-10-23 03:46 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.24 MB, patch)
2019-10-23 07:58 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (2.32 MB, patch)
2019-10-24 02:35 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (2.32 MB, patch)
2019-10-24 03:32 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.27 MB, patch)
2019-10-24 06:44 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.31 MB, patch)
2019-10-25 02:32 PDT, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Lord 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.
Comment 1 Chris Lord 2019-10-18 05:23:23 PDT
Created attachment 381295 [details]
Patch
Comment 2 Zan Dobersek 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.
Comment 3 Chris Lord 2019-10-18 05:52:36 PDT
Created attachment 381296 [details]
Patch
Comment 4 Chris Lord 2019-10-18 06:00:12 PDT
Created attachment 381297 [details]
Patch
Comment 5 EWS Watchlist 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
Comment 6 EWS Watchlist 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
Comment 7 Chris Lord 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.
Comment 8 Zan Dobersek 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.
Comment 9 Chris Lord 2019-10-23 02:56:08 PDT
Created attachment 381669 [details]
Patch
Comment 10 Chris Lord 2019-10-23 03:46:58 PDT
Created attachment 381674 [details]
Patch
Comment 11 Chris Lord 2019-10-23 05:19:36 PDT
Comment on attachment 381674 [details]
Patch

Not there yet...
Comment 12 Chris Lord 2019-10-23 07:58:26 PDT
Created attachment 381677 [details]
Patch
Comment 13 Chris Lord 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!
Comment 14 youenn fablet 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.
Comment 15 Chris Lord 2019-10-24 02:35:34 PDT
Created attachment 381792 [details]
Patch
Comment 16 Chris Lord 2019-10-24 03:32:48 PDT
Created attachment 381795 [details]
Patch
Comment 17 Chris Lord 2019-10-24 06:44:05 PDT
Created attachment 381803 [details]
Patch
Comment 18 Ryosuke Niwa 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.
Comment 19 Chris Lord 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.
Comment 20 Ryosuke Niwa 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 = ;
Comment 21 Ryosuke Niwa 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.
Comment 22 Chris Lord 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.
Comment 23 Chris Lord 2019-10-25 02:32:25 PDT
Created attachment 381898 [details]
Patch
Comment 24 WebKit Commit Bot 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>
Comment 25 WebKit Commit Bot 2019-10-26 00:13:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Radar WebKit Bug Importer 2019-10-26 00:14:19 PDT
<rdar://problem/56643266>