Bug 63319

Created attachment 98479 [details] Patch Simple refactoring patch.

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.

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

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.

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.

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

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

Created attachment 98484 [details] Patch v3

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.

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

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

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

(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.

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.

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

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

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

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 .........

Comment on attachment 98516 [details] Patch v5 r=me

Landed in r89690.

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.

(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 :-)