[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 #8 from Tony Gentilcore <tonyg at chromium.org>  2010-04-22 15:07:13 PST ---
(In reply to comment #7)
> (From update of attachment 54091 [details])
> 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*.

No reason except my unfamiliarity with the code. Done.

Because I have a couple of questions inline below, I'm holding off on uploading
a new patch until they are all worked out.

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

Changed to only test frame. Let me know if loader needs to be tested as well.

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

Not really. In the case that the resource is not cached, It has a reference to
a DocLoader (which has a reference to Frame). Passing a frame in this way does
feel like a strange thing to do, but it was the best I could come up with. Do
you have any alternate ideas?

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

Good, me too. I took your previous comment to add a struct too literally.
Fixed.

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

That does sound nice. But I'm clearly missing something about the overall
picture of the architecture. As far as I've been able to discover, routing
through the FrameLoaderClient is the only way to get a message back to the
embedding application. Can you point me to the interface where WebCore can send
a notification to the networking layer with this data.

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

Yes, it does seem odd. That's why I had initially not passed a type along with
the metadata. That leaves it in the hands of each implementer of metadata to
determine if the data belongs to them or not. In this case, V8Proxy would just
see if the data starts with its magic number or not. An enum, by definition,
means that we are naming each type of metadata that can be stored -- be it for
JSC, V8, or something else -- and that we need a registry of those types. If
you want to keep a type, but lose the enum, we could basically extract the
magic number out of the stream and make the type a GUID that each implementer
chooses (relying on the size of the space for uniqueness).

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