[webkit-reviews] review denied: [Bug 39156] FrameLoader: refactor FrameLoader::createWindow() to be a non-member, non-friend function : [Attachment 56184] Proposed patch (re-attaching newest)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Chris Jerdonek
<cjerdonek at webkit.org>'s request for review:
Bug 39156: FrameLoader: refactor FrameLoader::createWindow() to be a
non-member, non-friend function
https://bugs.webkit.org/show_bug.cgi?id=39156

Attachment 56184: Proposed patch (re-attaching newest)
https://bugs.webkit.org/attachment.cgi?id=56184&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
I don't think the name createWindowFromFrameLoader is good. These names that
mention the arguments don't usually work well in C++. Further, we almost always
pass Frame* rather than FrameLoader* to functions, as a part of the general
pattern of the Frame family of classes, so I suggest that first two arguments
both be Frame*, not FrameLoader*.

The name createWindow seems OK to me, except for the fact that the
non-web-browser meaning of window in computer operating systems competes a
little bit with the misguided use of window to mean the per-frame global
controller object, "window".

I also wonder if FrameLoader.h/cpp is the right place for this.

I think I'd say review+ if this was named createWindow and took two Frame*.
It's OK that the function in JSDOMWindowCustom is also called createWindow
because the arguments are different.

review- mainly because of the use of FrameLoader* as the argument type.