Bug 53213 - Refactoring: Extract TextCheckerClient from EditorClient
Summary: Refactoring: Extract TextCheckerClient from EditorClient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 53212
  Show dependency treegraph
 
Reported: 2011-01-26 17:32 PST by Hajime Morrita
Modified: 2011-02-15 00:28 PST (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-01-26 17:32:14 PST
This is required for implement hunspell-based spellchecker.
Comment 1 Hajime Morrita 2011-01-27 00:35:48 PST
Created attachment 80300 [details]
Patch
Comment 2 Hajime Morrita 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 WebKit Review Bot 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 WebKit Review Bot 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 Hajime Morrita 2011-01-27 00:58:34 PST
Created attachment 80302 [details]
Attempt to fix the build failure
Comment 6 WebKit Review Bot 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 WebKit Review Bot 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 Build Bot 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 Hajime Morrita 2011-01-27 01:12:00 PST
Created attachment 80304 [details]
Another trial...
Comment 10 Build Bot 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 Build Bot 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 Hajime Morrita 2011-01-27 03:35:25 PST
Created attachment 80314 [details]
Patch
Comment 13 Hajime Morrita 2011-01-27 15:34:50 PST
Purple bot looks due to the delay. I think it's ready for a review.
Comment 14 Hajime Morrita 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 Ryosuke Niwa 2011-02-03 00:09:39 PST
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?
Comment 16 Hajime Morrita 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 Ryosuke Niwa 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 Hajime Morrita 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 Ryosuke Niwa 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 Ryosuke Niwa 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 Ryosuke Niwa 2011-02-03 06:31:00 PST
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.
Comment 22 Ryosuke Niwa 2011-02-10 19:43:55 PST
Comment on attachment 80314 [details]
Patch

r- per my comments.
Comment 23 Hajime Morrita 2011-02-13 21:42:11 PST
Created attachment 82286 [details]
Patch
Comment 24 Hajime Morrita 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])
> 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 Ryosuke Niwa 2011-02-14 05:51:39 PST
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.
Comment 26 Hajime Morrita 2011-02-14 19:50:53 PST
Created attachment 82409 [details]
Patch
Comment 27 Hajime Morrita 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 Ryosuke Niwa 2011-02-14 20:07:43 PST
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.
Comment 29 Hajime Morrita 2011-02-15 00:28:41 PST
Committed r78533: <http://trac.webkit.org/changeset/78533>