[webkit-reviews] review granted: [Bug 30379] Factor ResourceLoadNotifier out of FrameLoader : [Attachment 41211] Patch v1
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 15 09:23:51 PDT 2009
Darin Adler <darin at apple.com> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 30379: Factor ResourceLoadNotifier out of FrameLoader
https://bugs.webkit.org/show_bug.cgi?id=30379
Attachment 41211: Patch v1
https://bugs.webkit.org/attachment.cgi?id=41211&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
> void FrameLoader::sendRemainingDelegateMessages(unsigned long identifier,
const ResourceResponse& response, int length, const ResourceError& error)
I'm surprised you left this function in FrameLoader. Seems to me it belongs in
ResourceLoadNotifier, although you might have to add a document loader
argument.
> @@ -3526,11 +3487,11 @@ void
FrameLoader::requestFromDelegate(ResourceRequest& request, unsigned long& i
This function also seems to belong in ResourceLoadNotifier. I didn't look
closely at other functions outside the patch.
> +void ResourceLoadNotifier::assignIdentifierToInitialRequest(unsigned long
identifier, const ResourceRequest& clientRequest)
> +{
> + dispatchAssignIdentifierToInitialRequest(identifier,
activeDocumentLoader(), clientRequest);
> +}
We may want to consider passing the document loader here instead of having the
notifier get the active document loader.
> +void ResourceLoadNotifier::didReceiveResponse(ResourceLoader* loader, const
ResourceResponse& r)
> +{
> + activeDocumentLoader()->addResponse(r);
> +
> + if (Page* page = m_frame->page())
> + page->progress()->incrementProgress(loader->identifier(), r);
> +
> + dispatchDidReceiveResponse(loader->documentLoader(),
loader->identifier(), r);
> +}
Here too, and then we could eliminate the activeDocumentLoader() function.
r=me
More information about the webkit-reviews
mailing list