Summary: | Put OffscreenCanvas behind a build flag | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Lord <clord> | ||||||||||||||||||||||||
Component: | Canvas | Assignee: | Chris Lord <clord> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, annulen, benjamin, cdumez, clopez, cmarcelo, commit-queue, dbates, dino, eric.carlson, esprehn+autocc, ews-watchlist, graouts, gyuyoung.kim, hi, jbedard, joepeck, kangil.han, keith_miller, kondapallykalyan, mark.lam, msaboff, rakuco, rniwa, ryuan.choi, saam, sergio, tzagallo, webkit-bug-importer, wilander, youennf, zan | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||
Bug Blocks: | 183720 | ||||||||||||||||||||||||||
Attachments: |
|
Description
Chris Lord
2019-10-18 02:21:42 PDT
Created attachment 381295 [details]
Patch
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. Created attachment 381296 [details]
Patch
Created attachment 381297 [details]
Patch
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 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
(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. 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. Created attachment 381669 [details]
Patch
Created attachment 381674 [details]
Patch
Comment on attachment 381674 [details]
Patch
Not there yet...
Created attachment 381677 [details]
Patch
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! 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. Created attachment 381792 [details]
Patch
Created attachment 381795 [details]
Patch
Created attachment 381803 [details]
Patch
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. (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. (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 = ; (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. (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. Created attachment 381898 [details]
Patch
Comment on attachment 381898 [details] Patch Clearing flags on attachment: 381898 Committed r251630: <https://trac.webkit.org/changeset/251630> All reviewed patches have been landed. Closing bug. |