1324543 - WebGL2RenderingContext interface doesn't match spec

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

We have it inheriting from WebGLRenderingContext, whereas the spec does not.

It didn't match spec when it initially landed in bug 1002302 either...

Comment 1

[Boris Zbarsky [:bzbarsky]]

> It didn't match spec when it initially landed in bug 1002302 either...

That is, did not match spec as it was written then (or now).

Comment 2

[Kelsey Gilbert [:jgilbert]]

The implementation matches the spec as much as it's expected to.

Is there anything in particular you wanted out of this bug? Without at least improving the DOM bindings generation to disambiguation with union types, we can't copy the idl directly.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Is there anything in particular you wanted out of this bug? 

Well, the observable behavior we have now is that Object.getPrototypeOf(WebGL2RenderingContext.prototype) == WebGLRenderingContext.prototype, whereas per spec it should be Object.prototype.  Plus all the consequences of that.  We should fix that, no?

> Without at least improving the DOM bindings generation to disambiguation with union types

Please tell me more.  What exactly is the issue?  Is there a bug filed on whatever this problem is?

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3)
> > Is there anything in particular you wanted out of this bug? 
> 
> Well, the observable behavior we have now is that
> Object.getPrototypeOf(WebGL2RenderingContext.prototype) ==
> WebGLRenderingContext.prototype, whereas per spec it should be
> Object.prototype.  Plus all the consequences of that.  We should fix that,
> no?

I'll check with the WG for what behavior we want here.

However, this is the sort of thing that the WG has declined to standardize in the past. In particular, we've punted on standardizing the interface names of extensions, so instanceof hierarchy for WebGL objects in general is not considered reliable. I do think we should fix this at some point, but until we ship WebGL 2, we're punting on this.

I'll leave this open, but marking P5.


> > Without at least improving the DOM bindings generation to disambiguation with union types
> 
> Please tell me more.  What exactly is the issue?  Is there a bug filed on
> whatever this problem is?

This is a thing I have asked about before, and been told it's a bunch of extra unstaffed work.
The gist is that our DOM bindings generator doesn't handle when it has an overloaded function where the types at the discriminating argument index include a union. The WebGL specs have a number of such overloads. I do not remember filing a bug for this.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I'll check with the WG for what behavior we want here.

Well, the current spec explicitly says we want Object.getPrototypeOf(WebGL2RenderingContext.prototype) == Object.prototype, so...

> However, this is the sort of thing that the WG has declined to standardize in the past.

You can't "decline to standardize" this while using web IDL.  Web IDL specifies what all the prototype chains look like.  This is very much web-observable, and I was told in https://github.com/KhronosGroup/WebGL/pull/2208#issuecomment-267896082 that the WG is explicitly trying to not have an "is-a" relationship here.

I agree that the issue is somewhat academic until we're actually shipping, so P5 is fine as long as we fix this before we ship.

> The gist is that our DOM bindings generator doesn't handle when it has an
> overloaded function where the types at the discriminating argument index include a union

This doesn't seem relevant to the inheritance vs "implements" (or just copy/paste, which is what we have in practice) issue here, right?  In that it looks like we've copied various WebGLRenderingContext bits into WebGLRenderingContext2 _and_ are also inheriting.  We could just stop inheriting and copy all the bits we care about.

In terms of just using the WebGL spec's IDL, I agree that this blocks us for the moment and we need to figure out how to proceed, but that's unrelated to this bug.  There's a bug tracking this issue at 1224090 but it's quite hard to actually fix.  If we're getting close to this blocking us, please let me know and I will try to either figure out a workaround for the webgl2 interface or a way to fix the codegen, but the latter is likely to be weeks of work if it needs to happen.

Comment 6

[Kelsey Gilbert [:jgilbert]]

We're shipping in 51, it's up to you if you want to argue for it to block release. However, I likely do not have time to fix this before Beta starts freezing.

There is a lot of glue code to connect what the DOM bindings generator gives us with how we handle these functions. (mostly recombining overloads) I suspect this will be perturbed by a structural change like this, particularly since you're indicating that the top-of-tree spec webidl isn't valid.

I'm marking this wontfix for 51 for now, classifying this as a P3 non-blocking (though valid) bug. We can probably target 52 for fixing it.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Move WebGL2RenderingContext somewhat closer to being aligned to the spec, insofar as it's possible to do so given the state of the spec. The key change is that it no longer inherits from WebGLRenderingContext; instead the shared things are pulled into bo

Try push at <https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec358cdfb73803a5362ef3b0fcbbf84f2c0cddca>.  I hope we have decent tests for this.  ;)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixing this in beta seems a bit too risky, I agree.  It's unfortunate that we didn't discover this earlier.  :(

> I suspect this will be perturbed by a structural change like this

I don't think so, actually.  We just have to make sure that all the things we were picking up by inheritance are pulled in via WebGLRenderingContextBase.  I think the attached patch does that.  The other minor changes are due to the C++ concrete type we're making the call changing from WebGLContext to WebGL2Context for all the things moved to WebGLRenderingContextBase.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: With the obvious bug fixed

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Argh, actually with the bug fixed

Comment 11

[Boris Zbarsky [:bzbarsky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6758cb17da3307eff85387903fa3953ca6bc57c

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8820172 [details] [diff] [review]
Argh, actually with the bug fixed

Review of attachment 8820172 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, but let's hold off on landing this until 54 and/or 51 ships.

Comment 13

[Kelsey Gilbert [:jgilbert]]

For clarity: We'd start landing this after 54, and uplift to 52 at the beginning of its beta.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I can do that, I guess, but why is that preferable to getting more bake time for this and only having to uplift to Aurora, not Beta?  Not to mention the mental overhead of having to remember this when 53 branches...

Comment 15

[Kelsey Gilbert [:jgilbert]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #14)
> I can do that, I guess, but why is that preferable to getting more bake time
> for this and only having to uplift to Aurora, not Beta?  Not to mention the
> mental overhead of having to remember this when 53 branches...

It's least painful to hold off. There's a relative ton of movement between 53 and 51 right now in preparation for shipping, and minimizing the set of changes that are in 53 but not targeted for 51 is important, so as to guarantee that we don't miss anything.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/729992b82a43
Fix inheritance in our webidl. - r=jgilbert,bz

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8820172 [details] [diff] [review]
Argh, actually with the bug fixed

Approval Request Comment
[Feature/Bug causing the regression]: webgl2
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/729992b82a43

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8820172 [details] [diff] [review]
Argh, actually with the bug fixed

Fix WebGL2 related issue. Beta51+ & Aurora52+. Should be in 51 beta 10.

Comment 20

[Iris Hsiao [:ihsiao]]

Check-in: https://hg.mozilla.org/releases/mozilla-aurora/rev/201b4eb48284

Comment 21

[Iris Hsiao [:ihsiao]]

sorry had to back this out for WebGL2RenderingContextBinding.cpp bustage, i.e., https://treeherder.mozilla.org/logviewer.html#?job_id=4530414&repo=mozilla-aurora#L7518

Comment 22

[Boris Zbarsky [:bzbarsky]]

> sorry had to back this out for WebGL2RenderingContextBinding.cpp bustage

Did you include bug 1317625 in the push?  If not, then that explains it.

If you want a version of this patch that doesn't depend on bug 1317625, please let me know.

Comment 23

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e58d9eef4c96

Comment 24

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-beta/rev/903642766352

Comment 25

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f5deff76fdfccdf38fd9b631f597210d85c8b3e5