[Webkit-unassigned] [Bug 51013] [Chromium] Should implement EditorClientImpl::requestCheckingOfString()

[bugzilla-daemon at webkit.org]

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


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

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




--- Comment #25 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2011-02-03 14:48:42 PST ---
(From update of attachment 81048)
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.

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