Attachment 362561[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 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 362570[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 362571[details]
Archive of layout-test-results from ews103 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 362575[details]
Archive of layout-test-results from ews113 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 362561[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=362561&action=review> Source/WebCore/editing/CompositeEditCommand.h:155
> + void insertParagraphSeparatorAtPosition(const Position&, bool useDefaultParagraphElement = false, bool pasteBlockqutoeIntoUnquotedArea = false);
We don't want to do this. We want to update the selection & have insertParagraphSeparator adjust the selection as it mutates DOM.
Otherwise, we can end up with an orphaned position (i.e. position is no long in document).
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1273
> + addNewLinesForSmartReplace();
We definitely want to have a separate check for this feature.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1348
> + Node* endNode = endUpstream.computeNodeBeforePosition();
> + int endOffset = is<Text>(endNode) ? downcast<Text>(*endNode).length() : 0;
> + if (endUpstream.anchorType() == Position::PositionIsOffsetInAnchor) {
> + endNode = endUpstream.containerNode();
> + endOffset = endUpstream.offsetInContainerNode();
> + }
This doesn't look right. Why are getting these node & offset?
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1356
> + Position startDownstream = startOfInsertedContent.deepEquivalent().downstream();
> + Node* startNode = startDownstream.computeNodeAfterPosition();
> + unsigned startOffset = 0;
> + if (startDownstream.anchorType() == Position::PositionIsOffsetInAnchor) {
> + startNode = startDownstream.containerNode();
> + startOffset = startDownstream.offsetInContainerNode();
> + }
Ditto.
Created attachment 362581[details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 362561[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=362561&action=review> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1366
> + if (isEndOfLine(positionBeforeStart))
Doesn't this return true even when positionBeforeStart is on an empty line?
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1373
> + if (isStartOfLine(positionAfterEnd))
Ditto.
Attachment 362752[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 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 363023[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=363023&action=review> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1340
> + Element* textControl = enclosingTextFormControl(positionAtStartOfInsertedContent().deepEquivalent());
Do we really need this check? Also use auto?
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1359
> + Position endUpstream = endOfInsertedContent.deepEquivalent().upstream();
> +
> + Position startDownstream = startOfInsertedContent.deepEquivalent().downstream();
These two variables are never used. Remove?
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1372
> + setEndingSelection(VisibleSelection(startOfInsertedContent));
This is correct use of setEndingSelection but the conversion to VisibleSelection is implicit
so you can just do setEndingSelection(startOfInsertedContent).
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1382
> + setEndingSelection(VisibleSelection(endOfInsertedContent));
Ditto.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1388
> + }
As I commented in the previous patch, we probably need to restore ending selection here in the case we've modified above.
(In reply to Ryosuke Niwa from comment #18)
> Comment on attachment 363023[details]
> Patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363023&action=review
>
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1340
> > + Element* textControl = enclosingTextFormControl(positionAtStartOfInsertedContent().deepEquivalent());
>
> Do we really need this check? Also use auto?
The check for smart paste for spaces checks to make sure this isn't a password field. I figured we just don't want any input fields at all, but I can just check for password fields specifically, it just seems like we just shouldn't even even try to put newlines of any form in an input field.
>
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1359
> > + Position endUpstream = endOfInsertedContent.deepEquivalent().upstream();
> > +
> > + Position startDownstream = startOfInsertedContent.deepEquivalent().downstream();
>
> These two variables are never used. Remove?
>
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1372
> > + setEndingSelection(VisibleSelection(startOfInsertedContent));
>
> This is correct use of setEndingSelection but the conversion to
> VisibleSelection is implicit
> so you can just do setEndingSelection(startOfInsertedContent).
>
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1382
> > + setEndingSelection(VisibleSelection(endOfInsertedContent));
>
> Ditto.
>
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1388
> > + }
>
> As I commented in the previous patch, we probably need to restore ending
> selection here in the case we've modified above.
Ok, that's what I thought I was doing with " m_endOfInsertedContent = newEnd.deepEquivalent();" Is that now how to set the end of the selection? This is what I thought was doing the ending selection update in the insert smart spaces code... I'm not updating the start selection, though, that is probably something that I should do as well. Or is this not the right way to do this anymore?
Created attachment 363070[details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 363170[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 363148[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=363148&action=review> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1355
> +static bool isNewLine(VisiblePosition &p)
Please don't use one letter variable names such as p.
Also, this is wrong style. & should be next to VisiblePosition.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1359
> + if (isStartOfLine(p) && (startOfLine(p.next()) != startOfLine(p)))
> + return true;
> + return false;
It seems like should just do this in single line? There is no need for a if statement.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1367
> + bool isFullParagraph = isStartOfParagraph(startOfInsertedContent) && isEndOfParagraph(endOfInsertedContent);
What does this check do? I don't get what you mean by "full paragraph".
We should either rid of this confusing local variable or come up with more a descriptive variable name instead.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1378
> + if (!isNewLine(positionBeforeStart) && !isNewLine(startOfInsertedContent) && isEndOfLine(positionBeforeStart) && !isEndOfEditableOrNonEditableContent(positionAfterEnd) && !isEndOfEditableOrNonEditableContent(endOfInsertedContent)) {
It seems that we should also check whether we're at the start of the editable region or not?
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1383
> + VisiblePosition newStart = startOfInsertedContent.previous(CannotCrossEditingBoundary, &reachedBoundaryStart);
> + if (!reachedBoundaryStart)
> + m_startOfInsertedContent = newStart.deepEquivalent();
A better change would be to use endingSelection().deepEquivalent().
The trouble here is that startOfInsertedContent may not be in the document after executing insertParagraphSeparator().
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1387
> + reachedBoundaryEnd = false;
Why is it okay to not re-set reachedBoundaryStart here?
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1391
> + if (isStartOfLine(positionAfterEnd) && !isEndOfLine(positionAfterEnd) && !isNewLine(positionAfterEnd) && !isNewLine(endOfInsertedContent)) {
Ditto.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1396
> + VisiblePosition newEnd = endOfInsertedContent.next(CannotCrossEditingBoundary, &reachedBoundaryEnd);
> + if (!reachedBoundaryEnd)
> + m_endOfInsertedContent = newEnd.deepEquivalent();
Ditto about restoring m_endOfInsertedContent from endingSelection instead.
Comment on attachment 364468[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=364468&action=review> Source/WebCore/ChangeLog:8
> + Reviewed by NOBODY (OOPS!).
Nit: This line should appear before the description but after the bug URL.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:781
> +static bool isNewLine(VisiblePosition& position)
I'd prefer calling this isBlankLine.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:783
> + return isStartOfLine(position) && (startOfLine(position.next()) != startOfLine(position));
Nit: We don't need parenthesis here.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1085
> + if (!reachedBoundaryStart && !reachedBoundaryEnd && isNewLine(visibleInsertionPos) && isEndOfLine(positionBeforeInsertion) && isStartOfLine(positionAfterInsertion))
> + hasBlankLinesBetweenParagraphs = true;
Let's extract this as a helper function like isBlankLineBetweenParagraphs.
Also, why don't we define a local bool like hasLineBeforePosition = isEndOfLine(positionBeforeInsertion)
so as to clarify the intent of the last two checks.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1144
> + isList = true;
> + }
> else {
Nit: } else {
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1390
> + VisiblePosition newStart = startOfInsertedContent.previous(CannotCrossEditingBoundary, &reachedBoundaryStart);
We still need to use VisiblePosition { endingSelection() }.previous() here. startOfInsertedContent could no longer be in the document.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1405
> + VisiblePosition newEnd = endOfInsertedContent.next(CannotCrossEditingBoundary, &reachedBoundaryEnd);
Ditto; endingSelection().
> LayoutTests/ChangeLog:7
> +
Please mention which test is testing pasting into a list.
In particular, we need to make sure we cover pasting into OL, UL, and between LI's
as well as nested list items.
We also need a test case where we paste a list.
Also we need tests for pasting into table cells. Let's make sure we have some tests.
Created attachment 364495[details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 364509[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 364542[details]
Archive of layout-test-results from ews103 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 364547[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
Comment on attachment 364548[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=364548&action=review> Source/WebCore/editing/ReplaceSelectionCommand.cpp:933
> +static bool hasBlankLineBetweenParagraphs(Position &position)
Nit: & should be next to Position as in Position&.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:937
> + VisiblePosition visiblePosition(position);
Nit: Use { ~ }?
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:939
> + VisiblePosition positionBeforeInsertion = visiblePosition.previous(CannotCrossEditingBoundary, &reachedBoundaryStart);
> + VisiblePosition positionAfterInsertion = visiblePosition.next(CannotCrossEditingBoundary, &reachedBoundaryStart);
It's strange to call this positionBeforeInsertion & positionAfterInsertion since we're not talking about any insertion point here.
How about previousPosition & nextPosition?
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:941
> +
Nit: whitespace.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1137
> + bool isList = false;
isList is a bit ambiguous here. What is a list?
I think we need to give it a more describe name like isPastingIntoList or isInsertingIntoList.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1357
> +
Nit: whitespace.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1360
> +
Nit: whitespace.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1362
> + if (is<HTMLInputElement>(textControl))
Do we really need this check? hasBlankLinesBetweenParagraphs should be false since there won't be an extra line no?
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1364
> +
Nit: whitespace.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1378
> + bool isParagraph = isStartOfParagraph(startOfInsertedContent) && isEndOfParagraph(endOfInsertedContent);
What is a paragraph? How about something like isPastedContentEntireParagraphs?
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1380
> + // if we aren't pasting a paragraph, no need to attempt to insert newlines.
Nit: Capitalize "if".
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1390
> + if (!isBlankLine(positionBeforeStart) && !isBlankLine(startOfInsertedContent) && isEndOfLine(positionBeforeStart) && !isEndOfEditableOrNonEditableContent(positionAfterEnd) && !isEndOfEditableOrNonEditableContent(endOfInsertedContent)) {
It seems that !isEndOfEditableOrNonEditableContent(positionAfterEnd) should be checked for the end case instead??
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1393
> + VisiblePosition newStart = VisiblePosition(endingSelection().start()).previous(CannotCrossEditingBoundary, &reachedBoundaryStart);
Use endingSelection().visibleStart() instead. Also, use auto?
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1398
> +
Nit: Whitespace.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1403
> +
Nit: Whitespace.
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1405
> + if (isStartOfLine(positionAfterEnd) && !isEndOfLine(positionAfterEnd) && !isBlankLine(positionAfterEnd) && !isBlankLine(endOfInsertedContent)) {
Can we order the checks in the same order as the start case?
Also, don't we need to check !isEndOfEditableOrNonEditableContent(positionAfterEnd) here?
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1408
> + if (!reachedBoundaryEnd)
reachedBoundaryStart check here seems bogus. Remove?
> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1409
> + m_endOfInsertedContent = endingSelection().start();
Use visibleEnd() maybe? visibleStart() is okay too.
> LayoutTests/editing/pasteboard/smart-paste-paragraph-001.html:55
> +async function editingTest() {
Why not enable iOS editing behavior manually so that we can always test this:
internals.settings.setEditingBehavior("ios");
Ditto for all other tests.
Created attachment 365076[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
2019-02-20 16:10 PST, Megan Gardner
2019-02-20 16:51 PST, EWS Watchlist
2019-02-20 16:59 PST, EWS Watchlist
2019-02-20 17:06 PST, EWS Watchlist
2019-02-20 18:24 PST, EWS Watchlist
2019-02-22 12:45 PST, Megan Gardner
2019-02-25 18:09 PST, Megan Gardner
2019-02-26 14:45 PST, Megan Gardner
2019-02-26 18:11 PST, Megan Gardner
2019-02-26 22:12 PST, EWS Watchlist
2019-02-27 15:49 PST, Megan Gardner
2019-02-27 17:57 PST, EWS Watchlist
2019-03-12 16:17 PDT, Megan Gardner
2019-03-12 19:27 PDT, EWS Watchlist
2019-03-12 19:38 PDT, Megan Gardner
2019-03-12 21:36 PDT, EWS Watchlist
2019-03-13 09:16 PDT, Megan Gardner
2019-03-13 10:12 PDT, EWS Watchlist
2019-03-13 10:39 PDT, EWS Watchlist
2019-03-13 10:42 PDT, Megan Gardner
2019-03-14 15:17 PDT, Megan Gardner
2019-03-15 16:37 PDT, Megan Gardner
2019-03-18 13:12 PDT, Megan Gardner
2019-03-18 15:25 PDT, EWS Watchlist
2019-03-18 18:17 PDT, Megan Gardner