[webkit-reviews] review granted: [Bug 187225] [macOS] Text replacements that end with symbols are expanded immediately : [Attachment 344042] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 187225: [macOS] Text replacements that end with symbols are expanded
immediately
https://bugs.webkit.org/show_bug.cgi?id=187225

Attachment 344042: Patch

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




--- Comment #10 from Darin Adler <darin at apple.com> ---
Comment on attachment 344042
  --> https://bugs.webkit.org/attachment.cgi?id=344042
Patch

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

> Source/WebCore/editing/Editor.h:296
> +    void markAllMisspellingsAndBadGrammarInRanges(TextCheckingTypeMask,
Range* spellingRange, Range* automaticReplacementRange, Range* grammarRange);

Why are these Range* instead of Range& or const Range&? Can all three be null?

> Source/WebCore/editing/TextCheckingHelper.h:82
> +    mutable int m_checkingStart { -1 };
> +    mutable int m_checkingEnd { -1 };
> +    mutable int m_checkingLength { -1 };
> +    mutable int m_automaticReplacementStart { -1 };
> +    mutable int m_automaticReplacementLength { -1 };

At some point we should consider using std::optional here instead of a magic
number of -1.

> Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.h:35
> + at interface LayoutTestTextCheckingResult : NSTextCheckingResult {

I think this class should be private and not mentioned in the header. The data
structures could just hold NSTextCheckingResult; no need to remember that our
special type was used once the object is created.

> Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.h:51
> ++ (instancetype)installIfNecessary;

Seems slightly unnecessarily wordy. Such methods often have simpler names like
"checker", with the "set this up so I can use it" part treated as implicit.

> Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.mm:36
> +typedef void (^TextCheckingCompletionHandler)(NSInteger sequenceNumber,
NSArray<NSTextCheckingResult *> *results, NSOrthography *orthography, NSInteger
wordCount);

New code should typically use "using" instead of "typedef".

> Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.mm:38
> +static NeverDestroyed<RetainPtr<LayoutTestSpellChecker>> globalSpellChecker;

Typically it’s overkill to use NeverDestroyed<RetainPtr>. Instead we just do a
leakRef when initializing the global or just skip the adoptNS.