WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Chris Lord
Comment 1
2019-10-18 05:23:23 PDT
Created
attachment 381295
[details]
Patch
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
Created
attachment 381296
[details]
Patch
Chris Lord
Comment 4
2019-10-18 06:00:12 PDT
Created
attachment 381297
[details]
Patch
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
Created
attachment 381669
[details]
Patch
Chris Lord
Comment 10
2019-10-23 03:46:58 PDT
Created
attachment 381674
[details]
Patch
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
Created
attachment 381677
[details]
Patch
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
Created
attachment 381792
[details]
Patch
Chris Lord
Comment 16
2019-10-24 03:32:48 PDT
Created
attachment 381795
[details]
Patch
Chris Lord
Comment 17
2019-10-24 06:44:05 PDT
Created
attachment 381803
[details]
Patch
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
Created
attachment 381898
[details]
Patch
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
<
rdar://problem/56643266
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug