Summary: | RenderText::isAllCollapsibleWhitespace() shouldn't upconvert string to 16-bit. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||
Component: | Text | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, buildbot, eric, kling, koivisto, msaboff, ojan.autocc, rniwa, webkit.review.bot | ||||||
Priority: | P2 | Keywords: | Performance | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Andreas Kling
2013-02-09 06:52:09 PST
Created attachment 187429 [details]
Patch
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 on attachment 187429 [details]
Patch
Fantastic. Thank you. Mac-WK2 looks sad, but I'm sure you'll check before landing.
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 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. Created attachment 187681 [details]
Patch for landing
Hoisted the 8/16 bit checks out of the loop as requested.
Comment on attachment 187681 [details] Patch for landing Clearing flags on attachment: 187681 Committed r142529: <http://trac.webkit.org/changeset/142529> All reviewed patches have been landed. Closing bug. |