[webkit-reviews] review denied: [Bug 53213] Refactoring: Extract TextCheckerClient from EditorClient : [Attachment 82286] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied MORITA Hajime <morrita at google.com>'s
request for review:
Bug 53213: Refactoring: Extract TextCheckerClient from EditorClient
https://bugs.webkit.org/show_bug.cgi?id=53213

Attachment 82286: Patch
https://bugs.webkit.org/attachment.cgi?id=82286&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=82286&action=review

r- for some nits.  Almost there!

> Source/WebCore/ChangeLog:9
> +	   spellcheck related API which is splitted 

Typo: splitted should be split. (paste tense and missing period)

> Source/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:491
> -	   client->checkSpellingOfString(charData, charLength,
&misspellingLocation, &misspellingLength);
> +	   client->textChecker()->checkSpellingOfString(charData, charLength,
&misspellingLocation, &misspellingLength);

Given that "client" isn't used anywhere else, we should be calling
Editor::textChecker here.

> Source/WebCore/editing/SpellChecker.cpp:109
> -    m_client->requestCheckingOfString(this, m_requestSequence,
m_requestText);
> +    m_client->textChecker()->requestCheckingOfString(this,
m_requestSequence, m_requestText);

Should SpellChecker take TextCheckerClient instead of EditorClient (or maybe
both)?	It seems odd that this is the only place we access m_client but only
thing we do is to obtain its TextCheckerClient.

> Source/WebCore/editing/TextCheckingHelper.cpp:183
> -	       m_client->checkSpellingOfString(chars, len,
&misspellingLocation, &misspellingLength);
> +	       m_client->textChecker()->checkSpellingOfString(chars, len,
&misspellingLocation, &misspellingLength);

Same comment about m_client used only to obtain TextCheckerClient.  But I guess
calls updateSpellingUIWithGrammarString and updateSpellingUIWithMisspelledWord
prevent moving forward her.

> Source/WebCore/platform/text/TextCheckerClient.h:15
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE

We should probably use other version that doesn't explicitly say Apple since
the code in this file has been contributed by Apple, Nokia, & Google.