[webkit-dev] js binding: function argument type checking

Maciej Stachowiak mjs at apple.com
Fri Aug 13 00:52:08 PDT 2010


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 <kbr at 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 <zhenyao at 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.weinig at gmail.com> wrote:
> >>
> >>
> >> On Wed, Aug 11, 2010 at 10:58 PM, Darin Fisher <darin at chromium.org> wrote:
> >>>
> >>> On Wed, Aug 11, 2010 at 10:37 PM, Sam Weinig <sam.weinig at gmail.com> wrote:
> >>>>
> >>>> On Wed, Aug 11, 2010 at 10:29 PM, Cedric Vivier <cedricv at neonux.com>
> >>>> wrote:
> >>>>>
> >>>>> On Thu, Aug 12, 2010 at 13:26, Sam Weinig <sam.weinig at 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 at lists.webkit.org
> >> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
> >>
> >>
> > _______________________________________________
> > webkit-dev mailing list
> > webkit-dev at lists.webkit.org
> > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
> >
> 
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20100813/133766ef/attachment.html>


More information about the webkit-dev mailing list