Bug 134731 – Make SharedBuffer::append(SharedBuffer*) be smarter about CFData and data arrays

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Pratik Solanki]

Currently SharedBuffer::append(SharedBuffer*) copies over all the data from the target SharedBuffer. When the target buffer has a data array or a platform data, we can just keep a reference to that instead of doing a copy.

[Pratik Solanki]

Created attachment 234571 [details]
Patch

[Pratik Solanki]

Created attachment 234587 [details]
Patch with build fix

[Antti Koivisto]

Comment on attachment 234587 [details]
Patch with build fix

View in context: https://bugs.webkit.org/attachment.cgi?id=234587&action=review

> Source/WebCore/ChangeLog:20
> [phone]  Pratik Solanki  <[e-mail]>
> +
> +        Make SharedBuffer::append(SharedBuffer*) be smarter about CFData and data arrays
> +        https://bugs.webkit.org/show_bug.cgi?id=134731
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests because no functional changes.
> +
> +        * platform/SharedBuffer.cpp:
> +        (WebCore::SharedBuffer::append):
> +        (WebCore::SharedBuffer::maybeAppendPlatformData):
> +        * platform/SharedBuffer.h:
> +        * platform/cf/SharedBufferCF.cpp:
> +        (WebCore::SharedBuffer::maybeAppendPlatformData):
> +        (WebCore::SharedBuffer::maybeAppendDataArray):
> +        * platform/soup/SharedBufferSoup.cpp:
> +        (WebCore::SharedBuffer::maybeAppendPlatformData):
> +
> [phone]  Pratik Solanki  <[e-mail]>

Two change logs. You could mention how it is smarter.

[Build Bot]

Comment on attachment 234587 [details]
Patch with build fix

Attachment 234587 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6699772173877248

New failing tests:
js/regress/emscripten-cube2hash.html
fast/canvas/image-potential-subsample.html
fast/writing-mode/broken-ideographic-font.html
http/tests/multipart/multipart-replace-non-html-content.php
fast/writing-mode/broken-ideograph-small-caps.html
http/tests/multipart/multipart-html.php

[Build Bot]

Created attachment 234652 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Pratik Solanki]

Created attachment 234661 [details]
Patch with potential layout test fixes

I couldn't reproduce the exact layout test failures but noticed some bugs in the code when debugging.

[Pratik Solanki]

Comment on attachment 234587 [details]
Patch with build fix

View in context: https://bugs.webkit.org/attachment.cgi?id=234587&action=review

> Source/WebCore/platform/cf/SharedBufferCF.cpp:119
> +    m_size = CFDataGetLength(m_cfData.get());

This function has bugs. We should bail out if size() is not 0 instead of checking for m_buffer.size() - we can have data in m_segments. Also we should not be setting m_size here. When platform data (CFDataRef) is present, m_size is 0 and size() returns the correct value if we have platform data. Updated patch coming up. That may fix the layout test failures.

[Pratik Solanki]

Committed r170938: <http://trac.webkit.org/changeset/170938>