[webkit-reviews] review denied: [Bug 115804] Avoid unnecessary data copies when loading subresources with DoNotBufferData option : [Attachment 203681] Patch rebased to apply on current git master

[bugzilla-daemon at webkit.org]

Darin Adler <darin 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 203681: Patch rebased to apply on current git master
https://bugs.webkit.org/attachment.cgi?id=203681&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=203681&action=review


review- because of some not-entirely-harmless PassRefPtr mistakes

> Source/WebCore/ChangeLog:20
> +	   incremental data chunks when not beffering and

typo: beffering

> Source/WebCore/loader/SubresourceLoader.cpp:243
> +	   if (resourceData())
> +	       m_resource->addDataBuffer(resourceData());

The resourceData function is not inlined, so it’s not great to call it twice. I
would write:

    if (ResourceBuffer* resourceData = this->resourceData().get())
	m_resource->addDataBuffer(resourceData);
    else

And also we need to fix resourceData() since it’s return a PassRefPtr even
though it is not transferring ownership.

> Source/WebCore/loader/cache/CachedCSSStyleSheet.h:50
> +	   virtual void finishLoading(PassRefPtr<ResourceBuffer> data);

I’m not sure this argument needs a name. The name data says almost nothing!

> Source/WebCore/loader/cache/CachedFont.h:51
> +    virtual void finishLoading(PassRefPtr<ResourceBuffer> data);

Can this virtual function be marked OVERRIDE, FINAL, or both? Can it be made
private?

> Source/WebCore/loader/cache/CachedImage.cpp:397
> +    addDataBuffer(data, false);

This use of a boolean is no good. With every caller passing a constant, it’s
not readable at the call site. We typically use enums in cases like this to
make the code readable.

> Source/WebCore/loader/cache/CachedImage.cpp:403
> +    addDataBuffer(ResourceBuffer::create(data, length), false);

Ditto.

> Source/WebCore/loader/cache/CachedImage.cpp:408
> +    addDataBuffer(data, true);

Ditto.

> Source/WebCore/loader/cache/CachedImage.cpp:410
> +	   CachedResource::finishLoading(data);

This is always going to pass a 0 to CachedResource::finishLoading, because data
has already passed off its resource buffer when calling the addDataBuffer
function. We know that doesn’t matter because CachedResource::finishLoading
ignores the data passed to it, but it’s really strange to code this way. Maybe
we can at least just pass 0 here instead of pretending to pass data.

> Source/WebCore/loader/cache/CachedImage.h:80
> +    virtual void addData(const char* data, int length);

I don’t think int is the right type for length. The type unsigned could be OK,
or maybe it needs to be size_t.

> Source/WebCore/loader/cache/CachedImage.h:81
> +    virtual void addDataBuffer(PassRefPtr<ResourceBuffer>);
> +    virtual void addData(const char* data, int length);
> +    virtual void finishLoading(PassRefPtr<ResourceBuffer>);

Can these virtual functions be marked OVERRIDE, FINAL, or both? Can they be
made private?

> Source/WebCore/loader/cache/CachedRawResource.cpp:74
> +    CachedResourceHandle<CachedRawResource> protect(this);
> +    ASSERT(m_options.dataBufferingPolicy == DoNotBufferData);
> +    notifyDataReceived(data, length);

The protector here seems like it’s at the wrong level. It could instead be
inside notifyDataReceived, since that’s the only function called by this one.

> Source/WebCore/loader/cache/CachedRawResource.h:55
> -    virtual void data(PassRefPtr<ResourceBuffer> data, bool
allDataReceived);
> +    virtual void addDataBuffer(PassRefPtr<ResourceBuffer>);
> +    virtual void addData(const char* data, int length);
> +    virtual void finishLoading(PassRefPtr<ResourceBuffer>);

Can these virtual functions be marked OVERRIDE, FINAL or both?

> Source/WebCore/loader/cache/CachedRawResource.h:67
> +    void notifyDataReceived(const char* data, int length);

I know we’ve used this kind of naming before, but it’s horribly confusing:
“notify data received”? Notify who? Notify the data? I think
notifyClientsDataWasReceived is one possible better name. Or
callDataReceivedForAllClients? Maybe there’s another one that’s clear.

> Source/WebCore/loader/cache/CachedResource.cpp:375
> +void CachedResource::addData(const char* data, int length)
> +{
> +    UNUSED_PARAM(data);
> +    UNUSED_PARAM(length);
> +    ASSERT(m_options.dataBufferingPolicy == DoNotBufferData);
> +}

I think it’s better to omit the argument names here.

> Source/WebCore/loader/cache/CachedResource.h:104
> +    virtual void addDataBuffer(PassRefPtr<ResourceBuffer> data);

I don’t think the argument name here is helpful or needed.

> Source/WebCore/loader/cache/CachedResource.h:106
> +    virtual void finishLoading(PassRefPtr<ResourceBuffer> data);

I don’t think the argument name here is helpful or needed.

> Source/WebCore/loader/cache/CachedSVGDocument.h:43
> +    virtual void finishLoading(PassRefPtr<ResourceBuffer> data);

Can these virtual functions be marked OVERRIDE, FINAL or both? I don’t think
the argument name here is helpful or needed.

> Source/WebCore/loader/cache/CachedScript.cpp:89
> +    CachedResource::finishLoading(data);

This is always going to pass a 0 to CachedResource::finishLoading, because data
has already passed off its resource buffer assigning to m_data. We know that
doesn’t matter because CachedResource::finishLoading ignores the data passed to
it, but it’s really strange to code this way. We could pass m_data.

> Source/WebCore/loader/cache/CachedScript.h:45
> +	   virtual void finishLoading(PassRefPtr<ResourceBuffer> data);

Can these virtual functions be marked OVERRIDE, FINAL or both?

> Source/WebCore/loader/cache/CachedShader.cpp:66
> +    m_data = data;
> +    CachedResource::finishLoading(data);

This is always going to pass a 0 to CachedResource::finishLoading, because data
has already passed off its resource buffer assigning to m_data. We know that
doesn’t matter because CachedResource::finishLoading ignores the data passed to
it, but it’s really strange to code this way. We could pass m_data. Seems this
bug already existed in the old version of this function.

> Source/WebCore/loader/cache/CachedTextTrack.cpp:65
> +    addDataBuffer(data);
> +    CachedResource::finishLoading(data);

This is always going to pass a 0 to CachedResource::finishLoading, because data
has already passed off its resource buffer calling addDataBuffer. We know that
doesn’t matter because CachedResource::finishLoading ignores the data passed to
it, but it’s really strange to code this way. We could pass 0, or go out of our
way to actually pass the data buffer.

> Source/WebCore/loader/cache/CachedTextTrack.h:42
> +    virtual void addDataBuffer(PassRefPtr<ResourceBuffer>);
> +    virtual void finishLoading(PassRefPtr<ResourceBuffer>);

Can these virtual functions be marked OVERRIDE, FINAL or both? Can they be made
private?

> Source/WebCore/loader/cache/CachedXSLStyleSheet.h:48
> +	   virtual void finishLoading(PassRefPtr<ResourceBuffer> data);

Can these virtual functions be marked OVERRIDE, FINAL or both? Can they be made
private?