[webkit-reviews] review denied: [Bug 109577] [WK2] Asynchronous spell checking implementation : [Attachment 189731] rebased after r143688

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 22 08:17:42 PST 2013


Anders Carlsson <andersca at apple.com> has denied Grzegorz Czajkowski
<g.czajkowski at samsung.com>'s request for review:
Bug 109577: [WK2] Asynchronous spell checking implementation
https://bugs.webkit.org/show_bug.cgi?id=109577

Attachment 189731: rebased after r143688
https://bugs.webkit.org/attachment.cgi?id=189731&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=189731&action=review


How is async text checking enabled? (My guess is that it's a setting in
WebCore?)

Patch looks good overall, but I'd like to see another patch with my suggested
changes incorporated.

> Source/WebKit2/UIProcess/TextCheckerCompletion.cpp:31
> +PassRefPtr<TextCheckerCompletionImpl>
TextCheckerCompletionImpl::create(uint64_t requestID, const
WebCore::TextCheckingRequestData& requestData, WebPageProxy* page)

Please use "using namespace WebCore;" so you don't have to use the WebCore::
qualifier here.

> Source/WebKit2/UIProcess/TextCheckerCompletion.cpp:36
> +TextCheckerCompletionImpl::TextCheckerCompletionImpl(uint64_t requestID,
const WebCore::TextCheckingRequestData& requestData, WebPageProxy* page)

Ditto.

> Source/WebKit2/UIProcess/TextCheckerCompletion.cpp:43
> +const WebCore::TextCheckingRequestData&
TextCheckerCompletionImpl::textCheckingRequestData() const

Ditto.

> Source/WebKit2/UIProcess/TextCheckerCompletion.cpp:53
> +void TextCheckerCompletionImpl::didFinishCheckingText(const
Vector<WebCore::TextCheckingResult>& result) const

Ditto.

> Source/WebKit2/UIProcess/TextCheckerCompletion.h:47
> +class TextCheckerCompletionImpl : public TextCheckerCompletion {

I don't understand why we need both a TextCheckerCompletion class and a
TextCheckerCompletionImpl class - can't we just have a single class?

> Source/WebKit2/UIProcess/TextCheckerCompletion.h:56
> +    virtual const WebCore::TextCheckingRequestData&
textCheckingRequestData() const OVERRIDE;
> +    virtual int64_t spellDocumentTag() OVERRIDE;
> +
> +    virtual void didFinishCheckingText(const
Vector<WebCore::TextCheckingResult>&) const OVERRIDE;
> +    virtual void didCancelCheckingText() const OVERRIDE;

These can probably all be private.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3783
> +void WebPage::addTextCheckingRequest(uint64_t requestID,
PassRefPtr<WebCore::TextCheckingRequest> request)

No need for WebCore:: here.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3788
> +void WebPage::didFinishCheckingText(uint64_t requestID, const
Vector<WebCore::TextCheckingResult>& result)

Ditto.


More information about the webkit-reviews mailing list