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

Mon May 3 14:07:58 PDT 2010

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

Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #54957|review?                     |review-
               Flag|                            |

--- Comment #14 from Adam Barth <abarth at webkit.org>  2010-05-03 14:07:56 PST ---
(From update of attachment 54957)
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.

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.

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.

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

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

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.
WebCore/loader/CachedMetadataMap.cpp:57
 +      return buffer;
return buffer.release();

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?

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

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.

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

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

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...
WebCore/loader/CachedResource.cpp:172
 +          m_cachedMetadataMap.set(CachedMetadataMap::create().release());
I don't think this release() is correct.

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.

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

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?

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.

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

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

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