[webkit-reviews] review denied: [Bug 92160] [BlackBerry] Support async spellcheck for the blackberry port : [Attachment 154142] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Nima Ghanavatian
<nima.ghanavatian at gmail.com>'s request for review:
Bug 92160: [BlackBerry] Support async spellcheck for the blackberry port
https://bugs.webkit.org/show_bug.cgi?id=92160

Attachment 154142: Patch
https://bugs.webkit.org/attachment.cgi?id=154142&action=review

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=154142&action=review


Looks good, still some stuff to fix.

> Source/WebKit/blackberry/ChangeLog:54
> +	   (WebCore):

I think something went wrong with the ChangeLogs, one should detail the
WebCore/ChangeLog changes, the other the changes in
WebKit/blackberry/ChangeLog. The git commit message should do the combination
of the two like above.

> Source/WebKit/blackberry/WebCoreSupport/EditorClientBlackBerry.cpp:43
> +class SpellChecker;

You don't need this.

> Source/WebKit/blackberry/WebCoreSupport/EditorClientBlackBerry.cpp:575
> +void EditorClientBlackBerry::checkTextOfParagraph(const UChar*, int,
WebCore::TextCheckingTypeMask, WTF::Vector<WebCore::TextCheckingResult>&)

You don't need some of the prefixes here, like WTF:: for Vector.

> Source/WebKit/blackberry/WebCoreSupport/EditorClientBlackBerry.h:88
> +    virtual void checkTextOfParagraph(const UChar*, int,
WebCore::TextCheckingTypeMask, WTF::Vector<WebCore::TextCheckingResult>&);

You don't need some of the prefixes here, like WTF:: for Vector.

> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:630
> +    if (frame) {

You can combine above two lines.

> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:632
> +	   if (editor)

Ditto.