[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 #20 from Adam Barth <abarth at webkit.org>  2010-05-05 11:34:33 PST ---
(In reply to comment #19)
> 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?

I'd rather we didn't have to reinterpret_cast, but I'm not sure where's a way
around it.

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

Good idea.  Please use a separate bug.  We can cross-link them if we need to
using the "blocks" field.

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

Ok.

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

Using Vector is generally better because it's clear who owns the memory.  The
main reason to use char* + length is if the malloc in Vector is too expensive
or there's a critical memcpy that can't be removed.

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

I guess my main concern is that the ownship of the memory is unclear.  How long
can the caller hold on to this object before it explodes?  I think this is the
problem that SharedBuffer is trying to solve.

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