[webkit-reviews] review granted: [Bug 101935] Add initiator to CachedResourceRequest. : [Attachment 173900] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Nov 13 09:27:02 PST 2012


Adam Barth <abarth at webkit.org> has granted  review:
Bug 101935: Add initiator to CachedResourceRequest.
https://bugs.webkit.org/show_bug.cgi?id=101935

Attachment 173900: Patch
https://bugs.webkit.org/attachment.cgi?id=173900&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=173900&action=review


This looks good.  Some nits below.  My main concern here is about maintenance. 
We want to make sure this code is well-tested so that the initiator doesn't
become copy pasta.  I appreciate that you've broken this work up into small
patches, but I would like to get to the point where we can test these changes
sooner rather than later.  :)

> Source/WebCore/ChangeLog:5
> +

You should leave the Reviewed by NODBODY (OPPS!) line in the ChangeLog entry. 
The bots use that to know where to insert the name of the person who actually
reviewed the patch.

> Source/WebCore/css/CSSCursorImageValue.cpp:142
> -    return CSSImageValue::cachedImage(loader, url());
> +    return CSSImageValue::cachedImage(loader, url(), 0);

In these cases, we won't have initiators?  Is that a problem?

> Source/WebCore/css/CSSImageValue.cpp:92
> +	       request.setInitiator(cachedResourceRequestInitiators().css,
loader->document());

Oh, I see.  It defaults to the document.  In that case, I might add a default
argument of 0 so that the callers don't have this funny 0 hanging around.

> Source/WebCore/css/WebKitCSSSVGDocumentValue.cpp:57
> +	   request.setInitiator("css", loader->document());

"css" -> cachedResourceRequestInitiators().css

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:73
> +void CachedResourceRequest::setInitiator(AtomicString name,
PassRefPtr<Document> document)

AtomicString -> const AtomicString& (that avoid churning the reference count)

> Source/WebCore/platform/ThreadGlobalData.cpp:32
> +#include "CachedResourceRequestInitiators.h"
>  #include "DOMImplementation.h"
>  #include "EventNames.h"

Technically these are all layering violations, but I'm not sure we have a plan
for solving this problem without this layering violation.


More information about the webkit-reviews mailing list