[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
bugzilla-daemon at webkit.org
Mon Apr 4 09:49:43 PDT 2011
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.
More information about the webkit-reviews
mailing list