Bug 17210 – Eliminate platform-specific #ifdefs in ResourceError.h and AuthenticationChallenge.h

• Component: Platform
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

Eliminate platform-specific #ifdefs in ResourceError.h and AuthenticationChallenge.h

[Darin Fisher (:fishd, Google)]

Created attachment 18986 [details]
patch v1

This patch introduces ResourceErrorBase and AuthenticationChallengeBase, similar to what was done for ResourceResponse and ResourceRequest.

Some things to note about this patch:

1- Generic implementations of AuthenticationChallenge, ResourceError, ResourceRequest, and ResourceResponse are now in a newly created network/generic directory.  Ports that do not need to customize these classes can just include the generic ones.  There does not seem to be any convention for this in WebKit, so I'm not sure how this will be received.  Please advise if you prefer another way.

2- The global operator== defined for many of these types is now implemented by calling a compare function defined on the *Base class.  That compare function then calls a platformCompare function to allow the platform to implement the equality test for platform-specific fields.  I chose the name "compare" after seeing it used in Vector.h.  I applied this technique to the operator== implemented for ResourceResponse, which frees ResourceResponseBase.cpp of a platform-specific #ifdef.

3- Many of these classes require lazy initialization on Mac and CF ports.  I came up with a fairly uniform way of dealing with this.  Each method that may need to perform lazy initialization, now calls a lazyInit function defined on the *Base class.  That function simply downcasts to the non-Base class and calls a platformLazyInit function.  The *Base class also defines platformLazyInit to do nothing.  That way, if a port does not define platformLazyInit, then all of the lazyInit calls should be simply eliminated by the compiler.  This takes advantage of how method names are shadowed by subclasses in C++.

I've tried to be fairly methodical with this patch.  Hopefully once you review one of the classes, the changes to the second will be obvious and also straightforward to review.

[Darin Adler]

(In reply to comment #1)
> 1- Generic implementations of AuthenticationChallenge, ResourceError,
> ResourceRequest, and ResourceResponse are now in a newly created
> network/generic directory.  Ports that do not need to customize these classes
> can just include the generic ones.  There does not seem to be any convention
> for this in WebKit, so I'm not sure how this will be received.  Please advise
> if you prefer another way.

I think it is fine to have these files, but they should be in the top level network directory, not a new one named "generic". I don't think "generic" makes the purpose of the files clearer. Comments in the files should explain when you'd use them.

[Darin Fisher (:fishd, Google)]

Well, since not every platform is going to use the generic versions of ResourcError.h, etc., it seems like it would be confusing to have them in the common base directory.  I thought it might be wierd to have a ResourceError.h in platform/network/ as well as in platform/network/mac/.  That doesn't seem awkward (potentially confusing) to you?

[Darin Adler]

(In reply to comment #3)
> Well, since not every platform is going to use the generic versions of
> ResourcError.h, etc., it seems like it would be confusing to have them in the
> common base directory.  I thought it might be wierd to have a ResourceError.h
> in platform/network/ as well as in platform/network/mac/.  That doesn't seem
> awkward (potentially confusing) to you?

Sure, that'd be more than awkward; it wouldn't even work with our build system.

I guess I didn't realize that your designs involved same named files in different directories. The old design went out of its way to avoid that; I see now that's the tradeoff here.

I'm still not sure "generic" is the right name for these. I'd like to understand what the quality would be that would allow a platform to use the base versions of the classes without adding anything.

I think Anders Carlsson should probably review this patch. I'll cc him on this bug so he'll notice it.

[Darin Fisher (:fishd, Google)]

I'm happy to kill the generic directory if that would be preferred.  I just thought it would be useful for new ports (sort of a starting point for a port).  The qt and curl ports just happen to be able to use most of the files in the generic folder.

[Robert Blaut]

P5 is not used for WebKit bugs. [http://webkit.org/quality/bugpriorities.html]

[Darin Fisher (:fishd, Google)]

Created attachment 19285 [details]
patch v2

OK, here's a version of the patch sans-generic folder.

[Darin Adler]

Comment on attachment 19285 [details]
patch v2

One thing I don't like about this approach is that we end up with so many identically named files in different directories. But it looks like you executed the design just fine.

r=me

[Mark Rowe (bdash)]

This patch does not apply cleanly to SVN as of r30564 due to conflicts in WebCore.vcproj and WebCore.xcodeproj.  Can you please regenerate this patch against SVN HEAD and attach it so that it can be landed?

[Darin Fisher (:fishd, Google)]

> One thing I don't like about this approach is that we end up with so many
> identically named files in different directories. But it looks like you
> executed the design just fine.

Hi Darin.  I plan on contributing more patches like this, so please let me know if you have a more preferred design that I should apply instead.  Thanks!

[Darin Adler]

(In reply to comment #10)
> > One thing I don't like about this approach is that we end up with so many
> > identically named files in different directories. But it looks like you
> > executed the design just fine.
> 
> Hi Darin.  I plan on contributing more patches like this, so please let me know
> if you have a more preferred design that I should apply instead. Thanks!

I'll let you know if I think of something.

[Darin Fisher (:fishd, Google)]

Created attachment 19367 [details]
patch v3

OK, here's the merged version.

[Mark Rowe (bdash)]

Landed in r30584.