[Webkit-unassigned] [Bug 37874] Provide mechanism to cache metadata for a resource

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37874





--- Comment #19 from Tony Gentilcore <tonyg at chromium.org>  2010-05-05 11:04:47 PST ---
I have a couple of questions inline below. I'll hold off on uploading the next
patch until those are answered.

By the way, do you think that reinterpret_cast is okay or should I ask
around/research it a little more?

(In reply to comment #18)
> (From update of attachment 55079 [details])
> This is getting close.  I'd like to see this code used by something so it's not
> dead code though.  :)

This patch is hairy enough that I don't think I should add to it. I'll upload a
separate patch for bindings/v8 now. Should it use this bug or a new bug?

> 
> 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.

Done.

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

I disagree. I think it is crucial for readability to have the offsets of the
format in one place. If I just inline it, then I'm left with code that says
readUnsigned(0) instead of readUnsigned(dataTypeIDStart). If you still feel
strongly then I'll defer to you and inline it.

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

This is a side-effect of the main API. It looks like:


Load:     ResourceLoader::didReceiveCachedMetadata(const char*, int)
Persist:  ResourceHandle::cacheMetadata(const ResourceResponse&, const
Vector<char>&)

They should probably both use Vectors or both use char*/length. My thought is
to remove the Vector in favor of char*/length. But I wanted to confirm with you
before making the change.

> 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?
> 

It is basically a class to point to data without owning it (to avoid a copy).
This allows CachedResource::cachedMetadata(unsigned) to return one object
rather than the much uglier CachedResource::cachedMetadata(unsigned, const
char**, int*). I couldn't find a suitable generic container in WebKit. Do you
have a suggestion for a better way to return a pointer/length combination
without a copy?

> 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?

Done. setSerializedCachedMetadata().

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

Done.

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

Done.

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

Oops, that was branched from didReceiveData, but it doesn't make sense here
since its a one-shot. Removed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.