Smart delete for paragraphs.
Created attachment 364890 [details] Patch
Attachment 364890 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 364890 [details] Patch Attachment 364890 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11526722 New failing tests: editing/deleting/5433862-2.html editing/deleting/5126166.html
Created attachment 364913 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 365088 [details] Patch
Attachment 365088 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 365114 [details] Patch
<rdar://problem/48813963>
Comment on attachment 365114 [details] Patch Attachment 365114 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11558425 New failing tests: editing/deleting/smart-delete-paragraph-001.html editing/deleting/smart-delete-paragraph-003.html editing/deleting/smart-delete-paragraph-002.html editing/deleting/smart-delete-paragraph-004.html
Created attachment 365122 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 365114 [details] Patch Attachment 365114 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11558927 New failing tests: editing/deleting/smart-delete-paragraph-001.html editing/deleting/smart-delete-paragraph-003.html editing/deleting/smart-delete-paragraph-002.html editing/deleting/smart-delete-paragraph-004.html
Created attachment 365125 [details] Archive of layout-test-results from ews112 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 365114 [details] Patch Attachment 365114 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11559303 New failing tests: editing/deleting/smart-delete-paragraph-001.html editing/deleting/smart-delete-paragraph-003.html editing/deleting/smart-delete-paragraph-002.html editing/deleting/smart-delete-paragraph-004.html
Created attachment 365127 [details] Archive of layout-test-results from ews106 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 365114 [details] Patch Attachment 365114 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11559241 New failing tests: editing/deleting/delete-3959464-fix.html
Created attachment 365131 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 365216 [details] Patch
Comment on attachment 365216 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365216&action=review I think we need one more iteration before we can land this patch. > Source/WebCore/editing/DeleteSelectionCommand.cpp:179 > + Nit: Whitespace. Also, please share this code with ReplaceSelectionCommand by moving it to VisibleUnits.h/cpp Also, I've started to think that what we need to be checking is isStartOfParagraph. X of line really is about soft wrapping, which shouldn't affect smart paragraph replacement / deletion. So this should really be called isBlankParagraph. > Source/WebCore/editing/DeleteSelectionCommand.cpp:274 > + if (document().editingBehavior().shouldSmartInsertDeleteParagraphs()) { The content of this if statement should be moved to a separate member function. > Source/WebCore/editing/DeleteSelectionCommand.cpp:275 > + VisiblePosition visStart(m_upstreamStart); Use { ~ } to initialize start & end. Also, please don't abbreviate visual as "vis". > Source/WebCore/editing/DeleteSelectionCommand.cpp:282 > + bool nextPositionIsBlankLine = isBlankLine(visEnd); "next position" is ambiguous here because it doesn't specify next to what. It should probably say endPositonIsBlankLine. > Source/WebCore/editing/DeleteSelectionCommand.cpp:284 > + bool isSameElement = unsplittableElementForPosition(visStart.deepEquivalent()) == unsplittableElementForPosition(visEnd.deepEquivalent()); This is not a descriptive name. Should say startAndEndInSameUnsplittableElement. > Source/WebCore/editing/DeleteSelectionCommand.cpp:285 > + Nit: Whitespace. > Source/WebCore/editing/DeleteSelectionCommand.cpp:290 > + pos = VisiblePosition(m_downstreamEnd, VP_DEFAULT_AFFINITY).next().next().deepEquivalent(); No need to specify VP_DEFAULT_AFFINITY. The default argument is VP_DEFAULT_AFFINITY. We should instead do: endOfParagraph(startOfNextParagraph((m_downstreamEnd)); or something. > Source/WebCore/editing/DeleteSelectionCommand.cpp:295 > + m_trailingWhitespace = m_downstreamEnd.trailingWhitespacePosition(VP_DEFAULT_AFFINITY); We don't care about affinity here because we know m_downstreamEnd is at a blank paragraph. > Source/WebCore/editing/DeleteSelectionCommand.cpp:300 > + VisiblePosition visiblePos = VisiblePosition(m_upstreamStart, VP_DEFAULT_AFFINITY).previous().previous(); We should probably be using startOfParagraph(VisiblePosition { m_upstreamStart }.previous()) > Source/WebCore/editing/DeleteSelectionCommand.cpp:301 > + pos = visiblePos.deepEquivalent(); Please declare a new local variable here. It's confusing to be re-using variable declared above like this. State dependency like that can lead to surprising bugs when someone else come in & modify code in the future. > Source/WebCore/editing/DeleteSelectionCommand.cpp:304 > + m_leadingWhitespace = m_upstreamStart.leadingWhitespacePosition(visiblePos.affinity()); No need to pass in visiblePos.affinity here. We know it's always DOWNSTREAM because we're at a blank paragraph.
Comment on attachment 365216 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365216&action=review >> Source/WebCore/editing/DeleteSelectionCommand.cpp:295 >> + m_trailingWhitespace = m_downstreamEnd.trailingWhitespacePosition(VP_DEFAULT_AFFINITY); > > We don't care about affinity here because we know m_downstreamEnd is at a blank paragraph. Apparently we need to put something, so default seems as good as any.
Created attachment 365393 [details] Patch
Comment on attachment 365393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365393&action=review > Source/WebCore/editing/DeleteSelectionCommand.cpp:175 > +static bool isBlankParagraph(VisiblePosition& position) We should share this code with ReplaceSelectionCommand as I mentioned earlier. > Source/WebCore/editing/DeleteSelectionCommand.cpp:280 > + VisiblePosition visibleStart { m_upstreamStart }; We should put this into a separate member function as I commented earlier. > Source/WebCore/editing/DeleteSelectionCommand.cpp:283 > + bool selectionEndIsEndOfContent = endOfEditableContent(visibleEnd) == visibleEnd ? true : false; Just do: selectionEndIsEndOfContent = endOfEditableContent(visibleEnd) == visibleEnd > Source/WebCore/editing/DeleteSelectionCommand.cpp:290 > + if (startAndEndInSameUnsplittableElement && previousPositionIsBlankParagraph && (!selectionEndIsEndOfContent && (endPositonIsBlankParagraph || selectionEndsInParagraphSeperator))) { Why don't we define some helper local booleans like: bool hasBlankParagraphAfterEnd = !selectionEndIsEndOfContent && (endPositonIsBlankParagraph || selectionEndsInParagraphSeperator); to make the condition more obvious? > Source/WebCore/editing/DeleteSelectionCommand.cpp:296 > + position = VisiblePosition(m_downstreamEnd, VP_DEFAULT_AFFINITY).next().deepEquivalent(); No need to specify VP_DEFAULT_AFFINITY. That's the default value. > Source/WebCore/editing/DeleteSelectionCommand.cpp:302 > + if (selectionEndIsEndOfContent && previousPositionIsBlankParagraph && selectionEndsInParagraphSeperator) { We need to check startAndEndInSameUnsplittableElement here as well. Maybe we can add a test? > Source/WebCore/editing/DeleteSelectionCommand.cpp:304 > + VisiblePosition visiblePos = endOfParagraph(VisiblePosition { m_upstreamStart }.previous().previous()); I think we should use a more descriptive name like endOfParagraphBeforeStart
Created attachment 365409 [details] Patch
Created attachment 365425 [details] Patch
Comment on attachment 365425 [details] Patch Attachment 365425 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11587981 New failing tests: editing/deleting/smart-delete-paragraph-004.html
Created attachment 365447 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 365393 [details] Patch Attachment 365393 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11588287 New failing tests: http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html
Created attachment 365454 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 365425 [details] Patch Attachment 365425 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11589378 New failing tests: editing/deleting/smart-delete-paragraph-004.html
Created attachment 365463 [details] Archive of layout-test-results from ews117 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 365425 [details] Patch Attachment 365425 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11589941 New failing tests: editing/deleting/smart-delete-paragraph-004.html
Created attachment 365465 [details] Archive of layout-test-results from ews201 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment on attachment 365425 [details] Patch Attachment 365425 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11590262 New failing tests: editing/deleting/smart-delete-paragraph-004.html
Created attachment 365466 [details] Archive of layout-test-results from ews105 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 365478 [details] Patch
Created attachment 365559 [details] Patch for landing
Comment on attachment 365559 [details] Patch for landing Clearing flags on attachment: 365559 Committed r243296: <https://trac.webkit.org/changeset/243296>
All reviewed patches have been landed. Closing bug.