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

[Sam Weinig]

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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20100812/15433122/attachment.html>