[Webkit-unassigned] [Bug 38180] Remove unneeded custom code for WebSocket.send

Wed Apr 28 09:08:33 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=38180

--- Comment #20 from Adam Barth <abarth at webkit.org>  2010-04-28 09:08:33 PST ---
(In reply to comment #18)
> > https://bugs.webkit.org/show_bug.cgi?id=38230 about
> 
> For WebSocket specifically, that would be a regression from this patch, right?

Yes it would.  However, the case is quite rare and we have this bug almost
everywhere already.  I'd be happy to fix it in the code generator so that the
fix can be applied to every non-custom binding.

> As mentioned in comment 6, if we're regression this behavior, it makes no sense
> to do that only for send() - constructWebSocket also has this check.

I don't understand this comment.  Is there a way to generate constructWebSocket
automatically?  I haven't looked at it yet.

> > I think the old behavior is really bogus because it magically catches and 
> > mutates the exception.
> 
> What do WebIDL or other applicable specs say?

I'm not sure, but if WebIDL says we should mutate the exception, then we have
many, many other parts of our bindings that are also out of spec.

My point is that autogenerating this code makes it work the same as the vast,
vast majority of our bindings.  If there are bugs in the autogenerated code,
then those bugs exist everywhere and should be fixed.  As far as I can tell,
there's no reason why this specific API should have custom code that's randomly
different than the rest of our code.  That's just entropy for the sake of
entropy.  Autogenerating this code is a step forward even if we randomly respin
a bunch of arbitrary subtleties in the API's behavior because we're making
those subtleties match how the rest of the bindings work.

(In reply to comment #19)
> (In reply to comment #12)
> > > I have to agree with Alexey on this one and would add that we should aim to
> > > separate cleanup from functionality change.
> > 
> > That's a noble aim, but more or less impossible.  The custom bindings are
> > riddled with trivial mistakes.  It seems a fools errand to document that we
> > don't have these specific mistakes.  There's are infinite sea of trivial
> > mistakes we could have.
> 
> For my benefit, can you list some of them?

Here's a list off the top of my head:

1) The order in which arguments are coerced to various types can be observed
use toString and valueOf.
2) What happens when each of the argument separately (or in combination) throw
exceptions: Are the other arguments coerced?  Do any side effects happen?  Does
the exception get swallowed?  Which exception do you get thrown?  What if the
underlying operation generates an exception?
3) Relatedly, if the method operates on objects, when are the identities of
those object frozen?  For example, if you mutate the DOM or the Frame hierarchy
during coercion, do you get the objects from before the mutation or after the
mutation?

Those are just the ones I can think of related into how and when you coerce
arguments.  As we remove more custom code, I'm sure we'll find lost more
exciting bugs.  For example, in NodeIterator, we saw that the custom code was
computing DOM wrappers using the wrong global object.

The bindings are extremely high-touch regions of code because the web platform
can poke at them very directly.  Essentially, the only way to ensure uniform
behavior is to autogenerate them.  The custom bindings we have are a necessary
evil in some places, but by and large just filled with trivial (and not so
trivial) bugs.

> > > And to reiterate, any functionality change here should be tested.
> > 
> > In that case, we should accept any new custom bindings code without testing all
> > the trivial behaviors it has since every time we write custom bindings we
> > introduce tons of subtle functionality changes.
> 
> We expect new code to be well tested, but we need to set a higher bar on
> changes to existing code since it is a change in existing behavior.  For this
> change in particular, it seems the correct way to go would be to remove the
> number of arguments check first (with a test as we would require if that was
> the only thing you wanted to do) and then auto-generate it.

Well, the patch I posted keeps the argument check using the
"RequiresAllArguments" IDL attribute.  We can remove that attribute later, if
we like.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.