[webkit-reviews] review granted: [Bug 25330] Implement text checking/autocorrection with new SnowLeopard text checking API : [Attachment 29761] new patch to improve conformance to style guidelines

[bugzilla-daemon at webkit.org]

Justin Garcia <justin.garcia at apple.com> has granted Douglas Davidson
<ddavidso at apple.com>'s request for review:
Bug 25330: Implement text checking/autocorrection with new SnowLeopard text
checking API
https://bugs.webkit.org/show_bug.cgi?id=25330

Attachment 29761: new patch to improve conformance to style guidelines
https://bugs.webkit.org/attachment.cgi?id=29761&action=review

------- Additional Comments from Justin Garcia <justin.garcia at apple.com>
+    if (performTextChecking) {
+	 if (m_frame->selection()->selectionType() ==
VisibleSelection::CaretSelection) {
+	     RefPtr<Range> offsetAsRange =
Range::create(paragraphRange->startContainer(ec)->document(),
paragraphRange->startPosition(), paragraphRange->startPosition());
+	     offsetAsRange->setEnd(m_frame->selection()->end().containerNode(),
m_frame->selection()->end().computeOffsetInContainerNode(), ec);
+	     if (!ec) {
+		 selectionOffset =
TextIterator::rangeLength(offsetAsRange.get());
+		 restoreSelectionAfterChange = true;	     
+		 adjustSelectionForParagraphBoundaries = (selectionOffset >=
paragraphLength) ? true : false;
+	     }
+	 }
+    }

You only need to do this if there will be a replacement, but perhaps it would
be cumbersome to find that out here/do this later once we've found that we'll
be doing a replacement.


+ offsetAsRange->setEnd(m_frame->selection()->end().containerNode(),
m_frame->selection()->end().computeOffsetInContainerNode(), ec)

Would look nice split out:

+ Position caretPosition = m_frame->selection()->end();
+ offsetAsRange->setEnd(caretPosition.containerNode(),
caretPosition.computeOffsetInContainerNode(), ec);


"performTextChecking"

Is there any way we can make this bool more clearly describe that we're doing
full text checking, not just spelling/grammar checking?  spelling/grammar
checking sound like they'd also obey "performTextChecking".


+ } else if (performTextChecking && resultLocation + resultLength <=
spellingRangeEndOffset && resultLocation + resultLength >=
spellingRangeStartOffset &&

Should the check instead be resultLocation >= spellingRangeStartOffset?

I guess I'm slightly confused about what spellingRangeStartOffset actually is. 
It looks like (in the !markGrammar case at least) to be the offset into the
containing paragraph of the start of the spellingRange passed to this function.
 Is that right?


+ RefPtr<Range> replacementRange = TextIterator::subrange(paragraphRange.get(),
resultLocation, resultLength);
+ VisibleSelection replacementSelection(replacementRange.get(), DOWNSTREAM);

I think better names might be rangeToReplace and selectionToReplace.


+ } else {
+    m_frame->selection()->moveTo(m_frame->selection()->end());
+    m_frame->selection()->modify(SelectionController::MOVE,
SelectionController::FORWARD, CharacterGranularity);
+ }

This else case deserves a comment I think.


+void Editor::markMisspellingsAfterTypingToPosition(const VisiblePosition &p)

Did you need to move this.


 void TypingCommand::typingAddedToOpenCommand()
 {
-    markMisspellingsAfterTyping();
     document()->frame()->editor()->appliedEditing(this);
+    markMisspellingsAfterTyping();
 }

I think remember you explaining this somewhere but I couldn't find it.


enum TextCheckingType { TextCheckingTypeSpelling = 1 << 1,
TextCheckingTypeGrammar = 1 << 2, TextCheckingTypeLink = 1 << 5,
TextCheckingTypeQuote = 1 << 6, TextCheckingTypeDash = 1 << 7,
TextCheckingTypeReplacement = 1 << 8, TextCheckingTypeCorrection = 1 << 9 };

Please split this out onto separate lines like the other enum definitions in
WebCore.


#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) &&
!defined(BUILDING_ON_LEOPARD)

Could we get a BUILDING_ON_SNOWLEOPARD?


Otherwise, r=me.