Closed Bug 632587 Opened 9 years ago Closed 9 years ago

implement getSupportedExtensions and isContextLost

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: vlad, Assigned: vlad)

Details

(Whiteboard: [softblocker])

Attachments

(3 files, 1 obsolete file)

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.
Attachment #510810 - Flags: review?(bjacob)
Attachment #510810 - Flags: review?(bjacob) → review+
blocking2.0: --- → final+
Whiteboard: [softblocker]
http://hg.mozilla.org/mozilla-central/rev/399f8c5eda4e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Backed out due to test failures

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=80211f053c46
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Surely we aren't taking patches with uuid changes anymore?
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.
Oh, I see, it was just part of the broken push.  Well, nothing in this patch caused it :)
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.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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.
Backed out per bz and bsmedberg

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7a889fa3d09c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
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.
So... how do we add a new method without breaking extensions?
> We should not be making uuid changes anymore; if we really needed to add
> this, it should have gone on a new temporary interface.
(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.
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 ?
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.
(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.
> 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.
> 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.
Thanks Kyle and Boris for explaining this to me. It's clear now.

Vlad --> please make updated patch
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.
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.
Attachment #513616 - Flags: review?(vladimir)
Summary: implement getSupportedExtensions → implement getSupportedExtensions and isContextLost
@ 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 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....
Attachment #513615 - Flags: review?(bzbarsky) → review-
Actually, tests did. Thanks for the pointer of where to look.
Are those imported into our tree?  If not, file a followup to do so?
(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?
(Thanks to Kyle for answering this over irc).
Comment on attachment 515400 [details] [diff] [review]
add new interface class

This version works (has the QI fix)
Attachment #515400 - Attachment description: spli → add new interface class
Attachment #515400 - Attachment is patch: true
Attachment #515400 - Attachment mime type: application/octet-stream → text/plain
Attachment #515400 - Flags: review?(bzbarsky)
Attachment #513615 - Attachment is obsolete: true
Comment on attachment 515400 [details] [diff] [review]
add new interface class

Thank you!
Attachment #515400 - Flags: review?(bzbarsky) → review+
Attachment #513616 - Flags: review?(vladimir) → review?(jmuizelaar)
Attachment #513616 - Flags: review?(jmuizelaar) → review+
Comment on attachment 510810 [details] [diff] [review]
implement GetSupportedExtensions()

Fold this and Vlad's patch into one when committing.
Attachment #510810 - Flags: approval2.0+
Attachment #513616 - Flags: approval2.0+
Attachment #515400 - Flags: approval2.0+
You need to log in before you can comment on or make changes to this bug.