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.
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
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 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
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
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
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
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.
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 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
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
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
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
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
2019-03-15 17:38 PDT, Megan Gardner
2019-03-15 21:33 PDT, EWS Watchlist
2019-03-18 16:32 PDT, Megan Gardner
2019-03-18 20:21 PDT, Megan Gardner
2019-03-18 21:24 PDT, EWS Watchlist
2019-03-18 23:03 PDT, EWS Watchlist
2019-03-18 23:56 PDT, EWS Watchlist
2019-03-19 00:20 PDT, EWS Watchlist
2019-03-19 13:04 PDT, Megan Gardner
2019-03-20 13:45 PDT, Megan Gardner
2019-03-20 15:10 PDT, Megan Gardner
2019-03-20 16:04 PDT, Megan Gardner
2019-03-20 17:33 PDT, EWS Watchlist
2019-03-20 18:00 PDT, EWS Watchlist
2019-03-20 19:23 PDT, EWS Watchlist
2019-03-20 19:56 PDT, EWS Watchlist
2019-03-20 20:08 PDT, EWS Watchlist
2019-03-20 21:00 PDT, Megan Gardner
2019-03-21 09:40 PDT, Megan Gardner