Bug 214648

Summary: Stop using live ranges in SpellChecker.h and TextCheckingHelper.h
Product: WebKit Reporter: Darin Adler <darin>
Component: New BugsAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cdumez, cfleizach, dmazzoni, esprehn+autocc, ews-watchlist, jcraig, jdiggs, kangil.han, mifenton, samuel_white, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch sam: review+

Description Darin Adler 2020-07-22 11:44:32 PDT
Stop using live ranges in SpellChecker.h and TextCheckingHelper.h
Comment 1 Darin Adler 2020-07-22 12:31:35 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2020-07-22 14:56:28 PDT Comment hidden (obsolete)
Comment 3 Darin Adler 2020-07-23 20:37:03 PDT
Created attachment 405123 [details]
Patch
Comment 4 Darin Adler 2020-07-23 21:33:33 PDT
OK, *this* time it looks like all the tests are passing.
Comment 5 Sam Weinig 2020-07-26 11:58:42 PDT
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 ;) ).
Comment 6 Darin Adler 2020-07-26 12:05:18 PDT
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.
Comment 7 Darin Adler 2020-07-26 12:12:36 PDT
Committed r264905: <https://trac.webkit.org/changeset/264905>
Comment 8 Radar WebKit Bug Importer 2020-07-26 12:13:15 PDT
<rdar://problem/66133407>