[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