Bug 63319 – Refactor text iterator code respecting surrogate pairs from WidthIterator

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nikolas Zimmermann]

After the patch from bug 59085 landed we found out there's a regression with Acid3, that led to the rollout of the patch.
In order to fix the Acid3 regression I need to handle surrogate pairs correctly in the SVGFonts GlyphPage.

For this I need to make SVGGlyphMap UTF16 aware by using UChar32 instead of UChar in the glyph maps.
This works just fine and fixed the Acid3 regression.

To prepare resubmitting the patch for bug 59085, I'm uploading a preparation patch here soon that breaks out SurrogatePairAwareTextIterator out of WidthIterator so SVGGlyphMap can reuse it soon.

[Nikolas Zimmermann]

Created attachment 98479 [details]
Patch

Simple refactoring patch.

[WebKit Review Bot]

Attachment 98479 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.cpp:23:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.cpp:128:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.cpp:266:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 98479 [details]
Patch

Attachment 98479 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8937345

[Nikolas Zimmermann]

Created attachment 98481 [details]
Patch v2

Merging failed :( SurrogatePairAwareTextIterator.cpp contained a copy of WidthIterator.cpp in the first patch :( Hopefully it works now, otherwhise I'll have to clean my tree.

[WebKit Review Bot]

Attachment 98481 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.cpp:72:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.cpp:106:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 98481 [details]
Patch v2

Attachment 98481 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8934443

[WebKit Review Bot]

Comment on attachment 98481 [details]
Patch v2

Attachment 98481 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8931531

[Nikolas Zimmermann]

Created attachment 98484 [details]
Patch v3

[WebKit Review Bot]

Attachment 98484 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.cpp:72:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.cpp:106:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 98484 [details]
Patch v3

Attachment 98484 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8937366

New failing tests:
editing/selection/anchor-focus1.html
editing/selection/4895428-2.html
editing/selection/5007143-2.html
editing/execCommand/insert-list-with-noneditable-content.html
css2.1/t1604-c541-word-sp-01-b-a.html
editing/execCommand/move-selection-back-line.html
editing/selection/5232159.html
editing/pasteboard/4947130.html
editing/deleting/delete-line-012.html
editing/selection/5057506.html
editing/deleting/smart-delete-002.html
editing/deleting/4916235-1.html
editing/execCommand/findString-2.html
editing/deleting/pruning-after-merge-1.html
css1/text_properties/word_spacing.html
editing/deleting/delete-line-011.html
editing/pasteboard/4242293-1.html
editing/selection/14971.html
editing/selection/5007143.html
css2.1/t1604-c541-word-sp-00-b-a.html

[WebKit Review Bot]

Created attachment 98486 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dirk Schulze]

Comment on attachment 98484 [details]
Patch v3

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

The patch looks good. I wonder about the failing tests on chromium. I hope you run the complete DRT suite. r=me if it passes all tests on your machine.

>> Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.cpp:72
>> +    // Do we have a surrogate pair?  If so, determine the full Unicode (32 bit) code point before glyph lookup.
> 
> Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]

Thats right :-) Can you fix it before landing?

>> Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.cpp:106
>> +        if (resultLength == 1 && uStatus == 0)
> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

Should also be a simple fix

[Nikolas Zimmermann]

(In reply to comment #12)
> (From update of attachment 98484 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98484&action=review
> 
> The patch looks good. I wonder about the failing tests on chromium. I hope you run the complete DRT suite. r=me if it passes all tests on your machine.

Yes, there was still a bug with the patch, I'm uploading a new one to see if also the chromium bot thinks there are no regressions.

[Nikolas Zimmermann]

Created attachment 98494 [details]
Patch v4

Fix problem with stale variable, should fix all regressions. Please wait for cr-linux EWS before touching the r? state.

[WebKit Review Bot]

Comment on attachment 98494 [details]
Patch v4

Attachment 98494 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8935411

New failing tests:
editing/selection/anchor-focus1.html
editing/selection/4895428-2.html
editing/selection/5007143-2.html
editing/execCommand/insert-list-with-noneditable-content.html
editing/execCommand/move-selection-back-line.html
editing/selection/5232159.html
editing/pasteboard/4947130.html
editing/deleting/delete-line-012.html
editing/selection/caret-bidi-first-and-last-letters.html
editing/selection/5057506.html
editing/deleting/smart-delete-002.html
editing/deleting/4916235-1.html
editing/execCommand/findString-2.html
editing/deleting/pruning-after-merge-1.html
editing/selection/click-in-padding-with-multiple-line-boxes.html
editing/deleting/delete-line-011.html
editing/pasteboard/4242293-1.html
editing/selection/14971.html
editing/selection/5007143.html
editing/selection/click-in-margins-inside-editable-div.html

[WebKit Review Bot]

Created attachment 98495 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dirk Schulze]

Looks like the css tests pass now but other edit tests fail as well.

[Nikolas Zimmermann]

Created attachment 98516 [details]
Patch v5

Fixed last merging problem :( Another stale variable, this time I ran all tests and verified none regressed, on my machine!
Especially editing is fine now:
editing/deleting ...............................................................................................................................................................................................................
editing/editability ....
editing/execCommand ..................................................................................................................................................................................................................................
editing/input .............
editing/inserting ......................................................................................................................................................
editing/pasteboard ..............................................................................................................................................................................................................................
editing/selection .....................................................................................................................................................................................................................................................................................................................
editing/spelling ..............
editing/style ...........................................................................................
editing/text-iterator .....
editing/undo .................................
editing/unsupported-content .........

[Dirk Schulze]

Comment on attachment 98516 [details]
Patch v5

r=me

[Nikolas Zimmermann]

Landed in r89690.

[Darin Adler]

Comment on attachment 98516 [details]
Patch v5

Performance impact?

Confusing to call this a "text iterator" when that is the name of a very different beast in WebCore.

It's trivial to iterate surrogate pairs without needing an object. There are many code paths in many places in WebKit that do it; the macros in ICU’s UTF16.h make it almost trivial. As far as I can see what’s non trivial here is the additional normalization, specifically the handling of voicing marks. Because of that, I think the name of the class is wrong.

[Nikolas Zimmermann]

(In reply to comment #21)
> (From update of attachment 98516 [details])
> Performance impact?
> 
> Confusing to call this a "text iterator" when that is the name of a very different beast in WebCore.
I wanted to call it TextIterator first, but it clashed with that one.
> 
> It's trivial to iterate surrogate pairs without needing an object. There are many code paths in many places in WebKit that do it; the macros in ICU’s UTF16.h make it almost trivial. As far as I can see what’s non trivial here is the additional normalization, specifically the handling of voicing marks. Because of that, I think the name of the class is wrong.
You've got a very good point here. I'll be happy to change the name to anything better if you have an idea!
I thought about it for a while, and both Dirk and me couldn't come up with a better name, help appreciated :-)