Summary: | Store CString data in the CStringBuffer to avoid the double indirection | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||||||||||
Component: | Web Template Framework | Assignee: | Benjamin Poulain <benjamin> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | dglazkov, gyuyoung.kim, jchaffraix, kling, peter+ews, rakuco, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 94871 | ||||||||||||||
Attachments: |
|
Description
Benjamin Poulain
2012-08-20 19:21:54 PDT
Created attachment 159592 [details]
Patch
Comment on attachment 159592 [details] Patch Attachment 159592 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13549197 Comment on attachment 159592 [details] Patch Attachment 159592 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13549200 Created attachment 159603 [details]
Patch
Once again Chromium has issues with WebKitTestAPI. I skipped the test as usual. I email Julien Chaffraix about the issue. 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 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
Created attachment 159726 [details]
Patch
(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? Comment on attachment 159726 [details]
Patch
Ok, Chromium uses CStringBuffer in WebCString. I missed that.
I'll update.
Created attachment 159738 [details]
Patch
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. Committed r126191: <http://trac.webkit.org/changeset/126191> |