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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 6 06:00:54 PDT 2013


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





--- Comment #32 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-06-06 05:59:27 PST ---
(In reply to comment #31)
> (From update of attachment 203681 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203681&action=review
> 
> review- because of some not-entirely-harmless PassRefPtr mistakes

Thanks for the review. Since some of the issues are not actually introduced by this patch, I've opened separate bugs to fix them.

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

Oops.

> > 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.

Yes, this was actually a refactoring of current code, see SubresourceLoader::sendDataToResource(). I also don't understand why resourceData returns a PassRefPtr. See bug #117288.

> > 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!

Agree.

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

Yes, actually all of them can be made private and marked as OVERRIDE, see bug #117289.

> > 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.

Will fix it.

> > 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.

Ah, right, PassRefPtr is leaked and assigned to a RefPtr. I think it's confusing in this case where we chain up to the parent. I've changed data() to receive a raw pointer too in bug #117288, I think it makes the code less confusing and still allows the cached resource to take a reference.

> > 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.

I think this comes from ResourceHandle::didReceiveData() that also uses int for length. I agree it should be size_t. ResourceHandle::didSendData() uses unsigned long long though.

> > 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.

Ok.

> > 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.

Fair enough, I'll rename it.

> > 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.

Right, I don't know why I used UNUSED_PARAM in this case.

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


More information about the webkit-unassigned mailing list