[webkit-reviews] review granted: [Bug 57665] [Mac] WebCore need to notify AppKit spell checker after user has modified autocorrected text. : [Attachment 88065] Patch (v2)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jia Pu <jpu at apple.com>'s request for
review:
Bug 57665: [Mac] WebCore need to notify AppKit spell checker after user has
modified autocorrected text.
https://bugs.webkit.org/show_bug.cgi?id=57665

Attachment 88065: Patch (v2)
https://bugs.webkit.org/attachment.cgi?id=88065&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=88065&action=review

> Source/WebCore/dom/DocumentMarkerController.cpp:351
> +	       if (node == startContainer && node == endContainer) {
> +		   // The range spans only one node.
> +		   if (it->endOffset >
static_cast<unsigned>(range->startOffset()) && it->startOffset <
static_cast<unsigned>(range->endOffset()))
> +		       foundMarkers.append(*it);
> +	       } else {
> +		   if (node == startContainer) {
> +		       if (it->endOffset >
static_cast<unsigned>(range->startOffset()))
> +			   foundMarkers.append(*it);
> +		   } else if (node == endContainer) {
> +		       if (it->startOffset <
static_cast<unsigned>(range->endOffset()))
> +			   foundMarkers.append(*it);
> +		   } else
> +		       foundMarkers.append(*it);

I think that with continue you could write this with less repeated code:

    if (node == startContainer && it->endOffset <=
static_cast<unsigned>(range->startOffset()))
	continue;
    if (node == endContainer && it->startOffset >=
static_cast<unsigned>(range->endOffset()))
	continue;
    foundMarkers.append(*it);

Cuts it down from 13 lines of code to 5 and I think it doesn’t lose clarity.

If this logic was copied from another function, I suggest the same
simplification for that other function.

> Source/WebCore/editing/Editor.cpp:2451
> +			   Range* range = replacedRange.get();

I don’t think this local variable does us much good.

> Source/WebCore/editing/Editor.cpp:2784
> +	   DocumentMarker::MarkerType markerType = markerTypesToAdd[i];
> +	   Range* range = replacementRange.get();
>	   if (m_correctionPanelInfo.panelType ==
CorrectionPanelInfo::PanelTypeReversion)
> -	       markers->addMarker(replacementRange.get(), markerTypesToAdd[i]);

> -	   else
> -	       markers->addMarker(replacementRange.get(), markerTypesToAdd[i],
m_correctionPanelInfo.replacedString);
> +	       markers->addMarker(range, markerType);
> +	   else {
> +	       if (markerType == DocumentMarker::Replacement || markerType ==
DocumentMarker::Autocorrected)
> +		   markers->addMarker(range, markerType,
m_correctionPanelInfo.replacedString);
> +	       else
> +		   markers->addMarker(range, markerType);
> +	   }

I think the logic here could be streamlined a bit. The different branches of
the if statement look almost the same to me.