[webkit-reviews] review granted: [Bug 195837] Smart delete for paragraphs. : [Attachment 365393] 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 195837: Smart delete for paragraphs.
https://bugs.webkit.org/show_bug.cgi?id=195837

Attachment 365393: Patch

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




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