Bug 109354 - RenderText::isAllCollapsibleWhitespace() shouldn't upconvert string to 16-bit.
Summary: RenderText::isAllCollapsibleWhitespace() shouldn't upconvert string to 16-bit.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Performance
Depends on:
Blocks:
 
Reported: 2013-02-09 06:52 PST by Andreas Kling
Modified: 2013-02-11 15:30 PST (History)
9 users (show)

See Also:


Attachments
Patch (1.32 KB, patch)
2013-02-09 06:52 PST, Andreas Kling
eric: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (1.62 KB, patch)
2013-02-11 14:45 PST, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2013-02-09 06:52:09 PST
Small win here.
Comment 1 Andreas Kling 2013-02-09 06:52:52 PST
Created attachment 187429 [details]
Patch
Comment 2 Build Bot 2013-02-10 06:07:27 PST
Comment on attachment 187429 [details]
Patch

Attachment 187429 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16470772

New failing tests:
compositing/checkerboard.html
accessibility/anchor-linked-anonymous-block-crash.html
http/tests/cache/cancel-multiple-post-xhrs.html
animations/3d/state-at-end-event-transform.html
animations/3d/replace-filling-transform.html
http/tests/cache/history-only-cached-subresource-loads.html
compositing/bounds-in-flipped-writing-mode.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
accessibility/accessibility-object-detached.html
accessibility/anonymous-render-block-in-continuation-causes-crash.html
animations/animation-controller-drt-api.html
compositing/absolute-position-changed-with-composited-parent-layer.html
compositing/absolute-position-changed-in-composited-layer.html
http/tests/cache/iframe-304-crash.html
animations/3d/transform-perspective.html
http/tests/cache/cancel-during-failure-crash.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
animations/3d/transform-origin-vs-functions.html
animations/animation-css-rule-types.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
Comment 3 Eric Seidel (no email) 2013-02-10 13:51:45 PST
Comment on attachment 187429 [details]
Patch

Fantastic.  Thank you.  Mac-WK2 looks sad, but I'm sure you'll check before landing.
Comment 4 Benjamin Poulain 2013-02-10 16:24:41 PST
Comment on attachment 187429 [details]
Patch

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

> Source/WebCore/rendering/RenderText.cpp:1193
> +    for (unsigned i = 0; i < textLength(); i++) {
> +        if (!style()->isCollapsibleWhiteSpace(characterAt(i)))

I would store textLength() in a temporary variable.

You may want to split this in 2 cases, 8bits and 16bits, instead of doing that in the loop.
Comment 5 Darin Adler 2013-02-11 07:40:48 PST
Comment on attachment 187429 [details]
Patch

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

>> Source/WebCore/rendering/RenderText.cpp:1193
>> +        if (!style()->isCollapsibleWhiteSpace(characterAt(i)))
> 
> I would store textLength() in a temporary variable.
> 
> You may want to split this in 2 cases, 8bits and 16bits, instead of doing that in the loop.

Ben has a good point about the two cases. The new code is going to be considerably slower than two separate loops. You could prove we need only a single loop with performance testing, or you could write the two loops.
Comment 6 Andreas Kling 2013-02-11 14:45:35 PST
Created attachment 187681 [details]
Patch for landing

Hoisted the 8/16 bit checks out of the loop as requested.
Comment 7 WebKit Review Bot 2013-02-11 15:30:20 PST
Comment on attachment 187681 [details]
Patch for landing

Clearing flags on attachment: 187681

Committed r142529: <http://trac.webkit.org/changeset/142529>
Comment 8 WebKit Review Bot 2013-02-11 15:30:24 PST
All reviewed patches have been landed.  Closing bug.