I think doing type checks in the bindings makes sense as a long-term strategy. Only the bindings level can tell actually detect wrong type errors; the C++ layer can't distinguish wrong type from a validly passed null. WebGL interfaces are not the only ones that have this problem. Here is a DOM Core example of the bug:
document.body.insertBefore(document.createElement("div"), "foo") This will append a div to the body, when it should throw. As far as tactics go: 1) I think it's much better to deploy this gradually than to all bindings at once; if a pervasive change causes a regression, then the regression becomes much harder to track down. 2) In many cases, this *will* change behavior. When deploying the new approach to methods where it changes behavior, we should create regression tests showing the behavior change. Regards, Maciej On Aug 12, 2010, at 7:11 PM, Sam Weinig wrote: > As I mentioned, I am not necessarily against ever changing the behavior, but > if we do, we should make sure to remove all the existing checks, as they > become an unnecessary branch. It just seems to me like that should be a > separate change than a bug due to ambiguity. > > -Sam > > On Thu, Aug 12, 2010 at 4:21 PM, Kenneth Russell <k...@google.com> wrote: > For what it's worth, I think this change should be made for all of the > DOM bindings, not just those for WebGL. The IDL code generators' > support for overloaded methods already generates TypeError when an > incoming argument doesn't implement any of the interfaces required by > the overloaded variants. The new behavior will be closer to the rules > specified by Web IDL in > http://dev.w3.org/2006/webapi/WebIDL/#es-interface and also, as I > understand it, closer to what Firefox implements. > > It would be possible to add support for another extended attribute to > the code generators and annotate all of the methods in > WebGLRenderingContext.idl, but I really think the default behavior > should be changed. > > -Ken > > On Thu, Aug 12, 2010 at 1:15 PM, Mo, Zhenyao <zhen...@gmail.com> wrote: > > Hardly. Right now we already do the type checking in the generated > > toType(argument) functions. Instead of casting to null, we throw a > > TypeError, which adds no extra cost if the type is correct. > > > > Besides, where I looked, after toType(argument) call, exception is > > checked. Only that currently toType(argument) is not generating > > exceptions. > > > > Mo > > > > On Thu, Aug 12, 2010 at 9:20 AM, Sam Weinig <sam.wei...@gmail.com> wrote: > >> > >> > >> On Wed, Aug 11, 2010 at 10:58 PM, Darin Fisher <da...@chromium.org> wrote: > >>> > >>> On Wed, Aug 11, 2010 at 10:37 PM, Sam Weinig <sam.wei...@gmail.com> wrote: > >>>> > >>>> On Wed, Aug 11, 2010 at 10:29 PM, Cedric Vivier <cedr...@neonux.com> > >>>> wrote: > >>>>> > >>>>> On Thu, Aug 12, 2010 at 13:26, Sam Weinig <sam.wei...@gmail.com> wrote: > >>>>>> > >>>>>> For this specific case, it seems like you could easily check for a null > >>>>>> WebGLProgram* in WebGLRenderingContext::useProgram and set the > >>>>>> ExceptionCode. > >>>>> > >>>>> Nope, null is a valid argument, it bounds to the initial program, which > >>>>> means nothing will be drawn with WebGL. > >>>>> Certainly not the expected behavior when one pass the wrong type to the > >>>>> argument like Zhenyao pointed out, therefore throwing TypeError really > >>>>> makes > >>>>> sense here (and elsewhere with WebGL API). > >>>> > >>>> Ok, in that case, it seems like you need to do it in the bindings for > >>>> this. I would prefer not making a sweeping change at this time, and > >>>> instead > >>>> keeping the changes just to places where the extra checking is necessary > >>>> due > >>>> to ambiguity (as in this useProgram case). > >>>> -Sam > >>> > >>> Out of curiosity, if we have the ability for code to be auto generated > >>> from the IDL, why not use it universally? I'm trying to guess to > >>> understand > >>> your preference :) > >>> -Darin > >> > >> My concern with doing it universally is the performance cost of doing the > >> check twice in many places (once in the bindings and once in the > >> implementation with a null check). We could certainly re-evaluate the way > >> we > >> do these type checks, potentially even converting the existing null checks > >> in the implementation to asserts, but I think that discussion shouldn't be > >> conflated with this bug fix. > >> -Sam > >> _______________________________________________ > >> webkit-dev mailing list > >> webkit-dev@lists.webkit.org > >> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev > >> > >> > > _______________________________________________ > > webkit-dev mailing list > > webkit-dev@lists.webkit.org > > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev > > > > _______________________________________________ > webkit-dev mailing list > webkit-dev@lists.webkit.org > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev