[webkit-reviews] review granted: [Bug 61318] Make CachedResource take a ResourceRequest instead of just a url string. : [Attachment 94513] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon May 23 16:36:46 PDT 2011
Adam Barth <abarth at webkit.org> has granted Nate Chapin <japhet at chromium.org>'s
request for review:
Bug 61318: Make CachedResource take a ResourceRequest instead of just a url
string.
https://bugs.webkit.org/show_bug.cgi?id=61318
Attachment 94513: patch
https://bugs.webkit.org/attachment.cgi?id=94513&action=review
------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=94513&action=review
I really like this direction. Some minor questions / comments below.
> Source/WebCore/loader/cache/CachedResourceLoader.cpp:135
> - KURL completeURL = m_document->completeURL(url);
> + KURL completeURL = m_document->completeURL(request.url());
Presumably request.url() is already complete, right?
> Source/WebCore/loader/cache/CachedResourceLoader.cpp:341
> + if (request.url() != url)
> + request.setURL(url);
This basically has the effect of stripping the fragment identifier, right?
> Source/WebCore/loader/cache/CachedResourceLoader.cpp:-373
> - CachedResource* newResource = createResource(resource->type(),
KURL(ParsedURLString, url), resource->encoding());
Yay. Every time you remove ParsedURLString, good things happen.
> Source/WebCore/loader/cache/CachedResourceLoader.cpp:683
> - CachedResource* resource = requestResource(type, url, encoding,
ResourceLoadPriorityUnresolved, true);
> + ResourceRequest request(m_document->completeURL(url));
> + CachedResource* resource = requestResource(type, request, encoding,
ResourceLoadPriorityUnresolved, true);
Here the caller isn't supposed to provide a ResourceRequest?
> Source/WebCore/loader/cache/CachedImage.cpp:228
> - m_shouldPaintBrokenImage =
frame->loader()->client()->shouldPaintBrokenImage(KURL(ParsedURLString,
m_url));
> + m_shouldPaintBrokenImage =
frame->loader()->client()->shouldPaintBrokenImage(KURL(ParsedURLString,
m_resourceRequest.url()));
Can't we get rid of the ParsedURLString here? m_resourceRequest.url() should
already be a KURL, right?
> Source/WebCore/loader/cache/CachedResourceLoader.h:45
> +class CachedRawResource;
CachedRawResource <-- That's from the future, right?
> Source/WebCore/loader/cache/CachedResource.cpp:268
> +static bool reuseRequest(const ResourceRequest& request)
reuseRequest => canReuseRequest ?
> Source/WebCore/loader/cache/CachedResource.cpp:281
> +// FIXME: This doesn't handle Access-Control headers yet, but when it does,
it will need the
> +// currently unused Document* to get a SecurityOrigin.
> +bool CachedResource::allowReuseOfRequest(Document*, const ResourceRequest&
newRequest) const
I don't fully understand what this function does. It seems like these cases
can't occur yet, right?
> Source/WebCore/loader/cache/CachedResource.h:99
> + const String& url() const { return m_resourceRequest.url();}
Oh, I see. This returns a string. Can we return the underlying KURL from
m_resourceRequest instead?
More information about the webkit-reviews
mailing list