[webkit-reviews] review granted: [Bug 214648] Stop using live ranges in SpellChecker.h and TextCheckingHelper.h : [Attachment 405123] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Darin Adler <darin at apple.com>'s request
for review:
Bug 214648: Stop using live ranges in SpellChecker.h and TextCheckingHelper.h
https://bugs.webkit.org/show_bug.cgi?id=214648

Attachment 405123: Patch

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




--- Comment #5 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 405123
  --> https://bugs.webkit.org/attachment.cgi?id=405123
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405123&action=review

> Source/WebCore/ChangeLog:90
> +	   not mark when called just to find the first mispelled word. The old
version

mispelled -> misspelled

> Source/WebCore/dom/DocumentMarkerController.h:115
> +void addMarker(const SimpleRange&, DocumentMarker::MarkerType, const
DocumentMarker::Data& = { });
> +void addMarker(Text&, unsigned startOffset, unsigned length,
DocumentMarker::MarkerType, DocumentMarker::Data&& = { });

Why does one of these take the data as a const& and one as &&? I assume it has
something to do with what the underlying DocumentMarkerController does, but
still pretty odd.

> Source/WebCore/editing/Editor.h:326
> +    Optional<SimpleRange> markMisspellings(const VisibleSelection&); //
Returns first misspelling range.

Irony!

> Source/WebCore/editing/TextCheckingHelper.cpp:229
> +auto TextCheckingHelper::findMispelledWords(Operation operation) const ->
std::pair<MisspelledWord, Optional<SimpleRange>>

Typo: Should be "findMisspelledWords" (two s's in misspelled).

> Source/WebCore/editing/TextCheckingHelper.cpp:298
> +    Optional<SimpleRange> mispelledWordRange;

Typo: Should be "misspelledWordRange" (two s's in misspelled).

> Source/WebCore/editing/TextCheckingHelper.cpp:477
> +	   int badGrammarIndex = findUngrammaticalPhrases(operation,
grammarDetails, badGrammarPhraseLocation, paragraph.checkingStart(),
paragraph.checkingEnd());

For consistency, would be nice to rename this variable (and the ones like it)
to ungrammaticalPhraseIndex (or something like that).  Can't tell from the diff
how pervasive the usage of "badGrammar" vs. "ungrammaticalPhrase" is, so it
might not make sense for this patch.

> Source/WebCore/editing/TextCheckingHelper.h:79
>  class TextCheckingHelper {

One day, this class could use a better name. Helper of whom? To what end?

> Source/WebCore/editing/TextCheckingHelper.h:92
> +    struct MisspelledWord {
> +	   String word;
> +	   uint64_t offset { 0 };
> +    };
> +    struct UngrammaticalPhrase {
> +	   String phrase;
> +	   uint64_t offset { 0 };
> +	   GrammarDetail detail;
> +    };
> +

I like this a lot.

> Source/WebCore/platform/text/TextChecking.h:80
> +    bool mispelled { false };

Should be "misspelled". (this feels like a trap left in to test reviewers ;) ).