[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