We have it inheriting from WebGLRenderingContext, whereas the spec does not. It didn't match spec when it initially landed in bug 1002302 either...

> 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).

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.

> 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?

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

> 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.

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.

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

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 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.

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

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...

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

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

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 on attachment 8820172 [details] [diff] [review] Argh, actually with the bug fixed Fix WebGL2 related issue. Beta51+ & Aurora52+. Should be in 51 beta 10.

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

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

> 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.