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.
Created attachment 227666 [details] try patch Just try this patch on the bot to see how many test cases fail or need rebaseline.
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.
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
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
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
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
Created attachment 227744 [details] fix patch
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
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
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
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
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
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
Created attachment 227758 [details] fix patch
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.
(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!
(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.