632587 - implement getSupportedExtensions and isContextLost

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: implement GetSupportedExtensions()

This call was missing from our implementation.  It's supposed to return an array of supported extension names; we currently don't have any, so return an empty array.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/399f8c5eda4e

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Backed out due to test failures

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=80211f053c46

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Surely we aren't taking patches with uuid changes anymore?

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

new interface in Fx4 that's not for external use; it may still change in .0.x releases as well, though this should be the final missing API.

What were the test failures?  I didn't think anything in the current version of the test suite actually touched this.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Oh, I see, it was just part of the broken push.  Well, nothing in this patch caused it :)

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Re-landed:
http://hg.mozilla.org/mozilla-central/rev/7ed4cb02ffbf

This had indeed nothing to do with the failure, it just was part of the same push.

Comment 7

[Boris Zbarsky [:bzbarsky]]

No, the point is that we promised API stability for extensions several betas back.  We should not be making uuid changes anymore; if we really needed to add this, it should have gone on a new temporary interface.

We really can't promise people "we won't break you" and then break them.

And it doesn't matter that this is a new interface in Fx4.  If it was present at all in earlier betas, it can't change now.  Adding new interfaces is ok, of course.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Backed out per bz and bsmedberg

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7a889fa3d09c

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Here's a naive question. Here's the code of the interface that changed uuid:


-[scriptable, uuid(2f21ca21-9720-4eee-ad94-27eefe4f72dc)]
+[scriptable, uuid(7cb54577-df8d-40bf-b7bb-2ddec3600a2c)]
 interface nsIDOMWebGLRenderingContext : nsISupports
 {
   //
@@ -836,5 +836,6 @@ interface nsIDOMWebGLRenderingContext : 
   [noscript] DOMString mozGetUnderlyingParamString(in WebGLenum pname);
 
   // extensions
+  nsIVariant getSupportedExtensions();
   nsISupports getExtension(in DOMString name);
 };


the only change there was to add getSupportedExtensions();

Why did that require a uuid change? Or didn't it?

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It changes the vtable of the underlying C++ impl.  Anything that was trying to call getExtension would be calling getSupportedExtensions instead if you didn't change the uuid.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

So... how do we add a new method without breaking extensions?

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

> We should not be making uuid changes anymore; if we really needed to add
> this, it should have gone on a new temporary interface.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #12)
> > We should not be making uuid changes anymore; if we really needed to add
> > this, it should have gone on a new temporary interface.

But the WebGL spec says that this is a method on the WebGLRenderingContext.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

And what's the plan when WebGL 1.1 comes out and wants to add new entries in nsIDOMWebGLRenderingContext while keeping compatibility with WebGL 1.0 ?

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Seems to me like a huge bug in our IDL system. DOM interfaces need to be extensible without breaking extensions everytime. If a direct/naive mapping to c++ virtual methods doesn't allow that, then we need a different approach.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #13)
> (In reply to comment #12)
> > > We should not be making uuid changes anymore; if we really needed to add
> > > this, it should have gone on a new temporary interface.
> 
> But the WebGL spec says that this is a method on the WebGLRenderingContext.

There is no one to one correspondence between xpidl interfaces and dom "interfaces".  You just need to add your xpidl nsIDOMWebGLRendingContext_MOZILLA_2_0_BRANCH to http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#4011

(In reply to comment #14)
> And what's the plan when WebGL 1.1 comes out and wants to add new entries in
> nsIDOMWebGLRenderingContext while keeping compatibility with WebGL 1.0 ?

Nobody said the interfaces can't change between major releases.

(In reply to comment #15)
> Seems to me like a huge bug in our IDL system. DOM interfaces need to be
> extensible without breaking extensions everytime. If a direct/naive mapping to
> c++ virtual methods doesn't allow that, then we need a different approach.

It doesn't work that way.  DOMClassInfo allows a DOM "interface" to map to multiple xpidl interfaces.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> But the WebGL spec says that this is a method on the WebGLRenderingContext.

How would content script tell?  Compare nsIDOMNode and nsIDOM3Node...

> And what's the plan when WebGL 1.1 comes out and wants to add new entries in
> nsIDOMWebGLRenderingContext while keeping compatibility with WebGL 1.0 ?

We add them early enough in a release cycle that we're doing it before the interface freeze.

Again, the interface is not frozen forever.  It's frozen until the Gecko 2.0 release, because we are past the point at which we told everyone that interfaces will not change until the 2.0 release.  Interfaces can obviously change between major versions.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Seems to me like a huge bug in our IDL system. 

That's a separate argument, but unless you're proposing we revamp that for 2.0, has no bearing on this bug.

Kyle, the classinfo thing doesn't quite do what you think it does, but that's a separate issue as well.

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Thanks Kyle and Boris for explaining this to me. It's clear now.

Vlad --> please make updated patch

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

You guys have already spent more time talking about the uuid change than it would have taken to just leave this in, given the extremely unlikely chance of this breaking anyone -- this won't break script, and the WebGL interface isn't that usable outside of a script context (nor is there much of a reason to use it).  Maybe in the future we can have [internalonlygoaway] interfaces.

Comment 21

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: split into new nsIDOMWebGLRenderingContext_MOZILLA_2_0_BRANCH to preserve compatibility

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: add isContextLost

I take this occasion to add another missing method: webgl.isContextLost(). Indeed, since adding this without breaking compatibility is nontrivial, better do it in the same push.

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

@ Boris, Kyle: thanks for the explanations, doing this patch has been quite pedagogic for teaching me XPCOM.

@ Vlad: with this change, together with khronos/webgl bug 439, we are "compliant" wrt context-lost stuff, and even without that patch we pass the test (since the tests only _really_ test stuff when the extension WEBKIT_lose_context is present).

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 513615 [details] [diff] [review]
split into new nsIDOMWebGLRenderingContext_MOZILLA_2_0_BRANCH to preserve compatibility

You need to fix the QI implementation too, right?

Tests would probably have caught that....

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

Actually, tests did. Thanks for the pointer of where to look.

Comment 26

[Benoit Jacob [:bjacob] (mostly away)]

Well, with this patch applied to webgl tests:
http://www.khronos.org/bugzilla/show_bug.cgi?id=442

Comment 27

[Boris Zbarsky [:bzbarsky]]

Are those imported into our tree?  If not, file a followup to do so?

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #27)
> Are those imported into our tree?  If not, file a followup to do so?

This is bug 635059.

(In reply to comment #24)
> Comment on attachment 513615 [details] [diff] [review]
> split into new nsIDOMWebGLRenderingContext_MOZILLA_2_0_BRANCH to preserve
> compatibility
> 
> You need to fix the QI implementation too, right?

Can you give me a little pointer/example for this 'QI (I guess QueryInterface) implementation' thing?

Comment 29

[Benoit Jacob [:bjacob] (mostly away)]

(Thanks to Kyle for answering this over irc).

Comment 30

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: add new interface class

Comment 31

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 515400 [details] [diff] [review]
add new interface class

This version works (has the QI fix)

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 515400 [details] [diff] [review]
add new interface class

Thank you!

Comment 33

[Joe Drew (not getting mail)]

Comment on attachment 510810 [details] [diff] [review]
implement GetSupportedExtensions()

Fold this and Vlad's patch into one when committing.

Comment 34

[Benoit Jacob [:bjacob] (mostly away)]

on tryserver:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=a5deeb7f91ce

Comment 35

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/23ef0e8b9e12
http://hg.mozilla.org/mozilla-central/rev/4110a0918c4c
http://hg.mozilla.org/mozilla-central/rev/c0b114d35e7b