Bug 215433 - [WebGL2] Pass user-defined-properties-on-context.html layout test
Summary: [WebGL2] Pass user-defined-properties-on-context.html layout test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords: InRadar
Depends on: 214765
Blocks: 126404 189641
  Show dependency treegraph
 
Reported: 2020-08-12 16:41 PDT by Kenneth Russell
Modified: 2020-08-18 14:25 PDT (History)
15 users (show)

See Also:


Attachments
Patch (10.16 KB, patch)
2020-08-17 18:50 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (12.33 KB, patch)
2020-08-17 18:56 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (17.56 KB, patch)
2020-08-18 13:39 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2020-08-12 16:41:17 PDT
With the fix for Bug 214765, in which expando properties are being preserved correctly for objects latched into the WebGL context, the similar layout test:
webgl/2.0.0/conformance/context/user-defined-properties-on-context.html

is still failing.

In order to fix it, HTMLCanvasElements will have to keep the JavaScript wrapper for their context alive.
Comment 1 Kenneth Russell 2020-08-12 17:04:45 PDT
Also check the following layout test:
webgl/2.0.0/resources/webgl_test_files/conformance/context/context-lost-restored.html

which looks for expando properties on extensions, and which theoretically should already be passing.
Comment 2 Kenneth Russell 2020-08-17 18:50:38 PDT
Created attachment 406762 [details]
Patch
Comment 3 Kenneth Russell 2020-08-17 18:53:08 PDT
ysuzuki@: could you please review this? I'm not sure this is completely correct. My first attempt put the HTMLCanvasElement::renderingContext() into the opaque root set, and that didn't work - JSHTMLCanvasElement::visitAdditionalChildren seemed to be called after the rendering context's isReachableFromOpaqueRoots.
Comment 4 Kenneth Russell 2020-08-17 18:56:07 PDT
Created attachment 406763 [details]
Patch
Comment 5 Darin Adler 2020-08-18 08:38:00 PDT
Comment on attachment 406763 [details]
Patch

I understand how we are taking advantage of the test that’s already present for WebGL 2. But for the project, it would be better if we had test coverage for all three context types.
Comment 6 Yusuke Suzuki 2020-08-18 12:03:02 PDT
Comment on attachment 406763 [details]
Patch

r=me
Comment 7 Kenneth Russell 2020-08-18 12:31:08 PDT
Thanks darin@ and ysuzuki@ for your reviews.

I'll add 2d and webgpu context expando tests. Note that HTMLCanvasElement.getContext("gpu") doesn't exist in the WebGPU spec any more, so I'll add a FIXME in that test about updating it once the implementation matches the spec.
Comment 8 Kenneth Russell 2020-08-18 13:39:30 PDT
Created attachment 406805 [details]
Patch
Comment 9 Kenneth Russell 2020-08-18 13:40:01 PDT
Comment on attachment 406805 [details]
Patch

Added new tests in the latest patch. CQ'ing.
Comment 10 EWS 2020-08-18 14:24:56 PDT
Committed r265833: <https://trac.webkit.org/changeset/265833>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406805 [details].
Comment 11 Radar WebKit Bug Importer 2020-08-18 14:25:24 PDT
<rdar://problem/67352972>