[webkit-reviews] review denied: [Bug 74071] [Chromium] Chromium should have EditorClientImpl::checkTextOfParagraph : [Attachment 118740] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Shinya Kawanaka
<shinyak at chromium.org>'s request for review:
Bug 74071: [Chromium] Chromium should have
EditorClientImpl::checkTextOfParagraph
https://bugs.webkit.org/show_bug.cgi?id=74071

Attachment 118740: Patch
https://bugs.webkit.org/attachment.cgi?id=118740&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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?