Bug 53213 – Refactoring: Extract TextCheckerClient from EditorClient

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Hajime Morrita]

This is required for implement hunspell-based spellchecker.

[Hajime Morrita]

Created attachment 80300 [details]
Patch

[Hajime Morrita]

Hi, Darin, Ryosuke, could you take a look?
The change is the starting point of porting hunspell checker to WebCore (or WebCoreSupport),
which will help many ports.

[WebKit Review Bot]

Attachment 80300 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7540377

[WebKit Review Bot]

Attachment 80300 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7621356

[Hajime Morrita]

Created attachment 80302 [details]
Attempt to fix the build failure

[WebKit Review Bot]

Attachment 80302 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7535332

[WebKit Review Bot]

Attachment 80302 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7544351

[Build Bot]

Attachment 80300 [details] did not build on win:
Build output: http://queues.webkit.org/results/7578378

[Hajime Morrita]

Created attachment 80304 [details]
Another trial...

[Build Bot]

Attachment 80302 [details] did not build on win:
Build output: http://queues.webkit.org/results/7493380

[Build Bot]

Attachment 80304 [details] did not build on win:
Build output: http://queues.webkit.org/results/7598374

[Hajime Morrita]

Created attachment 80314 [details]
Patch

[Hajime Morrita]

Purple bot looks due to the delay. I think it's ready for a review.

[Hajime Morrita]

Could anyone take a look? Any feedback is appreciated.

This is a part of TextChecker sharing effort.
This change will separate platform API (TextCheckerClient) for TextChecker out from EditorClient.
I'll port hunspell-based spellchecker under WebKit/shared/WebCoreSupport/TextCheckerClientHunspell
once this change land.

[Ryosuke Niwa]

Comment on attachment 80314 [details]
Patch

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

> Source/WebCore/page/EditorClient.h:174
>      virtual void updateSpellingUIWithMisspelledWord(const String&) = 0;
>      virtual void showSpellingUI(bool show) = 0;
>      virtual bool spellingUIIsShowing() = 0;

Why are these methods not moved?  I'm having a trouble knowing what's the boundary between TextCheckingClient and EditorClient.  Could you clarify what you're trying to accomplish here?

[Hajime Morrita]

Hi Ryosuke, thank you for your response!

> 
> > Source/WebCore/page/EditorClient.h:174
> >      virtual void updateSpellingUIWithMisspelledWord(const String&) = 0;
> >      virtual void showSpellingUI(bool show) = 0;
> >      virtual bool spellingUIIsShowing() = 0;
> 
> Why are these methods not moved?  I'm having a trouble knowing what's the boundary between TextCheckingClient and EditorClient.  Could you clarify what you're trying to accomplish here?

TextCheckingClient is for abstracting out spell-checking backends like Hunspell, not for controlling GUI.
Because methods above are for controlling, GUI (pop up, etc), I left them on EditorClient.

 I assume that Spell-checkers have non-GUI backends, For Hunspell this is true.
NSSpellChecker is also separating its Panel UI to NSPanell instance (NSSpellChecker::spellingPanel).

[Ryosuke Niwa]

