[webkit-reviews] Patch to fix initialization issues in the WebView subproj

[Darin Adler]

On Jun 12, 2005, at 12:14 PM, Nick Zitzmann wrote:

> When I first downloaded WebKit, I noticed there were a number of  
> classes in the WebView subproject that either:
> 1. did not properly initialize the superclass,

This sounds like an important issue. Where did you discover this?  
It's hard for me to find these cases inside your patch. In fact, I  
couldn't find a single instance of this!

> 2. did not set self to the return value of the superclass, or

I don't think this is important.

While it's true that classes can use the technique where they return  
a different value from init, it's not necessary to always code as if  
your superclass does this. The vast majority of classes don't do  
this, and never will, and writing all the code as if they do doesn't  
provide a practical improvement.

On the other hand, I think there's not great harm in doing this  
either; it won't really make the code bigger and it's nice to be more  
consistent.

> 3. did not check to see if the superclass initializer actually  
> returned anything, or

Again, just because some classes can fail initialization and return  
nil doesn't mean that every class can do this. There are many classes  
that simply do not have a failure case, and adding code to check in  
those cases doesn't necessarily enhance things.

By adding error handling for a condition that is never true, this  
creates code paths that can never be exercised. So I think that  
arguably it does not make the code better.

Maybe others on the project will disagree, though. It's definitely  
more elegant to be consistent, so I guess on that basis we should  
probably do the change. But I worry about a structural change like  
this that's not easy to test, a change that adds code that is never  
executed, and a change that adds code but doesn't change the  
observable behavior of the code in any way.

> The attached patch fixes this for the classes in the WebView  
> subproject.

The patch does not follow the style guidelines. All the if statements  
look like this:

     if (self)
     {

but the style guidelines require

     if (self) {

I'd like to see a patch that follows the style guidelines in this  
respect.

I like the suggestion of using:

     if (!self)
         return nil;

It would cause fewer changes and make it easier to merge patches with  
this change. I'd like to see a patch that does it this way.

     -- Darin