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

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from David Levin <levin at chromium.org>  2010-12-14 05:37:30 PST ---
(From update of attachment 76526)
View in context: https://bugs.webkit.org/attachment.cgi?id=76526&action=review

Here's a few things that I noticed when I looked at this change quickly.

> WebKit/chromium/public/WebTextCheckingResponse.h:40
> +// Note that type() is opaque from the client

This looks like it should be a comment in the class (near the type() method).

> WebKit/chromium/public/WebTextCheckingResponse.h:43
> +class WebTextCheckingResult {

I thought we did one class per header in public and the file would be named the same as the class but Darin would know absolutely. (If so, this class should move into its own header file.)

> WebKit/chromium/public/WebTextCheckingResponse.h:46
> +    static WebTextCheckingResult makeBadGrammar(int location, int length);

It isn't clear to me what these methods do. Perhaps that could be fixed by better methods names. (The method names don't make sense right now.)

> WebKit/chromium/public/WebTextCheckingResponse.h:68
> +    explicit WebTextCheckingResponse(int sequence, const WebVector<WebTextCheckingResult>& results)

explicit shouldn't be here.

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

misspelled: asyncronous

> WebKit/chromium/src/EditorClientImpl.cpp:892
> +        coreResults.append(SpellCheckingResult(static_cast<DocumentMarker::MarkerType>(results[i].type()), results[i].location(), results[0].length()));

Shouldn't results[0].length() be results[i].length()?

> WebKitTools/ChangeLog:47
> +

There are a lot of things wrong here. I think you'll see them when you look at it.  You may find it helpful if you review the diff of your changes before getting others to do it.

> WebKitTools/DumpRenderTree/chromium/WebViewHost.h:213
> +    // Spellcheck realted helper APIs

spelling: realted

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