(In reply to comment #16)
> TextCheckingClient is for abstracting out spell-checking backends like Hunspell, not for controlling GUI.
> Because methods above are for controlling, GUI (pop up, etc), I left them on EditorClient.
>
>  I assume that Spell-checkers have non-GUI backends, For Hunspell this is true.
> NSSpellChecker is also separating its Panel UI to NSPanell instance (NSSpellChecker::spellingPanel).

Ok.  I think we need to give a better name to the class to reflect this concept.  How about SpellCheckingClient or SpellCheckDelegate?  Delegate might make more semantic sense here.

[Hajime Morrita]

> Ok.  I think we need to give a better name to the class to reflect this concept.  How about SpellCheckingClient or SpellCheckDelegate?  Delegate might make more semantic sense here.
Hmm...

The name "TextChecker" is actually from Cocoa.
http://codesearch.google.com/codesearch?as_q=NSTextChecker&hl=en&vert=chromium
The class doesn't only check spelling, but also check the grammar. So TextChecker looks appropriate. 

In my undestanding, "Client" prefix is a standard naming for classes that handle 
client or platform specific behavior. In WebCore, there are no "XxDelegate" class but 
manu "Client" classes. But I'm not sure the convention is appropriate for this case.
What do you think?

[Ryosuke Niwa]

(In reply to comment #18)
> The name "TextChecker" is actually from Cocoa.
> http://codesearch.google.com/codesearch?as_q=NSTextChecker&hl=en&vert=chromium
> The class doesn't only check spelling, but also check the grammar. So TextChecker looks appropriate. 

Ah I see. I wasn't aware of this.

> In my undestanding, "Client" prefix is a standard naming for classes that handle 
> client or platform specific behavior. In WebCore, there are no "XxDelegate" class but 
> manu "Client" classes. But I'm not sure the convention is appropriate for this case.
> What do you think?

Ok. I think TextCheckingClient seems fine then.

[Ryosuke Niwa]

(In reply to comment #19)
> Ok. I think TextCheckingClient seems fine then.

Oops, I meant to say TextCheckerClient seems fine.

[Ryosuke Niwa]

Comment on attachment 80314 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22447
> +				A7151BD812F1558F005A0F64 /* TextCheckerClient.h in Headers */,

You should put this in the right lexicological order.

> Source/WebCore/loader/EmptyClients.h:396
> -class EmptyEditorClient : public EditorClient {
> +class EmptyEditorClient : public EditorClient, public TextCheckerClient {

I'm not sure if EmptyEditorClient should be both EditorClient and TextCheckerClient.  I think we should have a separate EmptyTextCheckerClient.

> Source/WebCore/page/EditorClient.h:73
> +class TextCheckerClient;

Huh, shouldn't T come before V?

> Source/WebCore/page/EditorClient.h:163
> +    virtual TextCheckerClient* textChecker() = 0;

Is it common for a client to return another client?  If not, we should probably add textCheckingClient() to Editor, and let Page return TextCheckerClient.

[Ryosuke Niwa]

Comment on attachment 80314 [details]
Patch

r- per my comments.

[Hajime Morrita]

Created attachment 82286 [details]
Patch

[Hajime Morrita]

Ryosuke, thank you for reviewing again and I'm sorry for missing your last feedback.

(In reply to comment #21)
> (From update of attachment 80314 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80314&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:22447
> > +				A7151BD812F1558F005A0F64 /* TextCheckerClient.h in Headers */,
> 
> You should put this in the right lexicological order.
Fixed.

> 
> > Source/WebCore/loader/EmptyClients.h:396
> > -class EmptyEditorClient : public EditorClient {
> > +class EmptyEditorClient : public EditorClient, public TextCheckerClient {
> 
> I'm not sure if EmptyEditorClient should be both EditorClient and TextCheckerClient.  I think we should have a separate EmptyTextCheckerClient.
Defined EmptyTextCheckerClient and return it.

> 
> > Source/WebCore/page/EditorClient.h:73
> > +class TextCheckerClient;
> 
> Huh, shouldn't T come before V?
Oops. Fixed.

> 
> > Source/WebCore/page/EditorClient.h:163
> > +    virtual TextCheckerClient* textChecker() = 0;
> 
> Is it common for a client to return another client?  If not, we should probably add textCheckingClient() to Editor, and let Page return TextCheckerClient.
There isn't a common pattern. But here is a reason:
In contrast of other client objects which are owned by Page (PagecClient),
TextCheckerClient is owned by EditorClien   because of compatibility reason. 
(At this time EditorClient and TextChekerClient is a same object.)
If TextCheckerClient implementation is a standalone object, holding it on Page is reasonable.
But splitting not only interface abut also its implementation is too big to change at once.
I'd like to do it after all migration is done if it's good to do.

> we should probably add textCheckingClient() to Editor
Done this part.

[Ryosuke Niwa]

Comment on attachment 82286 [details]
Patch

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.

[Hajime Morrita]

Created attachment 82409 [details]
Patch

[Hajime Morrita]

Hi Ryosuke, thank you for your detailed review!

> > Source/WebCore/ChangeLog:9
> > +        spellcheck related API which is splitted 
> 
> Typo: splitted should be split. (paste tense and missing period)
Fixed. Good catch...

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

> 
> > 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.
> 
Good point. Fixed.

> > 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.
Yes. actually we/I need to consider the design around TextCheckingHelper and SpellChecker.
I want spell-checking concept out of Editor into its own class, but don't have clear idea yet.

> 
> > 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.
Replaced with "contributors" version.

[Ryosuke Niwa]

Comment on attachment 82409 [details]
Patch

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

r=me provided you fix copyright statement & a really long comment as described below.

> Source/WebCore/editing/SpellChecker.cpp:46
> -SpellChecker::SpellChecker(Frame* frame, EditorClient* client)
> +SpellChecker::SpellChecker(Frame* frame, TextCheckerClient* client)

Ah, beautiful! Only one line changed in this file.

> Source/WebCore/platform/text/TextCheckerClient.h:18
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.

Mn... you grabbed a wrong version here.  Take a look at EditingStyle.cpp.

> Source/WebCore/platform/text/TextCheckerClient.h:82
> +    // For spellcheckers that support multiple languages, it's often important to be able to identify the language in order to provide more accurate correction suggestions. Caller can pass in more text in "context" to aid such spellcheckers on language identification. Noramlly it's the text surrounding the "word" for which we are getting correction suggestions.

You should split this long sentence into several lines.  On IDEs that permit 1000+ columns per line, this line will be hard to read.

[Hajime Morrita]

Committed r78533: <http://trac.webkit.org/changeset/78533>