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

[bugzilla-daemon at webkit.org]

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


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #118740|review?                     |review-
               Flag|                            |




--- Comment #5 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2011-12-12 10:36:16 PST ---
(From update of attachment 118740)
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.

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

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.

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

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

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

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

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