[webkit-reviews] review granted: [Bug 194880] Smart Insert for paragraphs. : [Attachment 364548] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 194880: Smart Insert for paragraphs.
https://bugs.webkit.org/show_bug.cgi?id=194880

Attachment 364548: Patch

https://bugs.webkit.org/attachment.cgi?id=364548&action=review




--- Comment #41 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 364548
  --> https://bugs.webkit.org/attachment.cgi?id=364548
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.