Bug 14225 – Make it possible to define platform-specific ResourceRequest without #ifdefs

• Component: Platform
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

Make it possible to define platform-specific ResourceRequest without #ifdefs

Move ResourceRequest into a platform-specific directory, and have it extend from a BaseResourceRequest that carries the bulk of the code for the ResourceRequest.

See patch.

[Darin Fisher (:fishd, Google)]

Created attachment 15112 [details]
partial patch (mac only)

OK, this patch does the trick for PLATFORM(MAC).  If this approach is acceptable, then I will do the same for the USE(CFNETWORK).

[Darin Fisher (:fishd, Google)]

I thought about this patch some more, and perhaps it would be better to declare doUpdate* methods as virtual on the base class.  (This is somewhat of a textbook example of when you should use virtual functions.)  The cost of doing so would not be measurable in the product since we rarely call those methods, and moreover the work they do far outweighs the cost of the virtual functions.  If desired, I will make that modification.

Maciej asked in #webkit about finding a good pattern to apply universally to classes in the porting layer.  The two choices being virtual functions or downcasting.  I think that we'll have a better idea once more classes are converted.  In many cases, virtual functions would be more than appropriate, but we'll have to see.

[Darin Fisher (:fishd, Google)]

OK, I discussed this more with Maciej in #webkit, and we agreed on the following:

1. Name things FooBase instead of BaseFoo.  Hyatt was also happier with this.

2. Go with the downcasting approach, but invent a method for the downcast operation.  In this case, it would be something like this:

  class ResourceRequestBase
  {
      ...
  private:
      ResourceRequest& asResourceRequest() {
          return *static_cast<ResourceRequest*>(this);
      }
      const ResourceRequest& asResourceRequest() const {
          return *static_cast<const ResourceRequest*>(this);       
      }
  };

This naming convention is nice because it addresses situations where we have multiple FooBase classes in a heirarchy.

[Darin Fisher (:fishd, Google)]

Created attachment 15133 [details]
partial patch v2 (mac only)

OK, this implements the revised approach:
asResourceRequest() + ResourceRequestBase

[Darin Adler]

Comment on attachment 15133 [details]
partial patch v2 (mac only)

I'm a little concerned about having files with identical names but different contents in the various platform-specific subdirectories. Up until this point we've mostly avoided this, and it helps with build systems that have trouble understanding identically named source files. But I don't see an elegant alternative, so I guess we should go with it.

+#if PLATFORM(MAC)
+#include <wtf/RetainPtr.h>
+#ifdef __OBJC__
+@class NSURLRequest;
+#else
+class NSURLRequest;
+#endif
+#endif

I don't think that ResourceRequest.h in the mac subdirectory needs an #if PLATFORM(MAC). So you can just remove the ifdef.

+#include "config.h"
+#include "ResourceRequest.h"

I think it would be slightly better to explicitly include first "ResourceRequestBase.h" and then "ResourceRequest.h", even though that amounts to the same thing as just including "ResourceRequest.h". I want to take our "always includes your own header first" rule literally in cases like this.

+    const_cast<ResourceRequest&>(asResourceRequest()).doUpdatePlatformRequest();

I believe this won't compile. It's not just a  const_cast, it's also a downcast, so it needs to be a combination of const_cast and static_cast or a C-style cast. Did you try compiling this?

I'd slightly prefer it if we could come up with a way to do the downcast to ResourceRequest without a static_cast at each call site. Maybe an inline helper function would make the syntax a little sweeter?

[Darin Adler]

Comment on attachment 15133 [details]
partial patch v2 (mac only)

I'm a little concerned about having files with identical names but different contents in the various platform-specific subdirectories. Up until this point we've mostly avoided this, and it helps with build systems that have trouble understanding identically named source files. But I don't see an elegant alternative, so I guess we should go with it.

+#if PLATFORM(MAC)
+#include <wtf/RetainPtr.h>
+#ifdef __OBJC__
+@class NSURLRequest;
+#else
+class NSURLRequest;
+#endif
+#endif

I don't think that ResourceRequest.h in the mac subdirectory needs an #if PLATFORM(MAC). So you can just remove the ifdef.

+#include "config.h"
+#include "ResourceRequest.h"

I think it would be slightly better to explicitly include first "ResourceRequestBase.h" and then "ResourceRequest.h", even though that amounts to the same thing as just including "ResourceRequest.h". I want to take our "always includes your own header first" rule literally in cases like this.

