[webkit-reviews] review denied: [Bug 37874] Provide mechanism to cache metadata for a resource : [Attachment 55079] Get rid of map and fix smart pointers

[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 55079: Get rid of map and fix smart pointers
https://bugs.webkit.org/attachment.cgi?id=55079&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
This is getting close.	I'd like to see this code used by something so it's not
dead code though.  :)

WebCore/loader/CachedMetadataStore.h:108
 +	static const size_t unsignedSize = sizeof(unsigned);
This constant makes things harder to read.  Just use sizeof(unsigned).	That's
perfectly clear.

WebCore/loader/CachedMetadataStore.h:107
 +	static const size_t dataTypeIDStart = 0;
This constant also doesn't really help.

WebCore/loader/CachedMetadataStore.h:54
 +	static PassOwnPtr<CachedMetadataStore> deserialize(const char* data,
size_t size)
Why two parameters instead of a const Vector<char>&  ?

WebCore/loader/CachedMetadata.h:39
 +  class CachedMetadata : public RefCounted<CachedMetadata> {
I don't understand the role this class plays.  How is it different than
Vector<char> ?	Maybe because it doesn't take ownership of the bytes?

WebCore/loader/CachedResource.h:148
 +	// Sets the serialized metadata retrieved from the platform's cache.
If this is a setter, maybe it should have "set" in its name?

WebCore/loader/CachedResource.h:232
 +	OwnPtr<CachedMetadataStore> m_cachedMetadataStore; // Lazy
Comment not needed.

WebCore/loader/CachedResource.h:153
 +	void setCachedMetadata(unsigned dataTypeID, CachedMetadata*
cachedMetadata);
cachedMetadata parameter name not needed.

WebCore/loader/SubresourceLoader.cpp:181
 +	// So don't deliver any data to the loader yet.
I don't understand this comment.