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 <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> >>
> >>
> >> On Wed, Aug 11, 2010 at 10:58 PM, Darin Fisher <[email protected]> wrote:
> >>>
> >>> On Wed, Aug 11, 2010 at 10:37 PM, Sam Weinig <[email protected]> wrote:
> >>>>
> >>>> On Wed, Aug 11, 2010 at 10:29 PM, Cedric Vivier <[email protected]>
> >>>> wrote:
> >>>>>
> >>>>> On Thu, Aug 12, 2010 at 13:26, Sam Weinig <[email protected]> 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
> >> [email protected]
> >> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
> >>
> >>
> > _______________________________________________
> > webkit-dev mailing list
> > [email protected]
> > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
> >
>
> _______________________________________________
> webkit-dev mailing list
> [email protected]
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
_______________________________________________
webkit-dev mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
