[webkit-reviews] review denied: [Bug 115804] Avoid unnecessary data copies when loading subresources with DoNotBufferData option : [Attachment 201079] Try to fix mac build

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 115804: Avoid unnecessary data copies when loading subresources with
DoNotBufferData option
https://bugs.webkit.org/show_bug.cgi?id=115804

Attachment 201079: Try to fix mac build
https://bugs.webkit.org/attachment.cgi?id=201079&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=201079&action=review


Good direction.

We plan to do a pretty significant overhaul of Cache/Loader land somewhat soon,
so it'll be important to make very subtle things like this more difficult to
screw up while refactoring.

That's the motivation for my review comments below.

> Source/WebCore/loader/cache/CachedImage.cpp:402
> +void CachedImage::data(const char* data, int length)
> +{
> +    CachedImage::data(ResourceBuffer::create(data, length), false);
> +}

Let's throw the ASSERT(m_options.dataBufferingPolicy == DoNotBufferData); here,
too.

> Source/WebCore/loader/cache/CachedResource.h:105
> +    virtual void data(const char* /*data*/, int /*length*/) { }

Lets give this an implementation in CachedResource.cpp and have it
ASSERT(m_options.dataBufferingPolicy == DoNotBufferData)

Assuming all tests continue passing without that ASSERT firing, I'd like us to
take it a step further and name this method something different instead of just
overloading ::data().  "unbufferedData()" is my first instinct, though I'm
aware that's not perfect.