[webkit-reviews] Patch to fix initialization issues in the
WebView subproj
Darin Adler
darin at apple.com
Sun Jun 12 15:38:50 PDT 2005
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
More information about the webkit-reviews
mailing list