Bug 53213 - Refactoring: Extract TextCheckerClient from EditorClient
: Refactoring: Extract TextCheckerClient from EditorClient
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 53212
  Show dependency treegraph
 
Reported: 2011-01-26 17:32 PST by
Modified: 2011-02-15 00:28 PST (History)


Attachments
Patch (41.83 KB, patch)
2011-01-27 00:35 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Attempt to fix the build failure (43.08 KB, patch)
2011-01-27 00:58 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Another trial... (43.49 KB, patch)
2011-01-27 01:12 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (44.36 KB, patch)
2011-01-27 03:35 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (48.98 KB, patch)
2011-02-13 21:42 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (51.24 KB, patch)
2011-02-14 19:50 PST, Hajime Morrita
rniwa: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-01-26 17:32:14 PST
This is required for implement hunspell-based spellchecker.
------- Comment #1 From 2011-01-27 00:35:48 PST -------
Created an attachment (id=80300) [details]
Patch
------- Comment #2 From 2011-01-27 00:41:59 PST -------
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.
------- Comment #3 From 2011-01-27 00:51:08 PST -------
Attachment 80300 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7540377
------- Comment #4 From 2011-01-27 00:54:13 PST -------
Attachment 80300 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7621356
------- Comment #5 From 2011-01-27 00:58:34 PST -------
Created an attachment (id=80302) [details]
Attempt to fix the build failure
------- Comment #6 From 2011-01-27 01:05:51 PST -------
Attachment 80302 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7535332
------- Comment #7 From 2011-01-27 01:07:02 PST -------
Attachment 80302 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7544351
------- Comment #8 From 2011-01-27 01:09:40 PST -------
Attachment 80300 [details] did not build on win:
Build output: http://queues.webkit.org/results/7578378
------- Comment #9 From 2011-01-27 01:12:00 PST -------
Created an attachment (id=80304) [details]
Another trial...
------- Comment #10 From 2011-01-27 01:36:00 PST -------
Attachment 80302 [details] did not build on win:
Build output: http://queues.webkit.org/results/7493380
------- Comment #11 From 2011-01-27 01:37:05 PST -------
Attachment 80304 [details] did not build on win:
Build output: http://queues.webkit.org/results/7598374
------- Comment #12 From 2011-01-27 03:35:25 PST -------
Created an attachment (id=80314) [details]
Patch
------- Comment #13 From 2011-01-27 15:34:50 PST -------
Purple bot looks due to the delay. I think it's ready for a review.
------- Comment #14 From 2011-02-02 23:57:33 PST -------
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.
------- Comment #15 From 2011-02-03 00:09:39 PST -------
(From update of attachment 80314 [details])
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?
------- Comment #16 From 2011-02-03 00:37:47 PST -------
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).
------- Comment #17 From 2011-02-03 00:56:07 PST -------
(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.
------- Comment #18 From 2011-02-03 01:35:16 PST -------
> 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?
------- Comment #19 From 2011-02-03 01:37:19 PST -------
(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.
------- Comment #20 From 2011-02-03 01:38:24 PST -------
(In reply to comment #19)
> Ok. I think TextCheckingClient seems fine then.

Oops, I meant to say TextCheckerClient seems fine.
------- Comment #21 From 2011-02-03 06:31:00 PST -------
(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.

> 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.
------- Comment #22 From 2011-02-10 19:43:55 PST -------
(From update of attachment 80314 [details])
r- per my comments.
------- Comment #23 From 2011-02-13 21:42:11 PST -------
Created an attachment (id=82286) [details]
Patch
------- Comment #24 From 2011-02-13 21:51:53 PST -------
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] [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.
------- Comment #25 From 2011-02-14 05:51:39 PST -------
(From update of attachment 82286 [details])
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.
------- Comment #26 From 2011-02-14 19:50:53 PST -------
Created an attachment (id=82409) [details]
Patch
------- Comment #27 From 2011-02-14 19:57:16 PST -------
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.
------- Comment #28 From 2011-02-14 20:07:43 PST -------
(From update of attachment 82409 [details])
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.
------- Comment #29 From 2011-02-15 00:28:41 PST -------
Committed r78533: <http://trac.webkit.org/changeset/78533>