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

Tue May 4 17:34:27 PDT 2010

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

--- Comment #15 from Tony Gentilcore <tonyg at chromium.org>  2010-05-04 17:34:26 PST ---
(In reply to comment #14)
> (From update of attachment 54957 [details])
> Structurally, this looks a lot better.  There's some confusing about how
> RefPtr/PassRefPtr/OwnPtr/PassOwnPtr works.  If you haven't read
> http://webkit.org/coding/RefPtr.html, it can be helpful in explaining how these
> classes are meant to be used.

Thanks, I hadn't seen that doc. It was extremely helpful. You are right, I
didn't understand how the WTF smart pointers worked.

Would it be controversial if I mailed a patch to add a comment pointing to the
documentation in RefPtr.h?

> 
> I don't like the fancy template / subclass / reinterpret_cast way of
> serializing metadata.  It's a bit hard to tell from this patch how this
> machinery will be used, but I'd start with something dead simple and we can
> make it more complicated if we need to.  The simplest thing is probably a
> static registry of IDs and only allowing for kind of cached metadata per
> resource.  That will get rid of all the mystical code about subclasses being
> forbidden from adding data members.  In any case, it looks like the clients of
> this system need to covert their metadata into byte arrays already.

Okay, I've tossed the map and gone back to the assumption that there is one
type of metadata per resource. This simplifies things a great deal, but still
requires each WebCore generator of metadata to choose a GUID-like ID. I can't
think of any way to have a static registry unless you are okay with a single
bottle-neck file somewhere that has ID names like "V8Metadata", "JSCMetadata",
etc.

> 
> WebCore/loader/CachedMetadata.h:5
>  +      modify it under the terms of the GNU Library General Public
> Google usually contributes under the BSD license.  This license is fine, but
> just a little outside the norm.

Oops. I just copied the license from the nearest file not realizing that files
within WebKit have different licences. Hopefully I've got the right one now.

> 
> WebCore/loader/CachedMetadata.h:38
>  +  // Subclasses must not contain non-static member variables because they
> would
> Why not just make the destructor virtual?

Moot.

> 
> WebCore/loader/CachedMetadataMap.cpp:32
>  +  // static
> We usually skip these comments in WebKit.

Moot.

> 
> WebCore/loader/CachedMetadata.h:42
>  +      static CachedMetadata* create(const char* data, size_t size)
> This should probably return a PassOwnPtr
> If you haven't read http://webkit.org/coding/RefPtr.html, you might find it
> helpful.

Thanks again for the doc. You are right.

> WebCore/loader/CachedMetadataMap.cpp:57
>  +      return buffer;
> return buffer.release();

Moot.

> 
> WebCore/loader/CachedMetadataMap.cpp:76
>  +              // The dataSize has lied to us which means the format is
> corrupt.
> Do we want to signal this error condition somehow instead of failing silently?

Moot.

> 
> WebCore/loader/CachedMetadataMap.cpp:79
>  +              CachedMetadata* cachedMetadata =
> CachedMetadata::create(reinterpret_cast<const char*>(buffer + bufferPosition),
> dataSize);
> Why do we need this reinterpret_cast here?  Isn't buffer already a const char*?

Moot.

> 
> WebCore/loader/CachedMetadataMap.cpp:91
>  +  void CachedMetadataMap::appendValueToBuffer(const T* value,
> PassRefPtr<SharedBuffer> buffer) const
> This should be a raw SharedBuffer*.  We use PassRefPtr to signal that we're
> passing ownership of the refcounted object.

I'm now passing a const Vector<char>&. Is that ideal?

> 
> WebCore/loader/CachedMetadataMap.cpp:93
>  +      buffer->append(reinterpret_cast<const char*>(value), sizeof(T));
> Yuck.  This isn't strictly ANSI compliant.
> 

Are you referring to ANSI C (i.e. char could be another size) or ANSI unicode
(i.e. doesn't handle UTF-8)?

If C: I could switch to uint8_t/uint32_t instead of char/unsigned. But I'd
still have to cast at the API level to make this useful.

If Unicode: Keep in mind that I'm expecting this to be a serialization of bytes
of metadata. It is not a string.

If something else, please explain.

> WebCore/loader/CachedMetadataMap.cpp:108
>  +      *bufferPosition += readSize;
> WebKit usually uses non-const reference for output parameters instead of
> pointers.

Moot.

> 
> WebCore/loader/CachedMetadataMap.h:59
>  +          // the static_casts to work properly. This means no member
> variables.
> Oh, I see.  Hum...  There's got to be a better design.  This is too much
> low-level memory poking.
> I feel like you're trying to use the C++ type system to do a bunch of work that
> it's not very good at doing.  Why do you have a separate subtype for each kind
> of thing you might want to cache?  It feels like you just want to have some
> sort of static registry or something to avoid conflicts.
> 
> A lot of this feels like duplication of Pickle.  I guess we don't have that
> here, do we...

Okay. I was hoping to make the API just slightly cleaner by exposing:
cachedResource.get<MyDataType>();
instead of:
cachedResource.get(myDataTypeID);

But in the interest of simplicity, I've gotten rid of all of the templates.

Yes, if Pickle were available, I'd be able to use that.
WebCore/bindings/v8/SerializedScriptValue.cpp has similar serialization logic.
But since my format is so simple (an unsigned followed by a byte bucket), I've
just rolled my own. Let me know if you have a better idea for serialization.

> WebCore/loader/CachedResource.cpp:172
>  +          m_cachedMetadataMap.set(CachedMetadataMap::create().release());
> I don't think this release() is correct.

Moot.

> 
> WebCore/loader/CachedResource.cpp:407
>  +      ASSERT(m_cachedMetadataMap->size() == 1);
> Hum...  This mechanism seems like drastic overkill if there's only ever one
> thing in the map.  Maybe make the simpler thing first and we can elaborate it
> as needed later.

Done.

> 
> WebCore/loader/CachedResource.h:166
>  +              return m_cachedMetadataMap->get<T>();
> I'm not really sure what this templatization is buying you.

See previous comment. I thought it made the API more natural. It is gone now.

> 
> WebCore/platform/network/ResourceHandle.cpp:148
>  +  void ResourceHandle::cacheMetadata(const ResourceResponse&,
> PassRefPtr<SharedBuffer>)
> This PassRefPtr is confusing here.  Are we taking ownership of the
> SharedBuffer?  If so, is it ok to do nothing here?

I didn't understand PassRefPtr properly before reading that document. It is now
a raw pointer.

> 
> WebCore/platform/network/ResourceHandle.h:115
>  +      static void cacheMetadata(const ResourceResponse&,
> PassRefPtr<SharedBuffer>);
> I suspect this should be a SharedBuffer*, but I'm not sure.

Yep, done.

> 
> WebKit/chromium/tests/CachedMetadataMapTest.cpp:13
>  +   * distribution.
> This is the more usual license for Google contributions.
> 

I've used this license throughout.

> WebKit/chromium/tests/CachedMetadataMapTest.cpp:32
>  +  // original KURL's.
> This comment seems wrong.

Yep, unintentionally branched from another test. Removed.

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