[Webkit-unassigned] [Bug 74071] [Chromium] Chromium should have EditorClientImpl::checkTextOfParagraph

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=74071


Shinya Kawanaka <shinyak at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |shinyak at chromium.org




--- Comment #7 from Shinya Kawanaka <shinyak at chromium.org>  2011-12-13 03:58:36 PST ---
(In reply to comment #5)
> (From update of attachment 118740 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118740&action=review
> 
> > Source/WebKit/chromium/public/WebSpellCheckClient.h:44
> >  class WebSpellCheckClient {
> 
> By the way, it seems like WebSpellCheckClient is misnamed.  It should probably
> be called WebTextChecker.  Such a renaming is out-of-scope for this patch, but
> I just wanted to call attention to the issue.

I'll do this another patch if necessary.

> 
> > Source/WebKit/chromium/public/WebSpellCheckClient.h:57
> > +    virtual void checkTextOfParagraph(const WebString& text,
> 
> This appears to run synchronously, like spellCheck.  Should we have an asynchronous
> version like requestCheckingOfText?  Also, what about the optionalSuggestions out-
> param of spellCheck?  You don't need a way to provide suggestions with this new API?

I have a plan to add asynchronous version after this patch is landed.
I want to have the same interface WebCore::TextCheckerClient::checkTextOfParagraph. So optional suggestions is not mandatory. But we can specify adding auto correction list by WebTextCheckingTypeMask.

> 
> It seems like WebSpellCheckClient is a bit of a mess :-(
> 
> > Source/WebKit/chromium/public/WebTextChecking.h:31
> > +#ifndef WebTextChecking_h
> 
> nit: the file name should match the type name, so WebTextCheckingType.h would be better.

Done.

> 
> > Source/WebKit/chromium/public/WebTextChecking.h:36
> > +enum WebTextCheckingType {
> 
> WebTextCheckingResult has a similar enum.  Presumably, that one should be deleted
> in favor of this one?

Yeah, I want to keep WebTextCheckingType. So the previous enum should be removed. But removing it this time will break chromium build, so after this patch is landed, I'll change chromium code, then come back to WebKit to remove it.

> 
> > Source/WebKit/chromium/src/EditorClientImpl.cpp:769
> > +static void convertCheckingResult(const WebKit::WebTextCheckingResult& webResult,
> 
> nit: please do not break up the flow of class method definitions with non-class method definitions.
> please move static helper functions to the top of the file.
> 
> nit: you are already in the WebKit namespace, so no need for WebKit:: here.  also,
> there is a 'using namespace WebCore' at the top of the file, so no need for WebCore::
> here either.
> 
> nit: consider putting a conversion operator from WebTextCheckingResult to TextCheckingResult
> on WebTextCheckingResult in a #if WEBKIT_IMPLEMENTATION section.

I've added conversion operator.

> 
> > Source/WebKit/chromium/src/EditorClientImpl.cpp:793
> > +
> 
> perhaps you should assert that webMask is not 0 after this?  it seems like TextCheckingTypeMask
> could have other bits set too.

Done.

> > Source/WebKit/chromium/src/EditorClientImpl.cpp:795
> > +    m_webView->spellCheckClient()->checkTextOfParagraph(WebString(text, length), webMask, &webResults);
> 
> do we need to concern ourselves with the cost of constructing the WebString here?
> it seems like if we are doing a lot of spell checking that we might prefer to
> avoid the heap allocation here.  have you measured this to see if it matters?

Hmm... Currently SpellCheckClient uses WebString interface instead of UChar* and int. If it is important, we should change all the interface of SpellCheckClient, I guess. Currently it does not matter, I guess.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.