Bug 39156 – FrameLoader: refactor FrameLoader::createWindow() to be a non-member, non-friend function

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Jerdonek]

The FrameLoader class shouldn't be responsible for creating windows.  Moving createWindow() out of the FrameLoader class is a good first step.  This will also increase encapsulation by reducing the number of public functions with access to FrameLoader's private data.

[Chris Jerdonek]

Created attachment 56183 [details]
Proposed patch

Note that this will probably not build/apply until the patch for bug 39157 lands.

[Chris Jerdonek]

Created attachment 56184 [details]
Proposed patch (re-attaching newest)

As before, this will not apply.

[Adam Barth]

Comment on attachment 56184 [details]
Proposed patch (re-attaching newest)

WebCore/loader/FrameLoader.cpp:3966
 +  Frame* createWindowFromFrameLoader(FrameLoader* sourceFrameLoader, FrameLoader* frameLoaderForFrameLookup, const FrameLoadRequest& request, const WindowFeatures& features, bool& created)
I think sourceFrameLoader should be openerFrameLoader because that's the Frame that ends up being the opener.

It might make more sense to pass in the Frame as "opener" and then get the outgoing referrer, etc, from opener->loader()->...

The reason for do that is the Frame is the root object of a who constellation of objects, one of which is the FrameLoader.  We usually use the Frame as object to keep track of the context, not the FrameLoader.

createWindowFromFrameLoader also isn't a great name.  Maybe createWindow?

I'm not sure this should be a free function.  It feels like it should be a member of something...

[Darin Adler]

Comment on attachment 56184 [details]
Proposed patch (re-attaching newest)

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.

[Darin Adler]

Looks like Adam and I made quite similar comments. I think it would make sense for this to be the member of something, maybe a static member, that should be responsible for creating windows. But I think it's OK as a free function too.

[Chris Jerdonek]

(In reply to comment #3)
> (From update of attachment 56184 [details])
> WebCore/loader/FrameLoader.cpp:3966
>  +  Frame* createWindowFromFrameLoader(FrameLoader* sourceFrameLoader, FrameLoader* 
> ...
> I'm not sure this should be a free function.  It feels like it should be a member of something...

(In reply to comment #4)
> I also wonder if FrameLoader.h/cpp is the right place for this.

(In reply to comment #5)
> Looks like Adam and I made quite similar comments. I think it would make sense for this to be the member of something, maybe a static member, that should be responsible for creating windows. But I think it's OK as a free function too.

Thanks, guys, for the helpful comments.  With respect to making FrameLoader::createWindow() a free function, the intent was not for it to stay that way.

The intent was to be incremental and first separate the function from the class.  As we continue refactoring and grow to see the right abstractions, we can later move this to an appropriate class/location.  In the meantime, hopefully separating it out will make it slightly easier to think about FrameLoader.

[Chris Jerdonek]

Created attachment 56426 [details]
Proposed patch 2

Addressing your comments.

(In reply to comment #3)
> I'm not sure this should be a free function.  It feels like it should be a member of something...

(In reply to comment #4)
> I also wonder if FrameLoader.h/cpp is the right place for this.

(In reply to comment #5)
> I think it would make sense for this to be the member of something, maybe a static member, that should be responsible for creating windows. But I think it's OK as a free function too.

I also added a FIXME to move this to a more appropriate location/class per my comment 6.

[WebKit Review Bot]

Attachment 56426 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2304281

[Chris Jerdonek]

Created attachment 56455 [details]
Proposed patch 3

Qualified the call to createWindow() in BindingDOMWindow.h.

The compiler was interpreting the call to createWindow() in BindingDOMWindow.h as a dependent name because it has the same name as BindingDOMWindow<Binding>::createWindow() -- even though the signature is different.

[Chris Jerdonek]

Darin or Adam, can one of you review the newest patch that addresses both or your comments?

Adam, if createWindow() being a free function is a blocking issue for you, can you confirm that you thought the Page class is an appropriate location?  I've added a FIXME to move it, so we can always do that later.

[Adam Barth]

Comment on attachment 56455 [details]
Proposed patch 3

WebCore/bindings/generic/BindingDOMWindow.h:93
 +      Frame* newFrame = WebCore::createWindow(callingFrame, openerFrame, frameRequest, windowFeatures, created);
Do we need the WebCore:: prefix here?

[Adam Barth]

Comment on attachment 56455 [details]
Proposed patch 3

Turns out we do because otherwise we'll call the member function.

[WebKit Commit Bot]

Comment on attachment 56455 [details]
Proposed patch 3

Clearing flags on attachment: 56455

Committed r65164: <http://trac.webkit.org/changeset/65164>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.