Bug 128233 – Change TextIterator to use StringView, preparing to wean it from deprecatedCharacters

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Darin Adler]

Change TextIterator to use StringView, preparing to wean it from deprecatedCharacters

[Darin Adler]

Created attachment 223458 [details]
Patch

[Anders Carlsson]

Comment on attachment 223458 [details]
Patch

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

> Source/WebCore/platform/text/mac/TextBoundaries.mm:219
> +    RetainPtr<CFStringRef> uniString = text.createCFStringWithoutCopying();
> +    RetainPtr<CFStringRef> foundWord = text.substring(*start, *end - *start).createCFStringWithoutCopying();

These can both be auto.

[Build Bot]

Comment on attachment 223458 [details]
Patch

Attachment 223458 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5326948951654400

New failing tests:
editing/pasteboard/bad-placeholder.html
editing/execCommand/createLink.html
editing/execCommand/button.html
editing/pasteboard/8145-2.html
editing/execCommand/findString-2.html
editing/inserting/insert-paragraph-03.html
editing/pasteboard/5006779.html
editing/selection/4932260-2.html
editing/inserting/4960120-2.html
editing/execCommand/6355786.html
editing/input/password-echo-passnode2.html
editing/execCommand/findString.html
editing/input/password-echo-passnode.html
editing/input/emacs-ctrl-o.html
editing/execCommand/hilitecolor.html
editing/inserting/insert-before-link-1.html
editing/pasteboard/4806874.html
editing/input/password-echo-passnode3.html
editing/deleting/delete-cell-contents.html
editing/selection/4983858.html
editing/execCommand/findString-3.html
editing/selection/4932260-3.html
editing/execCommand/indent-pre.html
editing/execCommand/findString-diacriticals.html
editing/deleting/delete-by-word-001.html
editing/deleting/delete-by-word-002.html
editing/inserting/insert-paragraph-04.html
editing/deleting/5300379.html
editing/pasteboard/4944770-1.html
editing/pasteboard/4242293-1.html

[Build Bot]

Created attachment 223464 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 223458 [details]
Patch

Attachment 223458 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5051042400043008

New failing tests:
editing/pasteboard/bad-placeholder.html
editing/execCommand/createLink.html
editing/execCommand/button.html
editing/pasteboard/8145-2.html
editing/execCommand/findString-2.html
editing/inserting/insert-paragraph-03.html
editing/pasteboard/5006779.html
editing/selection/4932260-2.html
editing/inserting/4960120-2.html
editing/execCommand/6355786.html
editing/input/password-echo-passnode2.html
editing/execCommand/findString.html
editing/input/password-echo-passnode.html
editing/input/emacs-ctrl-o.html
editing/execCommand/hilitecolor.html
editing/inserting/insert-before-link-1.html
editing/pasteboard/4806874.html
editing/input/password-echo-passnode3.html
editing/deleting/delete-cell-contents.html
editing/selection/4983858.html
editing/execCommand/findString-3.html
editing/selection/4932260-3.html
editing/execCommand/indent-pre.html
editing/execCommand/findString-diacriticals.html
editing/deleting/delete-by-word-001.html
editing/deleting/delete-by-word-002.html
editing/inserting/insert-paragraph-04.html
editing/deleting/5300379.html
editing/pasteboard/4944770-1.html
editing/pasteboard/4242293-1.html

[Build Bot]

Created attachment 223468 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 223458 [details]
Patch

Attachment 223458 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5275092456046592

