Bug 203146 – Put OffscreenCanvas behind a build flag

• Component: Canvas
• Status: RESOLVED FIXED

[Chris Lord]

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.

[Chris Lord]

Created attachment 381295 [details]
Patch

[Zan Dobersek]

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]

Created attachment 381296 [details]
Patch

[Chris Lord]

Created attachment 381297 [details]
Patch

[EWS Watchlist]

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]

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]

(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]

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]

Created attachment 381669 [details]
Patch

[Chris Lord]

Created attachment 381674 [details]
Patch

[Chris Lord]

Comment on attachment 381674 [details]
Patch

Not there yet...

[Chris Lord]

Created attachment 381677 [details]
Patch

[Chris Lord]

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]

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]

Created attachment 381792 [details]
Patch

[Chris Lord]

Created attachment 381795 [details]
Patch

[Chris Lord]

Created attachment 381803 [details]
Patch

[Ryosuke Niwa]

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]

(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]

(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]

(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]

(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]

Created attachment 381898 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 381898 [details]
Patch

Clearing flags on attachment: 381898

Committed r251630: <https://trac.webkit.org/changeset/251630>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56643266>