Bug 130679 – Replace deprecatedIsCollapsibleWhitespace with a new function that takes RenderObject

• Component: DOM
• Status: ASSIGNED

[Chang Shu]

When renderer is available, it's more accurate to use renderer.style()->isCollapsibleWhiteSpace. Replace deprecatedIsCollapsibleWhitespace with a new function isCollapsibleWhitespace that takes RenderObject as the 2nd parameter.

[Chang Shu]

Created attachment 227666 [details]
try patch

Just try this patch on the bot to see how many test cases fail or need rebaseline.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/editing/htmlediting.h:243:  The parameter name "c" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/dom/Position.cpp:1071:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 6 files


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

[Build Bot]

Comment on attachment 227666 [details]
try patch

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

New failing tests:
editing/pasteboard/paste-text-at-tabspan-001.html
editing/pasteboard/paste-before-tab-span.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 227666 [details]
try patch

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

New failing tests:
editing/pasteboard/paste-text-at-tabspan-001.html
editing/pasteboard/paste-before-tab-span.html

[Build Bot]

Created attachment 227696 [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

[Chang Shu]

Created attachment 227744 [details]
fix patch

[Build Bot]

Comment on attachment 227744 [details]
fix patch

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

New failing tests:
editing/pasteboard/insert-u-with-text-decoration-none.html
editing/pasteboard/paste-blockquote-2.html
editing/pasteboard/paste-text-006.html
editing/pasteboard/paste-text-007.html
editing/style/iframe-onload-crash-win.html
editing/style/iframe-onload-crash-unix.html
editing/pasteboard/display-block-on-spans.html
editing/pasteboard/insert-font-with-size-and-css.html
editing/style/iframe-onload-crash-mac.html
editing/undo/undo-paste-when-caret-is-not-in-range.html

[Build Bot]

Created attachment 227748 [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 227744 [details]
fix patch

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

New failing tests:
editing/pasteboard/insert-u-with-text-decoration-none.html
editing/pasteboard/paste-blockquote-2.html
editing/pasteboard/paste-text-006.html
editing/pasteboard/paste-text-007.html
fast/lists/drag-into-marker.html
editing/style/iframe-onload-crash-win.html
editing/style/iframe-onload-crash-unix.html
editing/pasteboard/display-block-on-spans.html
editing/pasteboard/insert-font-with-size-and-css.html
editing/style/iframe-onload-crash-mac.html
editing/undo/undo-paste-when-caret-is-not-in-range.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 227744 [details]
fix patch

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

New failing tests:
editing/pasteboard/insert-u-with-text-decoration-none.html
editing/pasteboard/paste-blockquote-2.html
editing/pasteboard/paste-text-006.html
editing/pasteboard/paste-text-007.html
fast/lists/drag-into-marker.html
editing/style/iframe-onload-crash-win.html
editing/style/iframe-onload-crash-unix.html
editing/pasteboard/display-block-on-spans.html
editing/pasteboard/insert-font-with-size-and-css.html
editing/style/iframe-onload-crash-mac.html
editing/undo/undo-paste-when-caret-is-not-in-range.html

[Build Bot]

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

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

[Chang Shu]

Created attachment 227758 [details]
fix patch

[Darin Adler]

Comment on attachment 227758 [details]
fix patch

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

Nice idea to take this on.

Making these changes must fix some bugs. We have to create test cases that demonstrate the fixes, not just make the code change and cross our fingers.

But also, this really doesn’t sufficiently tackle the problem.

> Source/WebCore/editing/TextIterator.cpp:559
> +        while (runEnd < end && (isCollapsibleWhiteSpace(rendererText[runEnd], &renderer) || rendererText[runEnd] == '\t'))

This code should not be changed. Calling the new function here simply slows the code down by adding an extra function call for no benefit!

> Source/WebCore/editing/TextIterator.cpp:561
> +        bool addSpaceForPrevious = m_lastTextNodeEndedWithCollapsedSpace && m_lastCharacter && !isCollapsibleWhiteSpace(m_lastCharacter, &renderer);

This code should not be changed. Calling the new function here simply slows the code down by adding an extra function call for no benefit!

> Source/WebCore/editing/TextIterator.cpp:567
> +            bool addSpaceForCurrent = runStart || (m_lastCharacter && !isCollapsibleWhiteSpace(m_lastCharacter, &renderer));

This code should not be changed. Calling the new function here simply slows the code down by adding an extra function call for no benefit!

> Source/WebCore/editing/TextIterator.cpp:575
> +        while (runEnd < end && !isCollapsibleWhiteSpace(rendererText[runEnd], &renderer))

This code should not be changed. Calling the new function here simply slows the code down by adding an extra function call for no benefit!

> Source/WebCore/editing/TextIterator.cpp:631
> +        if (needSpace && !isCollapsibleWhiteSpace(m_lastCharacter, &renderer) && m_lastCharacter) {

This code should not be changed. Calling the new function here simply slows the code down by adding an extra function call for no benefit!

> Source/WebCore/editing/TextIterator.cpp:911
> -        if (!renderer.style().isCollapsibleWhiteSpace(text[i]))
> +        if (!isCollapsibleWhiteSpace(text[i], &renderer))

This code should not be changed. Calling the new function here simply slows the code down by adding an extra function call for no benefit!

> Source/WebCore/editing/htmlediting.cpp:1325
> +    return (c == ' ' || c == '\n');

It’s not correct to rename this isCollapsibleWhiteSpace. When there is no renderer, it’s not correct to just “do it wrong”. Even when there is no renderer, there is style that can be computed. A correct version of this function requires that callers pass in the style.

Given that, this function still needs to be named deprecatedIsCollabsibleWhiteSpace until we solve that problem.

No need for the parentheses here.

> Source/WebCore/editing/htmlediting.h:243
> +bool isCollapsibleWhiteSpace(UChar, RenderObject* renderer);

See above for the reason that this can’t be renamed to remove the word deprecated.

[Chang Shu]

(In reply to comment #15)
> (From update of attachment 227758 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227758&action=review
> 
> Nice idea to take this on.
> 
> Making these changes must fix some bugs. We have to create test cases that demonstrate the fixes, not just make the code change and cross our fingers.
> 
> But also, this really doesn’t sufficiently tackle the problem.
> 
> > Source/WebCore/editing/TextIterator.cpp:559
> > +        while (runEnd < end && (isCollapsibleWhiteSpace(rendererText[runEnd], &renderer) || rendererText[runEnd] == '\t'))
> 
> This code should not be changed. Calling the new function here simply slows the code down by adding an extra function call for no benefit!
> 
> > Source/WebCore/editing/TextIterator.cpp:561
> > +        bool addSpaceForPrevious = m_lastTextNodeEndedWithCollapsedSpace && m_lastCharacter && !isCollapsibleWhiteSpace(m_lastCharacter, &renderer);
> 
> This code should not be changed. Calling the new function here simply slows the code down by adding an extra function call for no benefit!
> 
> > Source/WebCore/editing/TextIterator.cpp:567
> > +            bool addSpaceForCurrent = runStart || (m_lastCharacter && !isCollapsibleWhiteSpace(m_lastCharacter, &renderer));
> 
> This code should not be changed. Calling the new function here simply slows the code down by adding an extra function call for no benefit!
> 
> > Source/WebCore/editing/TextIterator.cpp:575
> > +        while (runEnd < end && !isCollapsibleWhiteSpace(rendererText[runEnd], &renderer))
> 
> This code should not be changed. Calling the new function here simply slows the code down by adding an extra function call for no benefit!
> 
> > Source/WebCore/editing/TextIterator.cpp:631
> > +        if (needSpace && !isCollapsibleWhiteSpace(m_lastCharacter, &renderer) && m_lastCharacter) {
> 
> This code should not be changed. Calling the new function here simply slows the code down by adding an extra function call for no benefit!
> 
> > Source/WebCore/editing/TextIterator.cpp:911
> > -        if (!renderer.style().isCollapsibleWhiteSpace(text[i]))
> > +        if (!isCollapsibleWhiteSpace(text[i], &renderer))
> 
> This code should not be changed. Calling the new function here simply slows the code down by adding an extra function call for no benefit!
> 
> > Source/WebCore/editing/htmlediting.cpp:1325
> > +    return (c == ' ' || c == '\n');
> 
> It’s not correct to rename this isCollapsibleWhiteSpace. When there is no renderer, it’s not correct to just “do it wrong”. Even when there is no renderer, there is style that can be computed. A correct version of this function requires that callers pass in the style.
> 
> Given that, this function still needs to be named deprecatedIsCollabsibleWhiteSpace until we solve that problem.
> 
> No need for the parentheses here.
> 
> > Source/WebCore/editing/htmlediting.h:243
> > +bool isCollapsibleWhiteSpace(UChar, RenderObject* renderer);
> 
> See above for the reason that this can’t be renamed to remove the word deprecated.

Right, Darin. I am not comfortable with this patch either. Would using a 'defaultRenderStyle' make sense when the renderer doesn't exist? You also mentioned a style can be computed when there's no renderer. Could you provide some details? Thanks!

[Darin Adler]

(In reply to comment #16)
> Would using a 'defaultRenderStyle' make sense when the renderer doesn't exist?

No.

> You also mentioned a style can be computed when there's no renderer. Could you provide some details? Thanks!

Node::computedStyle returns a non-null style for any node that is in an element that is in a document. Not sure if it’s efficient enough.