Bug 16588

Summary: ResourceHandle::loadResourceSynchronously needs access to the FrameLoader
Product: WebKit Reporter: Holger Freyther <zecke>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, darin, hausmann, mjs
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.4   
Attachments:
Description Flags
Add FrameLoader* as parameter to the function
darin: review-
add WebCore::Frame * as parameter to loadResourceSynchronously lars.knoll: review+

Description Holger Freyther 2007-12-23 15:09:42 PST
With the Qt port/platform a QWebPage can have its own QNetworkAccessManager set. We want all network access for a page/frame to go through this QNetworkAccessManager. For the static ResourceHandle::loadResourceSynchronously function we have currently no way to get the QNetworkAccessManager of the QWebPage. All the current callers of this function do have the necessary information though.

What we did/propose is to add the FrameLoader* as last parameter to the above named function. With the chain of FrameLoader->FrameLoaderClient->QWebFrame we can access the data we need.
Comment 1 Holger Freyther 2007-12-23 15:12:09 PST
Created attachment 18077 [details]
Add FrameLoader* as parameter to the function

We want to add the FrameLoader* to the loadResourceSynchronously function. This patch adds it and is updating the other ports (or will once landed to trunk).
Comment 2 Darin Adler 2007-12-29 22:45:33 PST
I suppose this is OK, but it's really a layering violation. We want to build the FrameLoader on top of the ResourceHandle, without a circular dependency.

I'll see if I can think of something better.
Comment 3 Darin Adler 2007-12-30 23:48:24 PST
Comment on attachment 18077 [details]
Add FrameLoader* as parameter to the function

If you are going to add a parameter, I suggest adding a Frame* rather than a FrameLoader* to match ResourceHandle::create.

But since this is a layering violation, we should figure out a cleaner way to do this.

Perhaps we should have a ResourceHandleFactory class and FrameLoaderClient can return a ResourceHandleFactory. Then you'd use the ResourceHandleFactory to make the ResourceHandle objects.

Or we could have a class with a name like ResourceLoadingContext, add a FrameLoaderClient function that returns one, and we could pass that in to the static members of ResourceHandle.

A typical client could implement both interfaces on the FrameLoaderClient and it could just return "this". This might be enough allow us to remove the Frame* parameter from both functions and remove the circular dependency.
Comment 4 Simon Hausmann 2008-01-07 06:02:07 PST
Created attachment 18311 [details]
add WebCore::Frame * as parameter to loadResourceSynchronously

Revised patch that uses Frame * instead of FrameLoader * as last argument.

I agree about the use of Frame * instead of FrameLoader. For some reason I didn't see that the ResourceHandle constructor was using a Frame *, too.
Comment 5 Lars Knoll 2008-01-10 06:36:52 PST
Comment on attachment 18311 [details]
add WebCore::Frame * as parameter to loadResourceSynchronously

Since ResourceHandle::create() also has a pointer to the Frame, I see no reason why loadResourceSynchronously should be any different.
Comment 6 Simon Hausmann 2008-01-10 07:48:59 PST
Landed in r29352
Comment 7 Darin Adler 2008-01-10 09:14:46 PST
(In reply to comment #5)
> Since ResourceHandle::create() also has a pointer to the Frame, I see no reason
> why loadResourceSynchronously should be any different.

The Frame* in ResourceHandle::create() *is* the layering violation. I agree that adding it to the synchronous form doesn't make things any worse, but our goal is to eliminate this in the longer run and come up with a better way to create the resource handles with the right context that allows it to be a lower-level I/O subsystem rather than yet another part of the massive super-object Frame.