[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





--- Comment #41 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-06-14 02:08:09 PST ---
(In reply to comment #39)
> (From update of attachment 204059 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204059&action=review
> 
> Looks good.

Thanks!

> > Source/WebCore/loader/SubresourceLoader.cpp:208
> > +        // The loader delivers the data in a multipart section all at once, then sends eof.
> 
> Not sure this comment adds much. The comment two lines down seems to say the same thing. I know you moved this comment here another function, but I think we can just drop it.

Removed.

> > Source/WebCore/loader/cache/CachedImage.cpp:347
> > +bool CachedImage::isValidDecodedImageSize() const
> 
> I think the name here should be “<cached image> has valid decoded image size” not “<cached image> is valid decoded image size”. So hasValidDecodedImageSize.
> 
> But I am not sure I understand the use of the word “valid” here to mean “smaller than the maximum”. I find this function quite confusing and a better name might help, and a why comment is needed.

The use of valid is mainly because we are using this function to decide whether the image can be drawn or if we should raise an error. 

> The old code referred to this as “we have received all the data or the size is known”, but even that did not make the maximumDecodedImageSize feature clear.
> 
> I think this is actually “is ready to draw” or something like that.

Is ready to draw sounds also confusing to me, since it sounds like we are failing just because the image is not yet ready to be drawn. So I used canBeDrawn in the end, and added a comment to explain why we are failing when the image can't be drawn.

> > Source/WebCore/loader/cache/CachedImage.cpp:386
> > +    // Go ahead and tell our observers to try to draw if the size is known.
> > +    // Each chunk from the network causes observers to repaint, which will
> > +    // force that chunk to decode.
> > +    if (!isValidDecodedImageSize()) {
> > +        error(errorOccurred() ? status() : DecodeError);
> > +        if (inCache())
> > +            memoryCache()->remove(this);
> > +        return;
> >      }
> 
> This comment is confusing. It says “tell observers to draw if the size is known”, but the code says “don’t tell observers to draw unless the size is known”. We should fix the comment since we reversed the code.

What is confusing is the place where the comment is, right before the if that checks whether the image can be drawn or not, that now has nothing to do with if the size is available or not. So, I moved the comment and removed the part about the size available, since there's an early return when the size it's not available.

> > Source/WebCore/loader/cache/CachedImage.cpp:405
> > +    RefPtr<ResourceBuffer> resourceData = ResourceBuffer::create(data, length);
> > +    addIncrementalDataBuffer(resourceData.get());
> 
> We don’t need a local variable here: It would read better as a one-liner.

Ok.

> > Source/WebCore/loader/cache/CachedImage.cpp:414
> > +    if (!m_image && data) {
> > +        // When loading multipart content all data is delivered in finishLoading.
> > +        createImage();
> >      }
> 
> This is not a good comment to have here. Even if all data wasn’t delivered in finishLoading, the first data could be delivered here for some other reason. So this comment is a bit of trivia that does not correctly answer the question “why do we have this code?” We would not want to remove this if we removed support for multipart content, for example, or if we changed how multipart content loading works.

Ok, removed it.

> > Source/WebCore/loader/cache/CachedImage.h:68
> > +    virtual void addDataBuffer(ResourceBuffer*) OVERRIDE;
> > +    virtual void finishLoading(ResourceBuffer*) OVERRIDE;
> 
> Can these be private instead of public?

No, they are used by ImageDocument, so I left them public as we discussed in the other bug, see:

https://bugs.webkit.org/show_bug.cgi?id=117289#c2

> > Source/WebCore/loader/cache/CachedRawResource.cpp:52
> > +    incrementalDataLength = data->size() - previousDataLength;
> 
> Do we have a guarantee that this will not overflow 32 bits? The old code used size_t for this, not unsigned.

ResourceBuffer::size() returns unsigned, and CachedResource::encodedSize() also returns unsigned, so previousDataLength so actually be unsigned too instead of size_t. That makes clearer that unsigned is appropriate for incrementalDataLength.

> > Source/WebCore/loader/cache/CachedRawResource.cpp:92
> > +        unsigned incrementalDataLength;
> > +        const char* incrementalData = calculateIncrementalDataChunk(data, incrementalDataLength);
> > +        if (data)
> > +            setEncodedSize(data->size());
> > +        notifyClientsDataWasReceived(incrementalData, incrementalDataLength);
> 
> A little strange to have the setEncodedSize call in the middle of the logic to calculate the incremental data chunk. I guess the old code was like that, but is it important to do things in this order?

calculateIncrementalDataChunk() uses current encoded size to calculate the incremental data length, so we need to set the new one after calculateIncrementalDataChunk. I left it also before notifying the clients to make sure that new encoded size is updated in case clients need to query that value.

> > Source/WebCore/loader/cache/CachedRawResource.h:53
> > -    virtual void data(ResourceBuffer*, bool allDataReceived) OVERRIDE;
> > +    virtual void addDataBuffer(ResourceBuffer*);
> > +    virtual void addData(const char* data, unsigned length);
> > +    virtual void finishLoading(ResourceBuffer*);
> 
> These should all be marked OVERRIDE.

Right!

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