Bug 53213

Created attachment 80300 [details] Patch

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.

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

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

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

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

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

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

Created attachment 80304 [details] Another trial...

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

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

Created attachment 80314 [details] Patch

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

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 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?

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

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

> 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?

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

(In reply to comment #19) > Ok. I think TextCheckingClient seems fine then. Oops, I meant to say TextCheckerClient seems fine.

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 on attachment 80314 [details] Patch r- per my comments.

Created attachment 82286 [details] Patch

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

Created attachment 82409 [details] Patch

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

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