[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 #204059|review?                     |review+
               Flag|                            |




--- Comment #39 from Darin Adler <darin at apple.com>  2013-06-13 10:22:22 PST ---
(From update of attachment 204059)
View in context: https://bugs.webkit.org/attachment.cgi?id=204059&action=review

Looks good.

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

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

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

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

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

> Source/WebCore/loader/cache/CachedImage.h:68
> +    virtual void addDataBuffer(ResourceBuffer*) OVERRIDE;
> +    virtual void finishLoading(ResourceBuffer*) OVERRIDE;

Can these be private instead of public?

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

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

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

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