[Webkit-unassigned] [Bug 115804] Avoid unnecessary data copies when loading subresources with DoNotBufferData option

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #31 from Darin Adler <darin at apple.com>  2013-06-05 18:14:08 PST ---
(From update of attachment 203681)
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?

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