Bug 214648 – Stop using live ranges in SpellChecker.h and TextCheckingHelper.h

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Darin Adler]

Stop using live ranges in SpellChecker.h and TextCheckingHelper.h

[Darin Adler]

Created attachment 404949 [details]
Patch

[Darin Adler]

Comment on attachment 404949 [details]
Patch

Looks like all the tests passed.

[Darin Adler]

Created attachment 405123 [details]
Patch

[Darin Adler]

OK, *this* time it looks like all the tests are passing.

[Sam Weinig]

Comment on attachment 405123 [details]
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 ;) ).

[Darin Adler]

Comment on attachment 405123 [details]
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

I always misspell that. Have to fix it afterward with search and replace.

>> Source/WebCore/dom/DocumentMarkerController.h:115
>> +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.

Completely arbitrary because of what the caller does. Just matching the DocumentMarkerController functions, with the same arbitrary difference. For now I will leave it alone unless you have an idea which way I should go in regularizing it.

>> 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).

Will fix.

>> Source/WebCore/editing/TextCheckingHelper.cpp:298
>> +    Optional<SimpleRange> mispelledWordRange;
> 
> Typo: Should be "misspelledWordRange" (two s's in misspelled).

Will fix.

>> 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.

I’d prefer not to try to fix this terminology in this patch. There’s a lot of code using both terms.

>> Source/WebCore/editing/TextCheckingHelper.h:79
>>  class TextCheckingHelper {
> 
> One day, this class could use a better name. Helper of whom? To what end?

I could not agree more. It’s also the name of the source file! I’m sure it’s a class for helper utilities, for managing infrastructure.

>> Source/WebCore/platform/text/TextChecking.h:80
>> +    bool mispelled { false };
> 
> Should be "misspelled". (this feels like a trap left in to test reviewers ;) ).

If you saw how many times I misspelled misspelled while working on this, over and over again, you would be amazed.

[Darin Adler]

Committed r264905: <https://trac.webkit.org/changeset/264905>

[Radar WebKit Bug Importer]

<rdar://problem/66133407>