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

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from MORITA Hajime <morrita at google.com>  2010-12-14 17:50:14 PST ---
Hi Levin, I'm sorry for the bad CC ... and thank you for reviewing anyway!

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

> 
> 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.)
Gave his own header and source files.

> 
> > 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.)
Tried another name. I'm not sure even the new name is sufficient though...

> 
> > WebKit/chromium/public/WebTextCheckingResponse.h:68
> > +    explicit WebTextCheckingResponse(int sequence, const WebVector<WebTextCheckingResult>& results)
> 
> explicit shouldn't be here.
Removed.

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

> 
> > 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()?
Oops. Done.

> 
> > 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.
I totally missed about this file...Thank you for pointing this. Fixed.

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

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