New failing tests:
editing/selection/5195166-1.html
editing/execCommand/createLink.html
editing/execCommand/button.html
editing/execCommand/find-after-replace.html
editing/pasteboard/4944770-1.html
editing/execCommand/findString-2.html
editing/inserting/insert-paragraph-03.html
editing/selection/4932260-2.html
editing/inserting/4960120-2.html
editing/execCommand/6355786.html
editing/deleting/delete-by-word-002.html
editing/execCommand/findString.html
editing/input/password-echo-passnode.html
editing/input/emacs-ctrl-o.html
editing/execCommand/hilitecolor.html
editing/inserting/insert-before-link-1.html
editing/pasteboard/4806874.html
editing/input/password-echo-passnode3.html
editing/deleting/delete-cell-contents.html
editing/selection/4983858.html
editing/execCommand/findString-3.html
editing/selection/4932260-3.html
editing/execCommand/indent-pre.html
editing/execCommand/findString-diacriticals.html
editing/deleting/delete-by-word-001.html
editing/inserting/insert-paragraph-04.html
editing/deleting/5300379.html
editing/input/password-echo-passnode2.html
editing/pasteboard/4242293-1.html
editing/pasteboard/8145-2.html

[Build Bot]

Created attachment 223499 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Darin Adler]

Created attachment 223564 [details]
Patch

[Darin Adler]

Created attachment 223565 [details]
Patch

[Darin Adler]

Created attachment 223566 [details]
Patch

[Darin Adler]

Committed r163712: <http://trac.webkit.org/changeset/163712>

[Carlos Garcia Campos]

Comment on attachment 223566 [details]
Patch

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

While reviewing this patch because of bug #133047 I've found a couple of issues, although they didn't seem to cause the regression.

> Source/WebCore/editing/TextCheckingHelper.cpp:80
> -    int wordStart = textBreakCurrent(iterator);
> -    while (0 <= wordStart) {
> +    for (int wordStart = textBreakCurrent(iterator); wordStart > 0; ) {

The while loop condition was wordStart >= 0, shouldn't the for loop one be wordStart >= 0 too?

> Source/WebCore/editing/TextCheckingHelper.cpp:258
> -        if (!(len == 1 && chars[0] == ' ')) {
> -            
> +        if (textLength == 1 && text[0] == ' ') {

This was !() before, I guess we don't want to enter the if when the text is just one space.

[Darin Adler]

(In reply to comment #13)
> > Source/WebCore/editing/TextCheckingHelper.cpp:80
> > -    int wordStart = textBreakCurrent(iterator);
> > -    while (0 <= wordStart) {
> > +    for (int wordStart = textBreakCurrent(iterator); wordStart > 0; ) {
> 
> The while loop condition was wordStart >= 0, shouldn't the for loop one be wordStart >= 0 too?
>
> > Source/WebCore/editing/TextCheckingHelper.cpp:258
> > -        if (!(len == 1 && chars[0] == ' ')) {
> > -            
> > +        if (textLength == 1 && text[0] == ' ') {
> 
> This was !() before, I guess we don't want to enter the if when the text is just one space.

Yes, you are right about both.

I guess we don’t have any test coverage for either of these code paths. Ideally we’d make a regression test for this, but I think we should submit a patch right away to fix both even without a test. Would you be willing to do that, or would you like me to do it (or get someone else to do it)?

[Carlos Garcia Campos]

(In reply to comment #14)
> (In reply to comment #13)
> > > Source/WebCore/editing/TextCheckingHelper.cpp:80
> > > -    int wordStart = textBreakCurrent(iterator);
> > > -    while (0 <= wordStart) {
> > > +    for (int wordStart = textBreakCurrent(iterator); wordStart > 0; ) {
> > 
> > The while loop condition was wordStart >= 0, shouldn't the for loop one be wordStart >= 0 too?
> >
> > > Source/WebCore/editing/TextCheckingHelper.cpp:258
> > > -        if (!(len == 1 && chars[0] == ' ')) {
> > > -            
> > > +        if (textLength == 1 && text[0] == ' ') {
> > 
> > This was !() before, I guess we don't want to enter the if when the text is just one space.
> 
> Yes, you are right about both.
> 
> I guess we don’t have any test coverage for either of these code paths. Ideally we’d make a regression test for this, but I think we should submit a patch right away to fix both even without a test. Would you be willing to do that, or would you like me to do it (or get someone else to do it)?

I was indeed wrong, those changes (actually the second one) fix the regression in the GTK+ port, but I forgot to enable the spell checker when I tested it :-P I'll write the patch.