[webkit-reviews] review granted: [Bug 115804] Avoid unnecessary data copies when loading subresources with DoNotBufferData option : [Attachment 204059] Updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 115804: Avoid unnecessary data copies when loading subresources with
DoNotBufferData option
https://bugs.webkit.org/show_bug.cgi?id=115804

Attachment 204059: Updated patch
https://bugs.webkit.org/attachment.cgi?id=204059&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.