Bug 96590 – Added 8 bit path to WidthIterator::advance()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Michael Saboff]

This is part of adding 8 bit code path to text rendering.

[Michael Saboff]

Created attachment 163758 [details]
Patch

[Michael Saboff]

Created attachment 163893 [details]
Patch with updates 

Updated the original patch with two changes.

Changed Source/WebCore/platform/graphics/Latin1TextIterator.h to be an svn copy of Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.h to retain history.

Ran performance tests of original patch and it showed a regression in the Layout test.  Changed SurrogatePairAwareTextIterator to have inline versions of advance and consume with a new consumeSlowCase for the possible surrogate pair cases.  Performance result of this patch are:

                        Baseline        With patch      Diff
flexbox-column-nowrap   103708.6        109858.0
flexbox-column-nowrap   109955.7        106864.1
flexbox-column-nowrap   108447.6        106678.0        
flexbox-column-nowrap   106098.8        107921.9        
                Mean    107052.7        107830.5       -0.81%
flexbox-column-wrap     109909.5        106872.5       
flexbox-column-wrap     108883.3        105228.3        
flexbox-column-wrap     106647.2        106476.6        
flexbox-column-wrap     105324.4        107460.5        
                Mean    107691.1        106509.5        1.10%
flexbox-row-nowrap      146211.5        142378.8        
flexbox-row-nowrap      147438.1        143749.6        
flexbox-row-nowrap      143789.2        144680.3        
flexbox-row-nowrap      148418.2        145950.7        
                Mean    146464.3        144189.8       -1.55%
flexbox-row-wrap        104452.9        109111.9       
flexbox-row-wrap        109038.1        109778.9        
flexbox-row-wrap        107117.2        109969.8        
flexbox-row-wrap        107728.8        107058.3        
                Mean    107084.3        108979.7        1.77%
floats_100_100_nested      222.5           217.5        
floats_100_100_nested      224.0           215.5
floats_100_100_nested      224.5           214.5
floats_100_100_nested      224.0           216.5
                Mean       223.8           216.0        3.46%
floats_100_100             205.0           198.5        
floats_100_100             207.0           201.0
floats_100_100             205.5           199.5
floats_100_100             206.0           200.5
                Mean       205.9           199.9        2.91%
floats_2_100_nested        463.3           463.2        
floats_2_100_nested        462.7           464.3
floats_2_100_nested        464.7           463.7
floats_2_100_nested        467.0           462.3
                Mean       464.4           463.4        0.23%
floats_2_100               203.0           203.2        
floats_2_100               203.8           207.6
floats_2_100               205.9           207.5
floats_2_100               203.3           203.7
                Mean       204.0           205.5       -0.74%
floats_20_100_nested       623.5           613.7       
floats_20_100_nested       621.5           617.2
floats_20_100_nested       629.8           615.8
floats_20_100_nested       627.3           615.2
                Mean       625.5           615.5        1.61%
floats_20_100              426.3           424.0        
floats_20_100              422.6           420.3
floats_20_100              423.6           420.3
floats_20_100              426.9           420.1
                Mean       424.8           421.2        0.86%
floats_50_100_nested       380.0           372.8        
floats_50_100_nested       378.0           368.0
floats_50_100_nested       378.0           370.6
floats_50_100_nested       377.4           371.4
                Mean       378.4           370.7        2.02%
floats_50_100              307.2           300.4        
floats_50_100              304.4           300.0
floats_50_100              307.2           299.0
floats_50_100              304.4           299.8
                Mean       305.8           299.8        1.96%
line-layout                 75.5            78.2        
line-layout                 75.7            78.5
line-layout                 75.4            78.2
line-layout                 75.4            78.2
                Mean        75.5            78.3        3.68%

[Michael Saboff]

Created attachment 163894 [details]
Updated Patch with development #if

Same comments as prior patch.

[Geoffrey Garen]

Comment on attachment 163894 [details]
Updated Patch with development #if

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

> Source/WebCore/platform/graphics/Latin1TextIterator.h:2
> + * Copyright (C) Research In Motion Limited 2011. All rights reserved.

Why the RIM copyright, and no Apple copyright?

If this is new code, please use the license @ http://www.webkit.org/coding/bsd-license.html.

> Source/WebCore/platform/graphics/Latin1TextIterator.h:30
> +    // The passed in UChar pointer starts at 'currentCharacter'. The iterator operatoes on the range [currentCharacter, lastCharacter].

Typo: operatoes.

> Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.h:42
> +        if (character < 0x3041)

This should be a named constant.

> Source/WebCore/platform/graphics/WidthIterator.cpp:257
> +    if (int(m_currentCharacter) >= offset)

No need to cast: m_currentCharacter is an int.

[Michael Saboff]

(In reply to comment #4)
> (From update of attachment 163894 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163894&action=review
> 
> > Source/WebCore/platform/graphics/Latin1TextIterator.h:2
> > + * Copyright (C) Research In Motion Limited 2011. All rights reserved.
> 
> Why the RIM copyright, and no Apple copyright?
>
> If this is new code, please use the license @ http://www.webkit.org/coding/bsd-license.html.

The Apple copyright is above the RIM copyright.  This file is largely leveraged from SurrogatePairAwareTextIterator.h and therefore I kept the copyright from that file and added the Apple copyright after consulting.  Consulted with Dan on this.
 
> > Source/WebCore/platform/graphics/Latin1TextIterator.h:30
> > +    // The passed in UChar pointer starts at 'currentCharacter'. The iterator operatoes on the range [currentCharacter, lastCharacter].
> 
> Typo: operatoes.

Fixed.

> > Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.h:42
> > +        if (character < 0x3041)
> 
> This should be a named constant.

Added constant to WTF/wtf/unicode/CharacterNames.h.

> > Source/WebCore/platform/graphics/WidthIterator.cpp:257
> > +    if (int(m_currentCharacter) >= offset)
> 
> No need to cast: m_currentCharacter is an int.

m_currentCharacter is an unsigned (Source/WebCore/platform/graphics/WidthIterator.h line 62).
I changed this to
    if (m_currentCharacter >= static_cast<unsigned>(offset))
since we really want to change offset to unsigned.

[Michael Saboff]

Committed r128504: <http://trac.webkit.org/changeset/128504>