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

[bugzilla-daemon at webkit.org]

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





--- Comment #27 from MORITA Hajime <morrita at google.com>  2011-02-03 20:48:20 PST ---
Hi Darin, Thank you for reviewing again. 
I addressed the points.

> 
> > Source/WebKit/chromium/public/WebTextCheckCompletion.h:43
> > +class WebTextCheckCompletion {
> 
> nit: how about WebTextCheckingCompletion for consistency with the other interfaces, whose names begin with WebTextChecking?
Sure. Renamed

> 
> > 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.
Good point. I thought re-numbering ID is a bit redundant. But IDMap class helps.
Thank you for pointing this. I didn't know that.
Now removed identifier() method.

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

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

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

> 
> > Source/WebKit/chromium/public/WebViewClient.h:183
> > +    // WebView::respondTextCheck()
> 
> i think this comment is now wrong.  there is no respondTextCheck method.
Sure. removed.

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

> 
> > Source/WebKit/chromium/src/EditorClientImpl.h:194
> > +
> 
> nit: no extra line here
Done.

> 
> > 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.
I'm sorry for that. Moved.

> 
> > 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.
I tried, but it didn't happen. It looks that WEKIT_IMPLEMENTATION is not defined in DumpRenderTree.

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