[webkit-reviews] review denied: [Bug 15334] Split ResourceResponse into platform specific files : [Attachment 16487] First draft

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Julien Chaffraix
<julien.chaffraix at gmail.com>'s request for review:
Bug 15334: Split ResourceResponse into platform specific files
http://bugs.webkit.org/show_bug.cgi?id=15334

Attachment 16487: First draft
http://bugs.webkit.org/attachment.cgi?id=16487&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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".