[webkit-reviews] review denied: [Bug 51013] [Chromium] Should implement EditorClientImpl::requestCheckingOfString() : [Attachment 81048] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied MORITA Hajime
<morrita at google.com>'s request for review:
Bug 51013: [Chromium] Should implement
EditorClientImpl::requestCheckingOfString()
https://bugs.webkit.org/show_bug.cgi?id=51013

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81048&action=review

> Source/WebKit/chromium/public/WebTextCheckCompletion.h:43
> +class WebTextCheckCompletion {

nit: how about WebTextCheckingCompletion for consistency with the other
interfaces, whose names begin with WebTextChecking?

> Source/WebKit/chromium/public/WebTextCheckCompletion.h:45
> +    virtual int identifier() const = 0;

it seems like identifier should just be a parameter of requestCheckingOfText
just as it
is a parameter to requestCheckingOfString.

or, can you tell me why you need to expose this identifier to Chromium?  you
could also
put WebTextCheckingCompletion objects in a base::IDMap within Chromium, and use
the ID
vended from that for IPC purposes.  It seems like the internal identifier used
by the
editing code could be replaced with an interface like WebTextCheckingCompletion
instead.

> Source/WebKit/chromium/public/WebTextCheckCompletion.h:46
> +    virtual void didRespondTextCheck(const
WebVector<WebTextCheckingResult>&) = 0;

nit: how about didFinishCheckingText?

> Source/WebKit/chromium/public/WebTextCheckingResult.h:43
> +	   Misspelling = 1 << 0,

nit: how about a more specific enum name, like "Error"?

then you can have:

  enum Error {
      ErrorSpelling = 1 << 0,
      ErrorGrammar  = 1 << 1
  };

And the field can be:

  Error error() const { return m_error; }

> Source/WebKit/chromium/public/WebViewClient.h:182
> +    // Requests asynchronous spell and grammar checking. The result should
be returned by 

nit: "spelling and grammar checking"

> Source/WebKit/chromium/public/WebViewClient.h:183
> +    // WebView::respondTextCheck()

i think this comment is now wrong.  there is no respondTextCheck method.

> Source/WebKit/chromium/public/WebViewClient.h:184
> +    virtual void requestTextCheck(const WebString&, WebTextCheckCompletion*)
{ }

requestTextCheck -> requestCheckingOfText perhaps?

> Source/WebKit/chromium/src/EditorClientImpl.h:194
> +

nit: no extra line here

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:414
> +static void invokeRespondLastTextCheck(void* context)

please put static helper methods at the top of the file, so that you don't
interrupt the flow of member function implementations.

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:436
> +	   m_spellcheck.spellCheckWord(WebString(text.characters(),
text.length()), &misspelledPosition, &misspelledLength);

I think you should be able to just pass a String here and the conversion to
WebString will happen automatically.