+    const_cast<ResourceRequest&>(asResourceRequest()).doUpdatePlatformRequest();

I believe this won't compile. It's not just a  const_cast, it's also a downcast, so it needs to be a combination of const_cast and static_cast or a C-style cast. Did you try compiling this?

I'd slightly prefer it if we could come up with a way to do the downcast to ResourceRequest without a static_cast at each call site. Maybe an inline helper function would make the syntax a little sweeter?

[Darin Fisher (:fishd, Google)]

> +#if PLATFORM(MAC)
> +#include <wtf/RetainPtr.h>
> +#ifdef __OBJC__
> +@class NSURLRequest;
> +#else
> +class NSURLRequest;
> +#endif
> +#endif
> 
> I don't think that ResourceRequest.h in the mac subdirectory needs an #if
> PLATFORM(MAC). So you can just remove the ifdef.

Oops, yes.  That is spurious.  I will remove it.


> +#include "config.h"
> +#include "ResourceRequest.h"
> 
> I think it would be slightly better to explicitly include first
> "ResourceRequestBase.h" and then "ResourceRequest.h", even though that amounts
> to the same thing as just including "ResourceRequest.h". I want to take our
> "always includes your own header first" rule literally in cases like this.

OK, sure thing.


> +   
> const_cast<ResourceRequest&>(asResourceRequest()).doUpdatePlatformRequest();
> 
> I believe this won't compile. It's not just a  const_cast, it's also a
> downcast, so it needs to be a combination of const_cast and static_cast or a
> C-style cast. Did you try compiling this?

Yes, I compiled and Safari runs fine with this code.  I think you should look at the implementation of asResourceRequest(), which performs the downcast operation.  The const_cast is needed to call a non-const method on the ResourceRequest.


> I'd slightly prefer it if we could come up with a way to do the downcast to
> ResourceRequest without a static_cast at each call site. Maybe an inline helper
> function would make the syntax a little sweeter?

That would be asResourceRequest :-)  It is not inline however because I would have to define it after the #include of ResourceRequest.h, which would be messy.  I believe that any good compiler/linker will optimize away the function.

[Darin Fisher (:fishd, Google)]

Created attachment 15187 [details]
partial patch v2.1 (mac only)

OK, revised according to review feedback.  Please let me know if this is an OK approach, and then I will contribute a CFNet version as well.  Thanks!

[Maciej Stachowiak]

Comment on attachment 15187 [details]
partial patch v2.1 (mac only)

Looks good to me. We need at least a CFNetwork version as well to land this (assuming it breaks the ResourceRequest build for other platforms). And I think it would be good to convert the Qt and curl versions as well, to avoid breaking the build for the Qt and Gtk ports. Otherwise looks great. Please submit a new version that covers other platforms.

[Darin Fisher (:fishd, Google)]

Created attachment 15240 [details]
complete patch

OK, this covers all of the major network platforms: mac, cf, qt, and curl.

[Darin Fisher (:fishd, Google)]

Created attachment 15241 [details]
complete patch (that actually works!)

was a link error in the previous patch.  use this one instead.

[Maciej Stachowiak]

Comment on attachment 15241 [details]
complete patch (that actually works!)

Looks ok, please try to get someone to land this at a time when it is easy to follow the build bots and land appropriate follow-up fixes.

r=me

[Mark Rowe (bdash)]

Landed in r23806.

[Simon Fraser (smfr)]

> (In reply to comment #5)
> (From update of attachment 15133 [details])
> I'm a little concerned about having files with identical names but different contents in the various platform-specific subdirectories.

This exact issue was the cause of the recent WebSocket memory smasher (Mac Xcode project picked up the qt header).

[Darin Fisher (:fishd, Google)]

(In reply to comment #14)
> > (In reply to comment #5)
> > (From update of attachment 15133 [details] [details])
> > I'm a little concerned about having files with identical names but different contents in the various platform-specific subdirectories.
> 
> This exact issue was the cause of the recent WebSocket memory smasher (Mac Xcode project picked up the qt header).

How was it that the Xcode build was including headers from a different port?  How likely is that problem to repeat?

Perhaps the ResourceRequest.h headers should all have PLATFORM-specific guards so that if PLATFORM(QT) is not defined then ResourceRequest would also not be defined.  That would turn this kind of mistake into a compile time error.