Bug 94562 – Store CString data in the CStringBuffer to avoid the double indirection

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Benjamin Poulain]

Making CString more efficient...

[Benjamin Poulain]

Created attachment 159592 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159592 [details]
Patch

Attachment 159592 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13549197

[Peter Beverloo (cr-android ews)]

Comment on attachment 159592 [details]
Patch

Attachment 159592 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13549200

[Benjamin Poulain]

Created attachment 159603 [details]
Patch

[Benjamin Poulain]

Once again Chromium has issues with WebKitTestAPI.

I skipped the test as usual. I email Julien Chaffraix about the issue.

[WebKit Review Bot]

Comment on attachment 159603 [details]
Patch

Attachment 159603 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13546271

New failing tests:
accessibility/aria-checkbox-checked.html
animations/3d/matrix-transform-type-animation.html
http/tests/appcache/cyrillic-uri.html
canvas/philip/tests/2d.canvas.readonly.html
animations/animation-add-events-in-handler.html
http/tests/appcache/deferred-events-delete-while-raising.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
accessibility/anchor-linked-anonymous-block-crash.html
http/tests/appcache/deferred-events-delete-while-raising-timer.html
accessibility/aria-disabled.html
http/tests/appcache/crash-when-navigating-away-then-back.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
accessibility/accessibility-object-detached.html
http/tests/appcache/abort-cache-ondownloading-manifest-404.html
http/tests/appcache/credential-url.html
http/tests/appcache/access-via-redirect.php
animations/3d/change-transform-in-end-event.html
animations/animation-controller-drt-api.html
animations/3d/transform-perspective.html
accessibility/accessibility-node-reparent.html
http/tests/appcache/deferred-events.html
animations/3d/state-at-end-event-transform.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
animations/animation-direction-reverse-fill-mode-hardware.html
accessibility/adjacent-continuations-cause-assertion-failure.html
accessibility/aria-help.html

[WebKit Review Bot]

Created attachment 159622 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Benjamin Poulain]

Created attachment 159726 [details]
Patch

[Benjamin Poulain]

(In reply to comment #6)
> (From update of attachment 159603 [details])
> Attachment 159603 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13546271

I could not reproduce any of the Chromium failure. I uploaded the same patch again, maybe the bot was sick?

[Benjamin Poulain]

Comment on attachment 159726 [details]
Patch

Ok, Chromium uses CStringBuffer in WebCString. I missed that.
I'll update.

[Benjamin Poulain]

Created attachment 159738 [details]
Patch

[Darin Adler]

Comment on attachment 159738 [details]
Patch

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

> Source/WTF/wtf/text/CString.cpp:37
> +    if (length > (numeric_limits<size_t>::max() - sizeof(CStringBuffer) / sizeof(char)))
> +        CRASH();

sizeof(char) is defined as 1 by the C and C++ language. It's usually considered better style to just omit it.

> Source/WTF/wtf/text/CString.cpp:41
> +    size_t size = sizeof(CStringBuffer) + length * sizeof(char);

Same comment about sizeof(char).

> Source/WTF/wtf/text/CString.cpp:57
> +    if (!str)
> +        return;

I probably would have put an ASSERT(!length) in there too.

> Source/WTF/wtf/text/CString.h:35
> +// CStringBuffer is the ref-counted storage class for the characters of CStringBuffer.

Should be "for the characters in a CString", not "for the characters of CStringBuffer".

> Source/WTF/wtf/text/CString.h:36
> +// The data is implicitely allocated 1 character longer than length(), as it is zero-terminated.

the word implicitly does not have the letter “e”

> Source/WTF/wtf/text/CString.h:49
> +    CStringBuffer(size_t length)
> +        : m_length(length)
> +    { }

This is not quite the usual style. Normally we put everything on one line, or everything on different lines. It's unusual to have { } on one line with everything else on separate lines.

I also suggest marking this constructor explicit.

> Source/WTF/wtf/text/CString.h:52
> +    size_t m_length;

I think this should be defined const.

[Benjamin Poulain]

Committed r126191: <http://trac.webkit.org/changeset/126191>