|
Description
Megan Gardner
2019-02-20 16:05:04 PST
Created attachment 362561 [details]
Patch
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.
Comment on attachment 362561 [details] Patch Attachment 362561 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11225287 Number of test failures exceeded the failure limit. 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
Comment on attachment 362561 [details] Patch Attachment 362561 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11225445 Number of test failures exceeded the failure limit. 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
Comment on attachment 362561 [details] Patch Attachment 362561 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11225334 Number of test failures exceeded the failure limit. 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. Comment on attachment 362561 [details] Patch Attachment 362561 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11226425 Number of test failures exceeded the failure limit. 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. Created attachment 362752 [details]
Patch
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 362752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362752&action=review > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1272 > + Nit: whitespace. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1275 > + Ditto. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1337 > + // <MMG> add a check for iOS You mean IMG? > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1338 > + Nit: whitespace. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1355 > + Nit: Whitespace. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1358 > + Position endUpstream = endOfInsertedContent.deepEquivalent().upstream(); > + > + Position startDownstream = startOfInsertedContent.deepEquivalent().downstream(); These things aren't used anywhere. Remove? > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1359 > + Nit: whitespace. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1361 > + Ditto. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1364 > + Ditto. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1369 > + applyCommandToComposite(SetSelectionCommand::create(VisibleSelection(startOfInsertedContent), FrameSelection::defaultSetSelectionOptions())); Just call setEndingSelection instead. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1378 > + applyCommandToComposite(SetSelectionCommand::create(VisibleSelection(endOfInsertedContent), FrameSelection::defaultSetSelectionOptions())); Ditto. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1382 > +} We need to restore endingSelection here in the case we've inserted paragraphs above. Created attachment 362948 [details]
Patch
Created attachment 363023 [details]
Patch
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. Created attachment 363050 [details]
Patch
(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? Comment on attachment 363050 [details] Patch Attachment 363050 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11298821 New failing tests: editing/pasteboard/5065605.html editing/pasteboard/4076267-3.html editing/pasteboard/do-not-copy-unnecessary-styles.html editing/pasteboard/paste-text-013.html editing/pasteboard/paste-list-003.html editing/pasteboard/paste-text-012.html editing/pasteboard/paste-list-002.html editing/pasteboard/pasting-into-p-should-not-nest-p.html editing/pasteboard/paste-text-005.html editing/pasteboard/preserve-line-break-at-end-of-pasted-content.html editing/pasteboard/paste-text-016.html editing/pasteboard/avoid-copying-body-with-background.html editing/pasteboard/paste-list-001.html editing/pasteboard/paste-text-011.html editing/pasteboard/paste-text-006.html editing/pasteboard/paste-text-014.html editing/pasteboard/paste-text-015.html editing/pasteboard/copy-paste-pre-line-content.html editing/pasteboard/paste-text-007.html 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 363148 [details]
Patch
Comment on attachment 363148 [details] Patch Attachment 363148 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11309836 New failing tests: editing/pasteboard/paste-text-016.html editing/pasteboard/paste-text-007.html editing/pasteboard/paste-text-006.html editing/pasteboard/paste-text-005.html editing/pasteboard/paste-text-014.html 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. Created attachment 364468 [details]
Patch
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. Comment on attachment 364468 [details] Patch Attachment 364468 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11483560 New failing tests: editing/pasteboard/paste-text-016.html 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 364496 [details]
Patch
Comment on attachment 364496 [details] Patch Attachment 364496 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11484697 New failing tests: editing/pasteboard/smart-paste-paragraph-003.html 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 364539 [details]
Patch
Comment on attachment 364539 [details] Patch Attachment 364539 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11490622 New failing tests: editing/pasteboard/smart-paste-paragraph-003.html 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
Comment on attachment 364539 [details] Patch Attachment 364539 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11490777 New failing tests: editing/pasteboard/smart-paste-paragraph-003.html 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
Created attachment 364548 [details]
Patch
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 364699 [details]
Patch
Created attachment 364876 [details]
Patch
Comment on attachment 364876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364876&action=review > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1144 > if ((isListHTMLElement(refNode.get()) || (isLegacyAppleStyleSpan(refNode.get()) && isListHTMLElement(refNode->firstChild()))) > - && blockStart && blockStart->renderer()->isListItem()) > + && blockStart && blockStart->renderer()->isListItem()) { Why don't we do: bool isInsertingIntoList = (isListHTMLElement(refNode.get()) || ...; if (isInsertingIntoList) refNode = ...; instead? Created attachment 365049 [details]
Patch
Comment on attachment 365049 [details] Patch Attachment 365049 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11554488 New failing tests: http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html 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
Created attachment 365097 [details]
Patch for landing
Comment on attachment 365097 [details] Patch for landing Clearing flags on attachment: 365097 Committed r243124: <https://trac.webkit.org/changeset/243124> All reviewed patches have been landed. Closing bug. |