[webkit-reviews] review denied: [Bug 130679] Replace deprecatedIsCollapsibleWhitespace with a new function that takes RenderObject : [Attachment 227758] fix patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Chang Shu <cshu at webkit.org>'s request
for review:
Bug 130679: Replace deprecatedIsCollapsibleWhitespace with a new function that
takes RenderObject
https://bugs.webkit.org/show_bug.cgi?id=130679

Attachment 227758: fix patch
https://bugs.webkit.org/attachment.cgi?id=227758&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.