Closed Bug 779611 Opened 12 years ago Closed 12 years ago

Paris bindings for the remaining WebGL interfaces

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

(Whiteboard: webgl-next [bug can be closed now])

Attachments

(15 files, 9 obsolete files)

(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
jgilbert
: review+
Details | Diff | Splinter Review
(deleted), patch
jgilbert
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
jgilbert
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bjacob
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
jgilbert
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
WebGLContext and WebGLUniformLocation are already ported to the new DOM bindings, aka "Paris bindings". Let's do the remaining interfaces in this bug.
OS: Linux → All
Hardware: x86_64 → All
One of the simplest classes, to warm up.
Assignee: nobody → bjacob
Attachment #666652 - Flags: review?(bzbarsky)
r?bz for the bindings aspects, r?jgilbert for the WebGL aspects Some considerations: The IDL for getExtension says it returns "object?" i.e. a nullable JSObject*. There is no such thing as "WebGLExtension" in the IDL. I wanted to align with that: WebGLExtension never was anything more than an implementation detail, but that was unclear in the current code. This patch removes it, but we still need a common base class for WebGL extension, for a few reasons: there is quite a bit of goop that can be usefully abstracted in this way, and without a common base, we pretty much have to store the extension objects as separate member pointers on WebGLContext, which leads to more repetitive goop. WebKit does it that way, but IIUC they don't do cycle collection so they don't have the same kind of goop as we have. So, goodbye WebGLExtension, hello WebGLExtensionBase: new name making it more clear that it's internal, and WebGLExtensionBase is not exposed in the IDL. A goal here is to make per-new-extension goop minimal, as extension implementation is something we often turn into mentored bugs, and also, extensions are less testable than other features (because they are not universally supported) so we want to be paranoid wrt extension-specific bugs.
Attachment #666656 - Flags: review?(jgilbert)
Attachment #666656 - Flags: review?(bzbarsky)
Comment on attachment 666652 [details] [diff] [review] part 1: port WebGLShaderPrecisionFormat >+++ b/content/canvas/src/WebGLContext.h >+ // can't just return the WebGLContext, because nsISupports is an ambiguous base of it Actually, you could do it. You'd just need to define, probably in this file a function like so: inline nsISupports* ToSupports(WebGLContext*) { // Cast it as needed } and the bindings would pick that up and use it. >+ return Context()->GetParentObject(); But this is fine too, generally. Assuming Context() is never null. >+ , public WebGLContextBoundObject I'm trying to understand the ownership model here. The WebGLContextBoundObject holds a weak pointer to the WebGLContext. What makes sure this object doesn't outlive the WebGLContext? But more on this below. >+ = new WebGLShaderPrecisionFormat(this, range[0], range[1], precision); So the only way to get a WebGLShaderPrecisionFormat object is to create a brand-new one? In that case, you could make this not wrappercached at all, like uniformlocation, right? And then you wouldn't need the parent object bits either, I think. The rest looks fine.
Attachment #666652 - Flags: review?(bzbarsky) → review+
Comment on attachment 666653 [details] [diff] [review] part 2: drop old bindings for WebGLUniformLocation r=me
Attachment #666653 - Flags: review?(bzbarsky) → review+
Attached patch part 4: more extensions cleanup (deleted) — Splinter Review
Unrelated to the rest of this bug, but while working on extensions I saw stuff that needed fixing. - constipation of IsExtensionSupported and friends - implement mDisableExtension in one place, not two - don't spread nsStringAPI ugliness around for the extension names comparisons - simplifications and braces-style-fixes in IsExtensionSupported
Attachment #666674 - Flags: review?(jgilbert)
(In reply to Boris Zbarsky (:bz) from comment #4) > >+ = new WebGLShaderPrecisionFormat(this, range[0], range[1], precision); > > So the only way to get a WebGLShaderPrecisionFormat object is to create a > brand-new one? > > In that case, you could make this not wrappercached at all, like > uniformlocation, right? And then you wouldn't need the parent object bits > either, I think. Oh, right! This can only be returned by getShaderPrecisionFormat and the spec says: "Return a new WebGLShaderPrecisionFormat" like for uniform locations. Patch coming.
(In reply to Boris Zbarsky (:bz) from comment #4) > I'm trying to understand the ownership model here. The > WebGLContextBoundObject holds a weak pointer to the WebGLContext. What > makes sure this object doesn't outlive the WebGLContext? Nothing, so I need to fix that. Actually, we seem to have the same problem for all other WebGL-context-bound objects.
Comment on attachment 666656 [details] [diff] [review] part 3: port WebGL extensions, introduce WebGLExtensionBase, kill WebGLExtension, refactor things >+++ b/content/canvas/src/WebGLContext.cpp >+WebGLContext::GetExtension(JSContext *ctx, const nsAString& aName) The canonical name for a JSContext is "cx", for what it's worth. ;) >+++ b/content/canvas/src/WebGLContext.h > bool IsExtensionEnabled(WebGLExtensionID ext) { >+ return mExtensions.Length() > ext && mExtensions[ext]; You could do "return mExtensions.SafeElementAt(ext);" too. >+++ b/content/canvas/src/WebGLExtensionBase.cpp >+WebGLExtensionBase::WrapObject(JSContext *cx, JSObject *scope, bool *triedToWrap) Why is this needed? I don't think you need this at all. Either implemented here, or declared in the header. Subclasses do need it, of course. >+++ b/content/canvas/src/WebGLExtensions.h >+#define DECL_WEBGL_EXTENSION_GOOP \ >+ NS_DECL_ISUPPORTS_INHERITED \ Why do the subclasses need to declare isupports? I don't think they do... >+ JSObject* WrapObject(JSContext *cx, JSObject *scope, bool *triedToWrap); And _this_ class does not need to declare WrapObject. >+ NS_IMPL_ADDREF_INHERITED(WebGLExtensionType, WebGLExtensionBase) \ >+ NS_IMPL_RELEASE_INHERITED(WebGLExtensionType, WebGLExtensionBase) \ >+ NS_INTERFACE_MAP_BEGIN(WebGLExtensionType) \ >+ NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, WebGLExtensionBase) \ >+ NS_INTERFACE_MAP_END_INHERITING(WebGLExtensionBase) And if we don't declare isupports on the subclasses, all this can go away too. >+++ b/dom/bindings/Bindings.conf > 'WebGLShaderPrecisionFormat': { ... >+'WebGLExtensionStandardDerivatives': { Fix the alphabetical order here, please? And within the list of extensions. r=me with those nits.
Attachment #666656 - Flags: review?(bzbarsky) → review+
> Nothing, so I need to fix that. So one option, once all these objects are cycle-collected, is to just have them hold a strong ref... In the meantime, if all we use the pointer for is JS-wrapping we might be ok, because we can only JS-wrap these objects while the context is alive, right?
(In reply to Boris Zbarsky (:bz) from comment #10) > > Nothing, so I need to fix that. > > So one option, once all these objects are cycle-collected, is to just have > them hold a strong ref... That would be a solution, but wouldn't it be a serious performance concern? Remember that WebGL demo that had 20k WebGLUniformLocations... > > In the meantime, if all we use the pointer for is JS-wrapping we might be > ok, because we can only JS-wrap these objects while the context is alive, > right? Ah yes, indeed, if GetParentObject is only used then creating a new JS object wrapping these objects, then that is not possible to do without the WebGL context being live. Do you see this as a possible long-term solution or do you rather see us using strong refs everywhere and relying on CC?
Oh, WebGLUniformLocation is a bound object, right. Well, what we could do for now if we have to is just have the extension objects and shader precision format objects hold strong refs, if needed. There shouldn't be too many of those... > Do you see this as a possible long-term solution Not sure. Peter, what do you think?
Attachment #666656 - Attachment is patch: true
Comment on attachment 666656 [details] [diff] [review] part 3: port WebGL extensions, introduce WebGLExtensionBase, kill WebGLExtension, refactor things Review of attachment 666656 [details] [diff] [review]: ----------------------------------------------------------------- R+ with nits and suggestions. Consider adding macro for the things I mention, since it's a bunch of boilerplate code. I would almost say add macros and throw most (all?) extension .cpp files' contents into one file. ::: content/canvas/src/WebGLContext.cpp @@ +1062,5 @@ > + if (!IsExtensionSupported(ext)) { > + return nullptr; > + } > + > + // step 3: if the extension hadn't been previously been created, create it now Consider adding ", thus enabling it" to the end of this comment. @@ +1103,5 @@ > + JSObject* wrapper = GetWrapper(); > + JSAutoCompartment ac(ctx, wrapper); > + return WrapNewBindingObject(ctx, wrapper, mExtensions[ext], &v) > + ? &v.toObject() > + : nullptr; I don't love putting all this in the return statement. Consider |if (!WrapNew~())\n return nullptr;|, followed by returning &v.toObject() if WrapNew~() succeeds. ::: content/canvas/src/WebGLContext.h @@ +1181,4 @@ > > // returns true if the extension has been enabled by calling getExtension. > bool IsExtensionEnabled(WebGLExtensionID ext) { > + return mExtensions.Length() > ext && mExtensions[ext]; |ext < mExtensions.Length()| I feel is the more common orientation for this. ::: content/canvas/src/WebGLExtensionBase.cpp @@ +10,5 @@ > + > +WebGLExtensionBase::WebGLExtensionBase(WebGLContext* context) > + : WebGLContextBoundObject(context) > +{ > + SetIsDOMBinding(); I shall assume this is magic goop. ::: content/canvas/src/WebGLExtensionLoseContext.cpp @@ +22,1 @@ > WebGLExtensionLoseContext::LoseContext() You could even do the ctor, dtor, goop, and stuff via a macro, and just put LoseContext and RestoreContext below it in the file. ::: content/canvas/src/WebGLExtensionTextureFloat.cpp @@ +16,5 @@ > +WebGLExtensionTextureFloat::~WebGLExtensionTextureFloat() > +{ > +} > + > +IMPL_WEBGL_EXTENSION_GOOP(WebGLExtensionTextureFloat) This file looks like a good candidate for something like IMPL_WEBGL_EXTENSION_PLAIN, or something. This same text modulo extension names is used in a number of the extension .cpp files. ::: content/canvas/src/WebGLExtensions.h @@ +65,4 @@ > }; > > +class WebGLExtensionStandardDerivatives > + : public WebGLExtensionBase For this common case, could we not have a macro like DECL_WEBGL_EXTENSION_CLASS(Name): class WebGLExtension##Name : public WebGLExtensionBase { public: WebGLExtension##Name(WebGLContext*); virtual ~WebGLExtension##Name(); DECL_WEBGL_EXTENSION_GOOP }; ? It looks like all the extensions (with the exception of LoseContext) are just this meme with a different |Name|. ::: dom/base/nsDOMClassInfo.cpp @@ -1590,5 @@ > - DOM_DEFAULT_SCRIPTABLE_FLAGS | > - nsIXPCScriptable::WANT_ADDPROPERTY) > - NS_DEFINE_CLASSINFO_DATA(WebGLExtensionDepthTexture, WebGLExtensionSH, > - DOM_DEFAULT_SCRIPTABLE_FLAGS | > - nsIXPCScriptable::WANT_ADDPROPERTY) We are going to need to update the wiki page on adding an extension. On the bright side, we're going to be updating to say there's less one needs to do. :) ::: dom/webidl/WebGLRenderingContext.webidl @@ -8,5 @@ > * > * Copyright © 2012 Khronos Group > */ > > - // AUTOGENERATED FILE -- DO NOT EDIT -- SEE Makefile Are we no longer auto-gen'd?
Attachment #666656 - Flags: review?(jgilbert) → review+
Comment on attachment 666674 [details] [diff] [review] part 4: more extensions cleanup Review of attachment 666674 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContext.cpp @@ +989,1 @@ > gl->IsExtensionSupported(GLContext::EXT_packed_depth_stencil)) indent alignment issue. @@ +1023,4 @@ > { > ext = OES_texture_float; > } > + else if (CompareWebGLExtensionName(name, "OES_standard_derivatives")) It would be nice if we could use a map for this stuff, instead of this chain of else-ifs. ::: gfx/gl/GLContext.h @@ +1674,5 @@ > EXT_packed_depth_stencil, > Extensions_Max > }; > > + bool IsExtensionSupported(GLExtensions aKnownExtension) const { Const-correct! \o/
Attachment #666674 - Flags: review?(jgilbert) → review+
Now WebGLShaderPrecisionFormat is not wrappercached anymore. WebGLContextBoundObject doesn't provide GetParentObject anymore, instead wrappercached subclasses will have to provide their own, which will be easy thanks to the ToSupports trick. I didn't see any reason anymore to have WebGLShaderPrecisionFormat cycle-collected, as there are no strong references to or from it.
Attachment #667169 - Flags: review?(bzbarsky)
Attachment #667169 - Attachment is patch: true
Applies on top of existing part 3. I think this addresses all of Boris' review comments. (In reply to Jeff Gilbert [:jgilbert] from comment #13) > Consider adding macro for the things I mention, since it's a bunch of > boilerplate code. > > I would almost say add macros and throw most (all?) extension .cpp files' > contents into one file. There are a few things making me uncomfortable about going too far in this direction: * Making whole classes disappear behind macros can make things harder to find, especially if the class name itself is generated by ## inside the macro expansion, but not only. * These classes are really trivial, especially with Boris' comments removing most of the goop. Hiding them behind macros would give them an undeserved aura of scariness, as in http://mozillamemes.tumblr.com/post/29300345308/lost-in-boilerplate-hell-halp * Since there will be exceptions, having macros for the easy cases will only make it harder to explain in the wiki how to proceed in the non-easy cases, whereas if we don't hide stuff behind macros, people will be easy to see the patterns. > I don't love putting all this in the return statement. Consider |if > (!WrapNew~())\n return nullptr;|, followed by returning &v.toObject() if > WrapNew~() succeeds. Done. > |ext < mExtensions.Length()| I feel is the more common orientation for this. Boris' SafeElementAt comment addresses that already. > > ::: content/canvas/src/WebGLExtensionBase.cpp > @@ +10,5 @@ > > + > > +WebGLExtensionBase::WebGLExtensionBase(WebGLContext* context) > > + : WebGLContextBoundObject(context) > > +{ > > + SetIsDOMBinding(); > > I shall assume this is magic goop. This is prime magic goop. > > ::: content/canvas/src/WebGLExtensionLoseContext.cpp > @@ +22,1 @@ > > WebGLExtensionLoseContext::LoseContext() > > You could even do the ctor, dtor, goop, and stuff via a macro, and just put > LoseContext and RestoreContext below it in the file. Here's a proposition about where to draw the line for what to hide behind macros: put in macros what is utter nonsense to the intended audience. Here, the intended audience is novice Gecko coders. The idea is, if people are going to do blind copy-and-paste trial-and-error, better give them a macro, but if people are going to actually understand what they are doing line by line, then no need for a macro. So I claim that WrapObject hits the bar for being put in a macro, but the rest (trivial classes with ctor and dtor) can stay. ok? > > ::: content/canvas/src/WebGLExtensionTextureFloat.cpp > @@ +16,5 @@ > > +WebGLExtensionTextureFloat::~WebGLExtensionTextureFloat() > > +{ > > +} > > + > > +IMPL_WEBGL_EXTENSION_GOOP(WebGLExtensionTextureFloat) > > This file looks like a good candidate for something like > IMPL_WEBGL_EXTENSION_PLAIN, or something. This same text modulo extension > names is used in a number of the extension .cpp files. See above comments about the extent to which we should hide things behind macros. > > ::: content/canvas/src/WebGLExtensions.h > @@ +65,4 @@ > > }; > > > > +class WebGLExtensionStandardDerivatives > > + : public WebGLExtensionBase > > For this common case, could we not have a macro like > DECL_WEBGL_EXTENSION_CLASS(Name): > class WebGLExtension##Name This illustrates another problem with macros: totally confuse code search tools, as the identifier WebGLExtensionStandardDerivatives would not be found anywhere in the code. > We are going to need to update the wiki page on adding an extension. On the > bright side, we're going to be updating to say there's less one needs to do. > :) Yup :) > > ::: dom/webidl/WebGLRenderingContext.webidl > @@ -8,5 @@ > > * > > * Copyright © 2012 Khronos Group > > */ > > > > - // AUTOGENERATED FILE -- DO NOT EDIT -- SEE Makefile > > Are we no longer auto-gen'd? We never were, really. This comment is from the Khronos file and it refers apparently to it being autogenerated from something. Our own IDL is not identical to it anyway, as we have various custom annotations such as [Creator], so that comment was very confusing.
Attachment #667234 - Flags: review?(jgilbert)
Attachment #667234 - Flags: review?(bzbarsky)
> It would be nice if we could use a map for this stuff, instead of this chain > of else-ifs. Yes, that would be very nice. Going further, the "map" (whatever form it takes) should encompass the class name like WebGLExtensionStandardDerivatives. Surely some template magic can do that for us; but it's hard to prioritize it here.
Attachment #667234 - Flags: review?(jgilbert) → review+
Try is green!
Comment on attachment 667169 [details] [diff] [review] part 1.5: address review comments on part 1, and don't cycle collect WebGLShaderPrecisionFormat r=me
Attachment #667169 - Flags: review?(bzbarsky) → review+
Comment on attachment 667234 [details] [diff] [review] part 3.5: address review comments on part 3 You shouldn't need the nullptr second arg to SafeElementAt. The alphabetical ordering in Bindings.conf is still wrong. 'E' comes before 'S'. r=me with that fixed.
Attachment #667234 - Flags: review?(bzbarsky) → review+
Attached file part 5: port WebGLTexture (obsolete) (deleted) —
This also: - introduces some helpers to wrap WebGL objects in a new file, WebGLContextUtils.h. The rationale for a new header is these are template helpers that are needed in at least 2 different cpp files, hence need to be in a header, but I don't want to drag more stuff in WebGLContext.h, rather I want to trim it down. - moves some WebGLTexture stuff into a new file, WebGLTexture.cpp. It's currently small, but I would like us to start moving WebGL-class-specific stuff to specific files.
Attachment #667654 - Flags: review?(bzbarsky)
Attached patch part 5: port WebGLTexture (obsolete) (deleted) — Splinter Review
Now with hg add.
Attachment #667654 - Attachment is obsolete: true
Attachment #667654 - Flags: review?(bzbarsky)
Attachment #667661 - Flags: review?(bzbarsky)
Attached patch part 5: port WebGLBuffer (obsolete) (deleted) — Splinter Review
Attachment #667690 - Flags: review?(bzbarsky)
Attached patch part 6: port WebGLBuffer (obsolete) (deleted) — Splinter Review
with dom_quickstubs.qsconf update
Attachment #667690 - Attachment is obsolete: true
Attachment #667690 - Flags: review?(bzbarsky)
Attachment #667691 - Flags: review?(bzbarsky)
Attached patch part 5: port WebGLTexture (obsolete) (deleted) — Splinter Review
Attachment #667661 - Attachment is obsolete: true
Attachment #667661 - Flags: review?(bzbarsky)
Attachment #667753 - Flags: review?(bzbarsky)
Attached patch part 6: port WebGLBuffer (deleted) — Splinter Review
These new patches properly update WebGLContextNotSupported.cpp which had stubs needed for the --disable-webgl build.
Attachment #667691 - Attachment is obsolete: true
Attachment #667691 - Flags: review?(bzbarsky)
Attachment #667754 - Flags: review?(bzbarsky)
Comment on attachment 667753 [details] [diff] [review] part 5: port WebGLTexture Swallowing false return values from WrapNewBindingObject is really not ok. What that will do, if that actually fails, is trigger a bunch of fatal asserts in the JS engine, and then in an opt build probably do something very bizarre. You really need to make sure to throw an exception if that call fails... I should have pushed harder on this with the extensions patch. Simply passing an ErrorResult into WebGLObjectAsJSValue/Object should do the trick. > + print descriptor.interface.identifier.name Please take that out. That was just debugging gunk we needed, right? > +// /* Avoid conflict with WinAPI */ I don't understand this change. The rest looks fine, but we really need to fix the object-creation bits...
Attachment #667753 - Flags: review?(bzbarsky) → review-
Comment on attachment 667754 [details] [diff] [review] part 6: port WebGLBuffer r=me, I guess, but shouldn't we deal with that WrapObject comment before the class decl?
Attachment #667754 - Flags: review?(bzbarsky) → review+
Attached patch part 5: port WebGLTexture (obsolete) (deleted) — Splinter Review
Thanks for catching all this! Sorry for the random bits that slipped into the patch. Yes, ErrorResult is a thing of genius, now it is much easier to write helper functions that can generate exceptions.
Attachment #667753 - Attachment is obsolete: true
Attachment #667803 - Flags: review?(bzbarsky)
Attached patch part 6: port WebGLBuffer (UPDATED) (obsolete) (deleted) — Splinter Review
Updated to replace the WrapObject calls, while throwing exceptions as discussed on part 5. carrying forward r=bz
Attachment #667806 - Flags: review+
Comment on attachment 666656 [details] [diff] [review] part 3: port WebGL extensions, introduce WebGLExtensionBase, kill WebGLExtension, refactor things Review of attachment 666656 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContext.cpp @@ +1102,5 @@ > + JS::Value v; > + JSObject* wrapper = GetWrapper(); > + JSAutoCompartment ac(ctx, wrapper); > + return WrapNewBindingObject(ctx, wrapper, mExtensions[ext], &v) > + ? &v.toObject() Bah, this really is the kind of code I hoped we'd be able to avoid with the new bindings. Why would we want this instead of wrapping in the binding code?
(In reply to :Ms2ger from comment #35) > Bah, this really is the kind of code I hoped we'd be able to avoid with the > new bindings. Why would we want this instead of wrapping in the binding code? This is why in part 5 we introduce helper functions to do this once and for all.
previous version interverted WebGLBuffer.cpp and WebGLTexture.cpp.
Attachment #667806 - Attachment is obsolete: true
Attachment #667904 - Flags: review+
Attached patch part 5: port WebGLTexture (deleted) — Splinter Review
In WebGLObjectAsJSValue, I forgot to handle the case where the input object is null. That led to a crash in a conformance test.
Attachment #667803 - Attachment is obsolete: true
Attachment #667803 - Flags: review?(bzbarsky)
Attachment #667941 - Flags: review?(bzbarsky)
Attachment #667945 - Flags: review?(bzbarsky)
> Why would we want this instead of wrapping in the binding code? For the moment, because there is no common interface for WebGLExtensions and we don't have support for union return types in WebIDL yet. Also, because the official IDL says "object?". Once we have union return types, we can consider using them here.
Comment on attachment 667941 [details] [diff] [review] part 5: port WebGLTexture r=me. Looks great!
Attachment #667941 - Flags: review?(bzbarsky) → review+
Comment on attachment 667916 [details] [diff] [review] part 7: port WebGLFramebuffer and WebGLRenderbuffer The "NOTE" comments in the header can go away. r=me with that.
Attachment #667916 - Flags: review?(bzbarsky) → review+
Comment on attachment 667945 [details] [diff] [review] part 8: port WebGLShader and WebGLProgram r=me
Attachment #667945 - Flags: review?(bzbarsky) → review+
Backed out on inbound due to Mac build error. I likely should include a header here. http://hg.mozilla.org/integration/mozilla-inbound/rev/a898a428639f
Attached patch part 9: port WebGLActiveInfo (deleted) — Splinter Review
No nsWrapperCache here, as it can only be returned by getActiveUniform and getActiveAttrib which the spec say create new objects. Marked [Creator] accordingly.
Attachment #668150 - Flags: review?(bzbarsky)
Comment on attachment 668150 [details] [diff] [review] part 9: port WebGLActiveInfo r=me
Attachment #668150 - Flags: review?(bzbarsky) → review+
Relanded parts 5 -- 8: was just missing a dom:: before WrapNewBindingsObject. http://hg.mozilla.org/integration/mozilla-inbound/rev/e619280855e5 Now how could this break only on Mac, given that this is in a header? That suggests --- horror! --- that some headers have "using namespace dom". We probably should not do things like "using namespace" in headers, but it seems that we do it a lot: this command finds tons of occurences: $ ack-grep 'using namespace' -G 'h$'
Filed bug 798033 about "using namespace" in headers.
So with this patch, the only goop that remains in WebGLContext.cpp is the class WebGLContext goop.
Attachment #668192 - Flags: review?(jgilbert)
The next step is to remove all remaining traces of the old bindings stuff. This quickly evolves into the wider problem of completely get rid of XPCOM around here. Inded, if we remove the old IDL then we remove nsIDOMWebGLRenderingContext, so the natural way to make that work is to edit nsHTMLCanvasElement::GetContext to stop using the XPCOM factory to produce the context and instead create it in the plain c++ "operator new" way. The XPCOM way is only useful to allow add-on developers to provide their own context types, but that doesn't seem like a compelling enough reason to keep all this stuff around. That was useful to Vlad when he experimented with early WebGL stuff, but - now that WebGL is done, it's not clear that there is another use case. Also, WebGL is generalist enough to allow implementing about any other context type on top of it in JS. - this search found nothing, indicating that no AMO addon is doing that: https://mxr.mozilla.org/addons/search?string=canvas-rendering-context - even if there was a use case, add-on developers don't get to customize the compositor so they would be limited in what they could do. So, OK to remove all the XPCOM trickery in how canvas contexts are created, and to remove addon developers possibility to make new context types?
CC roc and vlad: please look at comment 53.
For what it's worth, I think comment 53 sounds great.
(In reply to Benoit Jacob [:bjacob] from comment #53) > So, OK to remove all the XPCOM trickery in how canvas contexts are created, > and to remove addon developers possibility to make new context types? Yes!
Depends on: 798438
Filed bug 798438 about things in nsHTMLCanvasElement::GetContext that really need fixing (including a large performance penalty due to creating the canvas drawingbuffers multiple times!)
Comment on attachment 668192 [details] [diff] [review] part 10: move remaining non-WebGLContext goop to separate files Review of attachment 668192 [details] [diff] [review]: ----------------------------------------------------------------- I don't see the two files you added in Makefile.in. Did you forget a pair of |hg add|s?
I did!
Attachment #668192 - Attachment is obsolete: true
Attachment #668192 - Flags: review?(jgilbert)
Attachment #668655 - Flags: review?(jgilbert)
Attachment #668655 - Flags: review?(jgilbert) → review+
It seems that we should have been doing this all along. Time-sensitive: we have people on mentored bugs implementing extensions.
Attachment #670450 - Flags: review?(bzbarsky)
Comment on attachment 670450 [details] [diff] [review] part 11: use the original interface names for WebGL extensions r=me if you make all these interfaces [NoInterfaceObject] as we discussed.
Attachment #670450 - Flags: review?(bzbarsky) → review+
This replaces the previous 'part 11' patch. We will need a spec update to make these [NoInterfaceObject] mandatory.
Attachment #670462 - Flags: review?(bzbarsky)
Comment on attachment 670462 [details] [diff] [review] part 11: WebGL extensions should be [NoInterfaceObject] as their compliant IDL interface names would pollute the global object r=me, though we could combine the two if we want the names to be pretty in our own IDL for some reason.
Attachment #670462 - Flags: review?(bzbarsky) → review+
Attachment #670450 - Attachment is obsolete: true
Meh, the first patch makes things rather more complicated, as it gives two greatly diverging families of names for WebGL extensions, and introduces a footgun in passing an incorrect interface name to IMPL_WEBGL_EXTENSION_GOOP, so I don't particularly want to land it since the second patch makes it non-necessary.
_Some_ remnants still remain, mostly nsIDOMWebGLRenderingContext remains for 2 reasons: - context creation still uses a XPCOM factory (see above comments). - about:support still tries to create a WebGL context using the factory directly. This now fails for other reasons (no canvas for this context) see bug 795701; but to keep things building I had to keep MozGetUnderlyingParameter. Will remove it in the course of fixing bug 795701. Let's say that this patch will be the last in this bug --- at this point we declare that WebGL is done porting to WebIDL.
Attachment #670485 - Flags: review?(bzbarsky)
Comment on attachment 670485 [details] [diff] [review] part 12: remove old bindings remnants r=me. Thanks!
Attachment #670485 - Flags: review?(bzbarsky) → review+
Whiteboard: webgl-next [leave open] → webgl-next [bug can be closed now]
Target Milestone: --- → mozilla19
Part 12 is what regressed index-validation -- bug 800657
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: