[webkit-reviews] review denied: [Bug 37874] Provide mechanism to cache metadata for a resource : [Attachment 54091] Fix compile by overloading rather than specializing in WebData.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Tony Gentilcore
<tonyg at chromium.org>'s request for review:
Bug 37874: Provide mechanism to cache metadata for a resource
https://bugs.webkit.org/show_bug.cgi?id=37874

Attachment 54091: Fix compile by overloading rather than specializing in
WebData.
https://bugs.webkit.org/attachment.cgi?id=54091&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
WebCore/loader/CachedResource.cpp:168
 +  void CachedResource::setCachedMetadata(CacheableMetadata::Type type,
PassRefPtr<SharedBuffer> data, FrameLoader* frameLoader)
Why pass a FrameLoader* here?  Usually we pass around Frame*.

WebCore/loader/CachedResource.cpp:175
 +	if (frameLoader && frameLoader->client())
client() is always non-NULL.

WebCore/loader/CachedResource.h:148
 +	void setCachedMetadata(CacheableMetadata::Type,
PassRefPtr<SharedBuffer>, FrameLoader*);
Does CachedResource have any other dependencies on Frame?  In general, we want
the Frame to be less involved in loading, not more.

WebCore/loader/FrameLoaderClient.h:47
 +	struct CacheableMetadata;
We prefer classes to structs.

WebCore/platform/network/ResourceResponseBase.h:48
 +	long long m_id;
This whole thing is very strangely layered.  In WebKit, there's no necessary
connection between the client and the networking layer.  This patch somehow
assumes some strange round-tripping between the two.

Why is the client involved in this process at all?  It seems like WebCore
should just ask the networking layer to cache these bytes.

WebCore/platform/network/ResourceResponseBase.h:52
 +	    V8_SCRIPTDATA
Code in this location isn't allowed to know about V8.  We need a better way of
assigning these type numbers.  Also, are these number persisted on disk?  We
might need some comments explaining the rules for modifying these bits.