Bug 109354 – RenderText::isAllCollapsibleWhitespace() shouldn't upconvert string to 16-bit.

• Component: Text
• Status: RESOLVED FIXED

[Andreas Kling]

Small win here.

[Andreas Kling]

Created attachment 187429 [details]
Patch

[Build Bot]

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

[Eric Seidel (no email)]

Comment on attachment 187429 [details]
Patch

Fantastic.  Thank you.  Mac-WK2 looks sad, but I'm sure you'll check before landing.

[Benjamin Poulain]

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.

[Darin Adler]

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.

[Andreas Kling]

Created attachment 187681 [details]
Patch for landing

Hoisted the 8/16 bit checks out of the loop as requested.

[WebKit Review Bot]

Comment on attachment 187681 [details]
Patch for landing

Clearing flags on attachment: 187681

Committed r142529: <http://trac.webkit.org/changeset/142529>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.