[webkit-reviews] Fix to Recent KJS_CHECK_EXCEPTION Change

[Darin Adler]

On Jun 9, 2005, at 7:26 AM, Mark Rowe wrote:

> As Darin Adler mentioned in a message to webkit-dev, my recent  
> change to the behaviour of KJS_CHECK_EXCEPTION resulted in outer  
> nodes overwriting the line numbers set by inner nodes.  I have  
> attached a patch that checks that the line number and source URL do  
> not exist before setting these attributes on the exception object.

Looks great!

Three comments:

     1) These changes need tests. It should be straightforward to  
make tests that throw exceptions and then have exception handlers  
that check the file and line number on the caught exceptions. We  
really want to write tests whenever we fix a bug unless it's not  
possible.

     2) This macro getting bigger and it's used in a lot of places.  
Since the new code is all for the exception case, which is not  
particularly speed-critical, I'd like to see as much as possible of  
the code put into a protected member function of the KJS::Node class,  
and called from the macro.

     3) KJS_CHECKEXCEPTION, KJS_CHECKEXCEPTIONREFERENCE, and  
KJS_CHECKEXCEPTIONLIST all need the same fix.

Thanks very much for tackling this important bug. I don't mean to be  
a pain with all these review comments; keep it up!

     -- Darin