Bug 15334 – Split ResourceResponse into platform specific files

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Julien Chaffraix]

ResourceRequest was split into platform specific files (
http://bugs.webkit.org/show_bug.cgi?id=14225). The idea is to have the same with ResourceResponse.

The following patch uses this bug as a guideline to use the same pattern with ResourceResponse.

[Julien Chaffraix]

Created attachment 16487 [details]
First draft

The patch should cover all platforms but has only been tested on Linux (both qt and gdk).

The patch also misses the entries in WebCore.xcodeproj/project.pbxproj as I do not have a mac with Xcode on which generating those entries.

[Eric Seidel (no email)]

Hum... I didn't look very closely, but normally we use Foo instead of FooBase for base class names, and then each platform has its own implementation, like FooMac or FooWin, etc.  But perhaps that won't work for this case... again I didn't look closely.

[Dave Hyatt]

If you look at other examples in platform, we usually retain a single cross-platform header and then just use ifdefs in that header for platform-specific methods.  This way we don't have a proliferation of platform-specific header files.

We make exceptions sometimes but usually only for classes for which the bulk of the implementation (nearly all) is platform-specific.

[Dave Hyatt]

I am not sure why ResourceRequest was split up the way it was.  It doesn't really follow our convention.  Maybe we are moving to this model.  I'll defer to mjs, since I'm kind of confused now.

[Darin Adler]

Comment on attachment 16487 [details]
First draft

This patch would be easier to review if you did an "svn move" of ResourceResponse.cpp to ResourceResponseBase.cpp -- the patch would then show only diffs, rather than showing the entire file as changed and we also wouldn't lose history on the files. svn-create-patch does a good job of handling cases like that.

+    void doUpdateResourceResponse() {
+        notImplemented();
+    }

We put braces on the second line, not the first.

+    const_cast<ResourceResponse&>(asResourceResponse()).doUpdateResourceResponse();

Why use const_cast here? Could we instead make the relevant members all be mutable? I'm always uncomfortable with the "do" prefix in cases like this. Perhaps we could come up with a more descriptive name for the function that does the actual work.

+    //Used when response is initialized from a platform representation

We normally put a space after the "//".

We normally don't put a blank line after "protected:" etc.

I'm going to mark this review- because I'd like to instead see a version of the patch that does "svn move".

[Darin Adler]

Oh, and also we need someone with Macintosh to do the Xcode project updates too; can't do review+ until the patch has that. So someone will have to help Julien.

[Julien Chaffraix]

Created attachment 16707 [details]
Patch updated with Darin's comment

The new patch has been generated with "svn move" and "svn-create-patch". Thanks for the tip.

Again the patch has been tested only on linux.

>+
>const_cast<ResourceResponse&>(asResourceResponse()).doUpdateResourceResponse();

> Why use const_cast here? Could we instead make the relevant members all be
> mutable?

After a quick look at the code, adding mutable to the members of  ResourceResponseBase would leave only one member non-mutable so I do not think that is worth it.

I agree that the 2 casts are not really beautiful. I have tried a lot of alternatives and the conclusion is that we need the 2 casts.

> I'm always uncomfortable with the "do" prefix in cases like this.
> Perhaps we could come up with a more descriptive name for the function that
> does the actual work.

I could not find a better name. Maybe someone has a better idea.

[Darin Adler]

Comment on attachment 16707 [details]
Patch updated with Darin's comment

Looks fine, r=me.

[Julien Chaffraix]

Comment on attachment 16707 [details]
Patch updated with Darin's comment

Forgot to remove the review+ flag. The patch should include Xcode information and be tested on mac.

[Julien Chaffraix]

Created attachment 17172 [details]
Patch updated with XCode information and tested on Mac

Finally manage to complete the Mac part. I had to change some part of it so set the review flag to ? again. Tested on Mac & Linux (both platform).

[Darin Adler]

Comment on attachment 17172 [details]
Patch updated with XCode information and tested on Mac

r=me

[Mark Rowe (bdash)]

Landed in r27714.